| Message ID | 20200510072404.986627-4-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] xfs: use XFS_IFORK_BOFF xchk_bmap_check_rmaps | expand |
On Sunday 10 May 2020 12:54:01 PM IST Christoph Hellwig wrote: > xfs_ifree only need to free inline data in the data fork, as we've > already taken care of the attr fork before (and in fact freed the > fork structure). Just open code the freeing of the inline data. > xfs_inactive() => xfs_attr_inactive() would have freed the attr fork. Hence the changes made in this patch are correct. Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_inode.c | 30 ++++++++++-------------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 549ff468b7b60..7d3144dc99b72 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2711,24 +2711,6 @@ xfs_ifree_cluster( > return 0; > } > > -/* > - * Free any local-format buffers sitting around before we reset to > - * extents format. > - */ > -static inline void > -xfs_ifree_local_data( > - struct xfs_inode *ip, > - int whichfork) > -{ > - struct xfs_ifork *ifp; > - > - if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL) > - return; > - > - ifp = XFS_IFORK_PTR(ip, whichfork); > - xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); > -} > - > /* > * This is called to return an inode to the inode free list. > * The inode should already be truncated to 0 length and have > @@ -2765,8 +2747,16 @@ xfs_ifree( > if (error) > return error; > > - xfs_ifree_local_data(ip, XFS_DATA_FORK); > - xfs_ifree_local_data(ip, XFS_ATTR_FORK); > + /* > + * Free any local-format data sitting around before we reset the > + * data fork to extents format. Note that the attr fork data has > + * already been freed by xfs_attr_inactive. > + */ > + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) { > + kmem_free(ip->i_df.if_u1.if_data); > + ip->i_df.if_u1.if_data = NULL; > + ip->i_df.if_bytes = 0; > + } > > VFS_I(ip)->i_mode = 0; /* mark incore inode as free */ > ip->i_d.di_flags = 0; >
On Sun, May 10, 2020 at 09:24:01AM +0200, Christoph Hellwig wrote: > xfs_ifree only need to free inline data in the data fork, as we've > already taken care of the attr fork before (and in fact freed the > fork structure). Just open code the freeing of the inline data. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_inode.c | 30 ++++++++++-------------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 549ff468b7b60..7d3144dc99b72 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2711,24 +2711,6 @@ xfs_ifree_cluster( > return 0; > } > > -/* > - * Free any local-format buffers sitting around before we reset to > - * extents format. > - */ > -static inline void > -xfs_ifree_local_data( > - struct xfs_inode *ip, > - int whichfork) > -{ > - struct xfs_ifork *ifp; > - > - if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL) > - return; > - > - ifp = XFS_IFORK_PTR(ip, whichfork); > - xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); > -} > - > /* > * This is called to return an inode to the inode free list. > * The inode should already be truncated to 0 length and have > @@ -2765,8 +2747,16 @@ xfs_ifree( > if (error) > return error; > > - xfs_ifree_local_data(ip, XFS_DATA_FORK); > - xfs_ifree_local_data(ip, XFS_ATTR_FORK); > + /* > + * Free any local-format data sitting around before we reset the > + * data fork to extents format. Note that the attr fork data has > + * already been freed by xfs_attr_inactive. > + */ > + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) { > + kmem_free(ip->i_df.if_u1.if_data); > + ip->i_df.if_u1.if_data = NULL; > + ip->i_df.if_bytes = 0; > + } > > VFS_I(ip)->i_mode = 0; /* mark incore inode as free */ > ip->i_d.di_flags = 0; > -- > 2.26.2 >
On Sun, May 10, 2020 at 09:24:01AM +0200, Christoph Hellwig wrote: > xfs_ifree only need to free inline data in the data fork, as we've > already taken care of the attr fork before (and in fact freed the > fork structure). Just open code the freeing of the inline data. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/xfs_inode.c | 30 ++++++++++-------------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 549ff468b7b60..7d3144dc99b72 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2711,24 +2711,6 @@ xfs_ifree_cluster( > return 0; > } > > -/* > - * Free any local-format buffers sitting around before we reset to > - * extents format. > - */ > -static inline void > -xfs_ifree_local_data( > - struct xfs_inode *ip, > - int whichfork) > -{ > - struct xfs_ifork *ifp; > - > - if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL) > - return; > - > - ifp = XFS_IFORK_PTR(ip, whichfork); > - xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); > -} > - > /* > * This is called to return an inode to the inode free list. > * The inode should already be truncated to 0 length and have > @@ -2765,8 +2747,16 @@ xfs_ifree( > if (error) > return error; > > - xfs_ifree_local_data(ip, XFS_DATA_FORK); > - xfs_ifree_local_data(ip, XFS_ATTR_FORK); > + /* > + * Free any local-format data sitting around before we reset the > + * data fork to extents format. Note that the attr fork data has > + * already been freed by xfs_attr_inactive. > + */ > + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) { > + kmem_free(ip->i_df.if_u1.if_data); > + ip->i_df.if_u1.if_data = NULL; > + ip->i_df.if_bytes = 0; > + } > > VFS_I(ip)->i_mode = 0; /* mark incore inode as free */ > ip->i_d.di_flags = 0; > -- > 2.26.2 >
Hi Christoph, On Sat, May 16, 2020 at 9:07 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Sun, May 10, 2020 at 09:24:01AM +0200, Christoph Hellwig wrote: > > xfs_ifree only need to free inline data in the data fork, as we've > > already taken care of the attr fork before (and in fact freed the > > fork structure). Just open code the freeing of the inline data. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Looks ok, > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > --D > > > --- > > fs/xfs/xfs_inode.c | 30 ++++++++++-------------------- > > 1 file changed, 10 insertions(+), 20 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > index 549ff468b7b60..7d3144dc99b72 100644 > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -2711,24 +2711,6 @@ xfs_ifree_cluster( > > return 0; > > } > > > > -/* > > - * Free any local-format buffers sitting around before we reset to > > - * extents format. > > - */ > > -static inline void > > -xfs_ifree_local_data( > > - struct xfs_inode *ip, > > - int whichfork) > > -{ > > - struct xfs_ifork *ifp; > > - > > - if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL) > > - return; > > - > > - ifp = XFS_IFORK_PTR(ip, whichfork); > > - xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); > > -} > > - > > /* > > * This is called to return an inode to the inode free list. > > * The inode should already be truncated to 0 length and have > > @@ -2765,8 +2747,16 @@ xfs_ifree( > > if (error) > > return error; > > > > - xfs_ifree_local_data(ip, XFS_DATA_FORK); > > - xfs_ifree_local_data(ip, XFS_ATTR_FORK); > > + /* > > + * Free any local-format data sitting around before we reset the > > + * data fork to extents format. Note that the attr fork data has > > + * already been freed by xfs_attr_inactive. > > + */ > > + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) { > > + kmem_free(ip->i_df.if_u1.if_data); > > + ip->i_df.if_u1.if_data = NULL; > > + ip->i_df.if_bytes = 0; > > + } > > > > VFS_I(ip)->i_mode = 0; /* mark incore inode as free */ > > ip->i_d.di_flags = 0; > > -- > > 2.26.2 > > I stumbled upon this patch, by analyzing a kernel panic we had [1]. Looking at the call trace, the panic happened in xfs_ifree_local_data() being called with XFS_ATTR_FORK. It looks like ifp = XFS_IFORK_PTR(ip, whichfork); returned NULL. Based on your patch, do I understand correctly that it fixes the panic? What happened seems to be that inode had an attribute fork and xfs_attr_fork_remove() was called on it. This function set: ip->i_afp = NULL ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS. As a result, xfs_ifree_local_data() [2] checked XFS_IFORK_FORMAT() and got XFS_DINODE_FMT_EXTENTS. So it performed: ifp = XFS_IFORK_PTR(ip, whichfork); xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); which caused NULL pointer exception. Is this analysis correct, that any time we have an inode with an attribute fork, we will crash deleting it? Thanks, Alex. [1] BUG: unable to handle kernel NULL pointer dereference at (null) IP: xfs_ifree+0x12c/0x150 [xfs] PGD 800000066eea0067 P4D 800000066eea0067 PUD 3066ce067 PMD 0 Oops: 0000 [#1] PREEMPT SMP PTI Modules linked in: binfmt_misc xt_nat xfs(OE) sd_mod sg bonding xt_CHECKSUM i libiscsi_tcp(OE) sunrpc libiscsi(OE) scsi_transport_iscsi(OE) i6300esb ip_ta CPU: 3 PID: 19176 Comm: swift-object-re Tainted: G W OE 4.14.99-zad Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 0 task: ffffa009b0d45640 task.stack: ffffb0a85dddc000 RIP: 0010:xfs_ifree+0x12c/0x150 [xfs] RSP: 0018:ffffb0a85dddfdb8 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffffa00999610f00 RCX: 000000168215a803 RDX: 0000000000000000 RSI: ffffd0a83f9b3ed0 RDI: ffffa00999610f00 RBP: ffffa00edbe1af30 R08: 00003091b4833ed0 R09: ffffffffc0a39de4 R10: 0000000000000000 R11: 0000000000000040 R12: ffffb0a85dddfe10 R13: ffffb0a85dddfe08 R14: ffffa00d7f147780 R15: 0000000000000000 FS: 00007f012ffff700(0000) GS:ffffa0168b180000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000000 CR3: 0000000749f26006 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: xfs_inactive_ifree+0xbb/0x220 [xfs] xfs_inactive+0x74/0x100 [xfs] xfs_fs_destroy_inode+0xb4/0x240 [xfs] do_unlinkat+0x1b3/0x310 do_syscall_64+0x6e/0x120 entry_SYSCALL_64_after_hwframe+0x3d/0xa2 [2] static inline void xfs_ifree_local_data( struct xfs_inode *ip, int whichfork) { struct xfs_ifork *ifp; if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL) return; ifp = XFS_IFORK_PTR(ip, whichfork); xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); }
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 549ff468b7b60..7d3144dc99b72 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -2711,24 +2711,6 @@ xfs_ifree_cluster( return 0; } -/* - * Free any local-format buffers sitting around before we reset to - * extents format. - */ -static inline void -xfs_ifree_local_data( - struct xfs_inode *ip, - int whichfork) -{ - struct xfs_ifork *ifp; - - if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL) - return; - - ifp = XFS_IFORK_PTR(ip, whichfork); - xfs_idata_realloc(ip, -ifp->if_bytes, whichfork); -} - /* * This is called to return an inode to the inode free list. * The inode should already be truncated to 0 length and have @@ -2765,8 +2747,16 @@ xfs_ifree( if (error) return error; - xfs_ifree_local_data(ip, XFS_DATA_FORK); - xfs_ifree_local_data(ip, XFS_ATTR_FORK); + /* + * Free any local-format data sitting around before we reset the + * data fork to extents format. Note that the attr fork data has + * already been freed by xfs_attr_inactive. + */ + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) { + kmem_free(ip->i_df.if_u1.if_data); + ip->i_df.if_u1.if_data = NULL; + ip->i_df.if_bytes = 0; + } VFS_I(ip)->i_mode = 0; /* mark incore inode as free */ ip->i_d.di_flags = 0;
xfs_ifree only need to free inline data in the data fork, as we've already taken care of the attr fork before (and in fact freed the fork structure). Just open code the freeing of the inline data. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_inode.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-)