| Message ID | 20231116022833.121551-9-leah.rumancik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [5.15,01/17] xfs: refactor buffer cancellation table allocation | expand |
On Wed, Nov 15, 2023 at 06:28:25PM -0800, Leah Rumancik wrote: > From: ChenXiaoSong <chenxiaosong2@huawei.com> > > [ Upstream commit 001c179c4e26d04db8c9f5e3fef9558b58356be6 ] > > Reproducer: > 1. fallocate -l 100M image > 2. mkfs.xfs -f image > 3. mount image /mnt > 4. setxattr("/mnt", "trusted.overlay.upper", NULL, 0, XATTR_CREATE) > 5. char arg[32] = "\x01\xff\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00" > "\x00\x00\x00\x00\x00\x08\x00\x00\x00\xc6\x2a\xf7"; > fd = open("/mnt", O_RDONLY|O_DIRECTORY); > ioctl(fd, _IOC(_IOC_READ|_IOC_WRITE, 0x58, 0x2c, 0x20), arg); > > NULL pointer dereference will occur when race happens between xfs_getbmap() > and xfs_bmap_set_attrforkoff(): > > ioctl | setxattr > ----------------------------|--------------------------- > xfs_getbmap | > xfs_ifork_ptr | > xfs_inode_has_attr_fork | > ip->i_forkoff == 0 | > return NULL | > ifp == NULL | > | xfs_bmap_set_attrforkoff > | ip->i_forkoff > 0 > xfs_inode_has_attr_fork | > ip->i_forkoff > 0 | > ifp == NULL | > ifp->if_format | > > Fix this by locking i_lock before xfs_ifork_ptr(). > > Fixes: abbf9e8a4507 ("xfs: rewrite getbmap using the xfs_iext_* helpers") > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > Signed-off-by: Guo Xuenan <guoxuenan@huawei.com> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > [djwong: added fixes tag] > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > Acked-by: Chandan Babu R <chandanbabu@kernel.org> > --- > fs/xfs/xfs_bmap_util.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > index fd2ad6a3019c..bea6cc26abf9 100644 > --- a/fs/xfs/xfs_bmap_util.c > +++ b/fs/xfs/xfs_bmap_util.c > @@ -439,29 +439,28 @@ xfs_getbmap( > whichfork = XFS_COW_FORK; > else > whichfork = XFS_DATA_FORK; > - ifp = XFS_IFORK_PTR(ip, whichfork); > > xfs_ilock(ip, XFS_IOLOCK_SHARED); > switch (whichfork) { > case XFS_ATTR_FORK: > + lock = xfs_ilock_attr_map_shared(ip); > if (!XFS_IFORK_Q(ip)) > - goto out_unlock_iolock; > + goto out_unlock_ilock; > > max_len = 1LL << 32; > - lock = xfs_ilock_attr_map_shared(ip); > break; > case XFS_COW_FORK: > + lock = XFS_ILOCK_SHARED; > + xfs_ilock(ip, lock); > + > /* No CoW fork? Just return */ > - if (!ifp) > - goto out_unlock_iolock; > + if (!XFS_IFORK_PTR(ip, whichfork)) > + goto out_unlock_ilock; > > if (xfs_get_cowextsz_hint(ip)) > max_len = mp->m_super->s_maxbytes; > else > max_len = XFS_ISIZE(ip); > - > - lock = XFS_ILOCK_SHARED; > - xfs_ilock(ip, lock); > break; > case XFS_DATA_FORK: > if (!(iflags & BMV_IF_DELALLOC) && > @@ -491,6 +490,8 @@ xfs_getbmap( > break; > } > > + ifp = XFS_IFORK_PTR(ip, whichfork); > + > switch (ifp->if_format) { > case XFS_DINODE_FMT_EXTENTS: > case XFS_DINODE_FMT_BTREE: > -- > 2.43.0.rc0.421.g78406f8d94-goog > This patch breaks the build, how was it tested? fs/xfs/xfs_bmap_util.c: In function ‘xfs_getbmap’: fs/xfs/xfs_bmap_util.c:457:21: error: the comparison will always evaluate as ‘true’ for the address of ‘i_df’ will never be NULL [-Werror=address] 457 | if (!XFS_IFORK_PTR(ip, whichfork)) | ^ In file included from fs/xfs/xfs_bmap_util.c:16: fs/xfs/xfs_inode.h:38:33: note: ‘i_df’ declared here 38 | struct xfs_ifork i_df; /* data fork */ | ^~~~ cc1: all warnings being treated as errors
On Mon, Nov 20, 2023 at 04:38:24PM +0100, Greg KH wrote: > On Wed, Nov 15, 2023 at 06:28:25PM -0800, Leah Rumancik wrote: > > From: ChenXiaoSong <chenxiaosong2@huawei.com> > > > > [ Upstream commit 001c179c4e26d04db8c9f5e3fef9558b58356be6 ] > > > > Reproducer: > > 1. fallocate -l 100M image > > 2. mkfs.xfs -f image > > 3. mount image /mnt > > 4. setxattr("/mnt", "trusted.overlay.upper", NULL, 0, XATTR_CREATE) > > 5. char arg[32] = "\x01\xff\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00" > > "\x00\x00\x00\x00\x00\x08\x00\x00\x00\xc6\x2a\xf7"; > > fd = open("/mnt", O_RDONLY|O_DIRECTORY); > > ioctl(fd, _IOC(_IOC_READ|_IOC_WRITE, 0x58, 0x2c, 0x20), arg); > > > > NULL pointer dereference will occur when race happens between xfs_getbmap() > > and xfs_bmap_set_attrforkoff(): > > > > ioctl | setxattr > > ----------------------------|--------------------------- > > xfs_getbmap | > > xfs_ifork_ptr | > > xfs_inode_has_attr_fork | > > ip->i_forkoff == 0 | > > return NULL | > > ifp == NULL | > > | xfs_bmap_set_attrforkoff > > | ip->i_forkoff > 0 > > xfs_inode_has_attr_fork | > > ip->i_forkoff > 0 | > > ifp == NULL | > > ifp->if_format | > > > > Fix this by locking i_lock before xfs_ifork_ptr(). > > > > Fixes: abbf9e8a4507 ("xfs: rewrite getbmap using the xfs_iext_* helpers") > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > > Signed-off-by: Guo Xuenan <guoxuenan@huawei.com> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > [djwong: added fixes tag] > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > > Acked-by: Chandan Babu R <chandanbabu@kernel.org> > > --- > > fs/xfs/xfs_bmap_util.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > index fd2ad6a3019c..bea6cc26abf9 100644 > > --- a/fs/xfs/xfs_bmap_util.c > > +++ b/fs/xfs/xfs_bmap_util.c > > @@ -439,29 +439,28 @@ xfs_getbmap( > > whichfork = XFS_COW_FORK; > > else > > whichfork = XFS_DATA_FORK; > > - ifp = XFS_IFORK_PTR(ip, whichfork); > > > > xfs_ilock(ip, XFS_IOLOCK_SHARED); > > switch (whichfork) { > > case XFS_ATTR_FORK: > > + lock = xfs_ilock_attr_map_shared(ip); > > if (!XFS_IFORK_Q(ip)) > > - goto out_unlock_iolock; > > + goto out_unlock_ilock; > > > > max_len = 1LL << 32; > > - lock = xfs_ilock_attr_map_shared(ip); > > break; > > case XFS_COW_FORK: > > + lock = XFS_ILOCK_SHARED; > > + xfs_ilock(ip, lock); > > + > > /* No CoW fork? Just return */ > > - if (!ifp) > > - goto out_unlock_iolock; > > + if (!XFS_IFORK_PTR(ip, whichfork)) Is this the line 457 that the compiler complains about below? If so, then whichfork == XFS_COW_FORK here, because the code just switch()d on that. The XFS_IFORK_PTR macro decomposes into: #define XFS_IFORK_PTR(ip,w) \ ((w) == XFS_DATA_FORK ? \ &(ip)->i_df : \ ((w) == XFS_ATTR_FORK ? \ (ip)->i_afp : \ (ip)->i_cowfp)) which means this test /should/ be turning into: if (!(ip->i_cowfp)) goto out_unlock_ilock; I'm not sure why your compiler is whining about &ip->i_df; that's not what's being selected here for testing. Unless your compiler is somehow deciding that XFS_DATA_FORK == XFS_COW_FORK? This should not ever be possible. --D > > + goto out_unlock_ilock; > > > > if (xfs_get_cowextsz_hint(ip)) > > max_len = mp->m_super->s_maxbytes; > > else > > max_len = XFS_ISIZE(ip); > > - > > - lock = XFS_ILOCK_SHARED; > > - xfs_ilock(ip, lock); > > break; > > case XFS_DATA_FORK: > > if (!(iflags & BMV_IF_DELALLOC) && > > @@ -491,6 +490,8 @@ xfs_getbmap( > > break; > > } > > > > + ifp = XFS_IFORK_PTR(ip, whichfork); > > + > > switch (ifp->if_format) { > > case XFS_DINODE_FMT_EXTENTS: > > case XFS_DINODE_FMT_BTREE: > > -- > > 2.43.0.rc0.421.g78406f8d94-goog > > > > This patch breaks the build, how was it tested? > > fs/xfs/xfs_bmap_util.c: In function ‘xfs_getbmap’: > fs/xfs/xfs_bmap_util.c:457:21: error: the comparison will always evaluate as ‘true’ for the address of ‘i_df’ will never be NULL [-Werror=address] > 457 | if (!XFS_IFORK_PTR(ip, whichfork)) > | ^ > In file included from fs/xfs/xfs_bmap_util.c:16: > fs/xfs/xfs_inode.h:38:33: note: ‘i_df’ declared here > 38 | struct xfs_ifork i_df; /* data fork */ > | ^~~~ > cc1: all warnings being treated as errors > >
On Mon, Nov 20, 2023 at 04:38:24PM +0100, Greg KH wrote: >On Wed, Nov 15, 2023 at 06:28:25PM -0800, Leah Rumancik wrote: >> From: ChenXiaoSong <chenxiaosong2@huawei.com> >> >> [ Upstream commit 001c179c4e26d04db8c9f5e3fef9558b58356be6 ] >> >> Reproducer: >> 1. fallocate -l 100M image >> 2. mkfs.xfs -f image >> 3. mount image /mnt >> 4. setxattr("/mnt", "trusted.overlay.upper", NULL, 0, XATTR_CREATE) >> 5. char arg[32] = "\x01\xff\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00" >> "\x00\x00\x00\x00\x00\x08\x00\x00\x00\xc6\x2a\xf7"; >> fd = open("/mnt", O_RDONLY|O_DIRECTORY); >> ioctl(fd, _IOC(_IOC_READ|_IOC_WRITE, 0x58, 0x2c, 0x20), arg); >> >> NULL pointer dereference will occur when race happens between xfs_getbmap() >> and xfs_bmap_set_attrforkoff(): >> >> ioctl | setxattr >> ----------------------------|--------------------------- >> xfs_getbmap | >> xfs_ifork_ptr | >> xfs_inode_has_attr_fork | >> ip->i_forkoff == 0 | >> return NULL | >> ifp == NULL | >> | xfs_bmap_set_attrforkoff >> | ip->i_forkoff > 0 >> xfs_inode_has_attr_fork | >> ip->i_forkoff > 0 | >> ifp == NULL | >> ifp->if_format | >> >> Fix this by locking i_lock before xfs_ifork_ptr(). >> >> Fixes: abbf9e8a4507 ("xfs: rewrite getbmap using the xfs_iext_* helpers") >> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> >> Signed-off-by: Guo Xuenan <guoxuenan@huawei.com> >> Reviewed-by: Darrick J. Wong <djwong@kernel.org> >> [djwong: added fixes tag] >> Signed-off-by: Darrick J. Wong <djwong@kernel.org> >> Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> >> Acked-by: Chandan Babu R <chandanbabu@kernel.org> >> --- >> fs/xfs/xfs_bmap_util.c | 17 +++++++++-------- >> 1 file changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c >> index fd2ad6a3019c..bea6cc26abf9 100644 >> --- a/fs/xfs/xfs_bmap_util.c >> +++ b/fs/xfs/xfs_bmap_util.c >> @@ -439,29 +439,28 @@ xfs_getbmap( >> whichfork = XFS_COW_FORK; >> else >> whichfork = XFS_DATA_FORK; >> - ifp = XFS_IFORK_PTR(ip, whichfork); >> >> xfs_ilock(ip, XFS_IOLOCK_SHARED); >> switch (whichfork) { >> case XFS_ATTR_FORK: >> + lock = xfs_ilock_attr_map_shared(ip); >> if (!XFS_IFORK_Q(ip)) >> - goto out_unlock_iolock; >> + goto out_unlock_ilock; >> >> max_len = 1LL << 32; >> - lock = xfs_ilock_attr_map_shared(ip); >> break; >> case XFS_COW_FORK: >> + lock = XFS_ILOCK_SHARED; >> + xfs_ilock(ip, lock); >> + >> /* No CoW fork? Just return */ >> - if (!ifp) >> - goto out_unlock_iolock; >> + if (!XFS_IFORK_PTR(ip, whichfork)) >> + goto out_unlock_ilock; >> >> if (xfs_get_cowextsz_hint(ip)) >> max_len = mp->m_super->s_maxbytes; >> else >> max_len = XFS_ISIZE(ip); >> - >> - lock = XFS_ILOCK_SHARED; >> - xfs_ilock(ip, lock); >> break; >> case XFS_DATA_FORK: >> if (!(iflags & BMV_IF_DELALLOC) && >> @@ -491,6 +490,8 @@ xfs_getbmap( >> break; >> } >> >> + ifp = XFS_IFORK_PTR(ip, whichfork); >> + >> switch (ifp->if_format) { >> case XFS_DINODE_FMT_EXTENTS: >> case XFS_DINODE_FMT_BTREE: >> -- >> 2.43.0.rc0.421.g78406f8d94-goog >> > >This patch breaks the build, how was it tested? > >fs/xfs/xfs_bmap_util.c: In function ‘xfs_getbmap’: >fs/xfs/xfs_bmap_util.c:457:21: error: the comparison will always evaluate as ‘true’ for the address of ‘i_df’ will never be NULL [-Werror=address] > 457 | if (!XFS_IFORK_PTR(ip, whichfork)) > | ^ >In file included from fs/xfs/xfs_bmap_util.c:16: >fs/xfs/xfs_inode.h:38:33: note: ‘i_df’ declared here > 38 | struct xfs_ifork i_df; /* data fork */ > | ^~~~ >cc1: all warnings being treated as errors That's odd. I actually ended up queueing these patches earlier, and I don't see any such warnings. Looking at the code, this is a bit weird too - do you see these warnings with the current 5.15 queue?
On Mon, Nov 20, 2023 at 09:50:45PM -0500, Sasha Levin wrote: > On Mon, Nov 20, 2023 at 04:38:24PM +0100, Greg KH wrote: > > On Wed, Nov 15, 2023 at 06:28:25PM -0800, Leah Rumancik wrote: > > > From: ChenXiaoSong <chenxiaosong2@huawei.com> > > > > > > [ Upstream commit 001c179c4e26d04db8c9f5e3fef9558b58356be6 ] > > > > > > Reproducer: > > > 1. fallocate -l 100M image > > > 2. mkfs.xfs -f image > > > 3. mount image /mnt > > > 4. setxattr("/mnt", "trusted.overlay.upper", NULL, 0, XATTR_CREATE) > > > 5. char arg[32] = "\x01\xff\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00" > > > "\x00\x00\x00\x00\x00\x08\x00\x00\x00\xc6\x2a\xf7"; > > > fd = open("/mnt", O_RDONLY|O_DIRECTORY); > > > ioctl(fd, _IOC(_IOC_READ|_IOC_WRITE, 0x58, 0x2c, 0x20), arg); > > > > > > NULL pointer dereference will occur when race happens between xfs_getbmap() > > > and xfs_bmap_set_attrforkoff(): > > > > > > ioctl | setxattr > > > ----------------------------|--------------------------- > > > xfs_getbmap | > > > xfs_ifork_ptr | > > > xfs_inode_has_attr_fork | > > > ip->i_forkoff == 0 | > > > return NULL | > > > ifp == NULL | > > > | xfs_bmap_set_attrforkoff > > > | ip->i_forkoff > 0 > > > xfs_inode_has_attr_fork | > > > ip->i_forkoff > 0 | > > > ifp == NULL | > > > ifp->if_format | > > > > > > Fix this by locking i_lock before xfs_ifork_ptr(). > > > > > > Fixes: abbf9e8a4507 ("xfs: rewrite getbmap using the xfs_iext_* helpers") > > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > > > Signed-off-by: Guo Xuenan <guoxuenan@huawei.com> > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > > [djwong: added fixes tag] > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > > > Acked-by: Chandan Babu R <chandanbabu@kernel.org> > > > --- > > > fs/xfs/xfs_bmap_util.c | 17 +++++++++-------- > > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > > index fd2ad6a3019c..bea6cc26abf9 100644 > > > --- a/fs/xfs/xfs_bmap_util.c > > > +++ b/fs/xfs/xfs_bmap_util.c > > > @@ -439,29 +439,28 @@ xfs_getbmap( > > > whichfork = XFS_COW_FORK; > > > else > > > whichfork = XFS_DATA_FORK; > > > - ifp = XFS_IFORK_PTR(ip, whichfork); > > > > > > xfs_ilock(ip, XFS_IOLOCK_SHARED); > > > switch (whichfork) { > > > case XFS_ATTR_FORK: > > > + lock = xfs_ilock_attr_map_shared(ip); > > > if (!XFS_IFORK_Q(ip)) > > > - goto out_unlock_iolock; > > > + goto out_unlock_ilock; > > > > > > max_len = 1LL << 32; > > > - lock = xfs_ilock_attr_map_shared(ip); > > > break; > > > case XFS_COW_FORK: > > > + lock = XFS_ILOCK_SHARED; > > > + xfs_ilock(ip, lock); > > > + > > > /* No CoW fork? Just return */ > > > - if (!ifp) > > > - goto out_unlock_iolock; > > > + if (!XFS_IFORK_PTR(ip, whichfork)) > > > + goto out_unlock_ilock; > > > > > > if (xfs_get_cowextsz_hint(ip)) > > > max_len = mp->m_super->s_maxbytes; > > > else > > > max_len = XFS_ISIZE(ip); > > > - > > > - lock = XFS_ILOCK_SHARED; > > > - xfs_ilock(ip, lock); > > > break; > > > case XFS_DATA_FORK: > > > if (!(iflags & BMV_IF_DELALLOC) && > > > @@ -491,6 +490,8 @@ xfs_getbmap( > > > break; > > > } > > > > > > + ifp = XFS_IFORK_PTR(ip, whichfork); > > > + > > > switch (ifp->if_format) { > > > case XFS_DINODE_FMT_EXTENTS: > > > case XFS_DINODE_FMT_BTREE: > > > -- > > > 2.43.0.rc0.421.g78406f8d94-goog > > > > > > > This patch breaks the build, how was it tested? > > > > fs/xfs/xfs_bmap_util.c: In function ‘xfs_getbmap’: > > fs/xfs/xfs_bmap_util.c:457:21: error: the comparison will always evaluate as ‘true’ for the address of ‘i_df’ will never be NULL [-Werror=address] > > 457 | if (!XFS_IFORK_PTR(ip, whichfork)) > > | ^ > > In file included from fs/xfs/xfs_bmap_util.c:16: > > fs/xfs/xfs_inode.h:38:33: note: ‘i_df’ declared here > > 38 | struct xfs_ifork i_df; /* data fork */ > > | ^~~~ > > cc1: all warnings being treated as errors > > That's odd. I actually ended up queueing these patches earlier, and I > don't see any such warnings. > > Looking at the code, this is a bit weird too - do you see these warnings > with the current 5.15 queue? I did, that's where I saw this, so I dropped this commit from there, it failed my builds using gcc-12. thanks, greg k-h
On Mon, Nov 20, 2023 at 11:11:30AM -0800, Darrick J. Wong wrote: > On Mon, Nov 20, 2023 at 04:38:24PM +0100, Greg KH wrote: > > On Wed, Nov 15, 2023 at 06:28:25PM -0800, Leah Rumancik wrote: > > > From: ChenXiaoSong <chenxiaosong2@huawei.com> > > > > > > [ Upstream commit 001c179c4e26d04db8c9f5e3fef9558b58356be6 ] > > > > > > Reproducer: > > > 1. fallocate -l 100M image > > > 2. mkfs.xfs -f image > > > 3. mount image /mnt > > > 4. setxattr("/mnt", "trusted.overlay.upper", NULL, 0, XATTR_CREATE) > > > 5. char arg[32] = "\x01\xff\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00" > > > "\x00\x00\x00\x00\x00\x08\x00\x00\x00\xc6\x2a\xf7"; > > > fd = open("/mnt", O_RDONLY|O_DIRECTORY); > > > ioctl(fd, _IOC(_IOC_READ|_IOC_WRITE, 0x58, 0x2c, 0x20), arg); > > > > > > NULL pointer dereference will occur when race happens between xfs_getbmap() > > > and xfs_bmap_set_attrforkoff(): > > > > > > ioctl | setxattr > > > ----------------------------|--------------------------- > > > xfs_getbmap | > > > xfs_ifork_ptr | > > > xfs_inode_has_attr_fork | > > > ip->i_forkoff == 0 | > > > return NULL | > > > ifp == NULL | > > > | xfs_bmap_set_attrforkoff > > > | ip->i_forkoff > 0 > > > xfs_inode_has_attr_fork | > > > ip->i_forkoff > 0 | > > > ifp == NULL | > > > ifp->if_format | > > > > > > Fix this by locking i_lock before xfs_ifork_ptr(). > > > > > > Fixes: abbf9e8a4507 ("xfs: rewrite getbmap using the xfs_iext_* helpers") > > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com> > > > Signed-off-by: Guo Xuenan <guoxuenan@huawei.com> > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > > [djwong: added fixes tag] > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com> > > > Acked-by: Chandan Babu R <chandanbabu@kernel.org> > > > --- > > > fs/xfs/xfs_bmap_util.c | 17 +++++++++-------- > > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > > index fd2ad6a3019c..bea6cc26abf9 100644 > > > --- a/fs/xfs/xfs_bmap_util.c > > > +++ b/fs/xfs/xfs_bmap_util.c > > > @@ -439,29 +439,28 @@ xfs_getbmap( > > > whichfork = XFS_COW_FORK; > > > else > > > whichfork = XFS_DATA_FORK; > > > - ifp = XFS_IFORK_PTR(ip, whichfork); > > > > > > xfs_ilock(ip, XFS_IOLOCK_SHARED); > > > switch (whichfork) { > > > case XFS_ATTR_FORK: > > > + lock = xfs_ilock_attr_map_shared(ip); > > > if (!XFS_IFORK_Q(ip)) > > > - goto out_unlock_iolock; > > > + goto out_unlock_ilock; > > > > > > max_len = 1LL << 32; > > > - lock = xfs_ilock_attr_map_shared(ip); > > > break; > > > case XFS_COW_FORK: > > > + lock = XFS_ILOCK_SHARED; > > > + xfs_ilock(ip, lock); > > > + > > > /* No CoW fork? Just return */ > > > - if (!ifp) > > > - goto out_unlock_iolock; > > > + if (!XFS_IFORK_PTR(ip, whichfork)) > > Is this the line 457 that the compiler complains about below? > > If so, then whichfork == XFS_COW_FORK here, because the code just > switch()d on that. The XFS_IFORK_PTR macro decomposes into: > > #define XFS_IFORK_PTR(ip,w) \ > ((w) == XFS_DATA_FORK ? \ > &(ip)->i_df : \ > ((w) == XFS_ATTR_FORK ? \ > (ip)->i_afp : \ > (ip)->i_cowfp)) > > which means this test /should/ be turning into: > > if (!(ip->i_cowfp)) > goto out_unlock_ilock; > > I'm not sure why your compiler is whining about &ip->i_df; that's not > what's being selected here for testing. Unless your compiler is somehow > deciding that XFS_DATA_FORK == XFS_COW_FORK? This should not ever be > possible. This is using gcc-12, and gcc-13, no idea what happened, just that it throws up that warning which stops my builds :( thanks, greg k-h
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index fd2ad6a3019c..bea6cc26abf9 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -439,29 +439,28 @@ xfs_getbmap( whichfork = XFS_COW_FORK; else whichfork = XFS_DATA_FORK; - ifp = XFS_IFORK_PTR(ip, whichfork); xfs_ilock(ip, XFS_IOLOCK_SHARED); switch (whichfork) { case XFS_ATTR_FORK: + lock = xfs_ilock_attr_map_shared(ip); if (!XFS_IFORK_Q(ip)) - goto out_unlock_iolock; + goto out_unlock_ilock; max_len = 1LL << 32; - lock = xfs_ilock_attr_map_shared(ip); break; case XFS_COW_FORK: + lock = XFS_ILOCK_SHARED; + xfs_ilock(ip, lock); + /* No CoW fork? Just return */ - if (!ifp) - goto out_unlock_iolock; + if (!XFS_IFORK_PTR(ip, whichfork)) + goto out_unlock_ilock; if (xfs_get_cowextsz_hint(ip)) max_len = mp->m_super->s_maxbytes; else max_len = XFS_ISIZE(ip); - - lock = XFS_ILOCK_SHARED; - xfs_ilock(ip, lock); break; case XFS_DATA_FORK: if (!(iflags & BMV_IF_DELALLOC) && @@ -491,6 +490,8 @@ xfs_getbmap( break; } + ifp = XFS_IFORK_PTR(ip, whichfork); + switch (ifp->if_format) { case XFS_DINODE_FMT_EXTENTS: case XFS_DINODE_FMT_BTREE: