Hi Christoph,
On Sat, May 16, 2020 at 9:07 PM Darrick J. Wong <darrick.wong@xxxxxxxxxx> 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@xxxxxx>
>
> Looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> --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);
}
