On Tue, Apr 03, 2018 at 02:03:58PM +1000, Dave Chinner wrote:
> On Mon, Apr 02, 2018 at 08:06:47PM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 03, 2018 at 11:44:58AM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > >
> > > When an intent is aborted during it's initial commit through
> > > xfs_defer_trans_abort(), there is a use after free. The current
> > > report is for a RUI through this path in generic/388:
> > >
> > > Freed by task 6274:
> > > __kasan_slab_free+0x136/0x180
> > > kmem_cache_free+0xe7/0x4b0
> > > xfs_trans_free_items+0x198/0x2e0
> > > __xfs_trans_commit+0x27f/0xcc0
> > > xfs_trans_roll+0x17b/0x2a0
> > > xfs_defer_trans_roll+0x6ad/0xe60
> > > xfs_defer_finish+0x2a6/0x2140
> > > xfs_alloc_file_space+0x53a/0xf90
> > > xfs_file_fallocate+0x5c6/0xac0
> > > vfs_fallocate+0x2f5/0x930
> > > ioctl_preallocate+0x1dc/0x320
> > > do_vfs_ioctl+0xfe4/0x1690
> > >
> > > The problem is that the RUI has two active references - one in the
> > > current transaction, and another held by the defer_ops structure
> > > that is passed to the RUD (intent done) so that both the intent and
> > > the intent done structures are freed on commit of the intent done.
> > >
> > > Hence during abort, we need to release the intent item, because the
> > > defer_ops reference is released separately via ->abort_intent
> > > callback. Fix all the intent code to do this correctly.
> > >
> > > Signed-Off-By: Dave Chinner <dchinner@xxxxxxxxxx>
> .....
> > > @@ -142,7 +161,7 @@ xfs_bui_item_unlock(
> > > struct xfs_log_item *lip)
> > > {
> > > if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> >
> > Hm, in my tree this is still "if (lip->li_flags & XFS_LI_ABORTED)" :)
> > /me wonders where the test_bit() came from?
>
> Oh, sorry, I just dumped the patch on the end of my stack, which has
> this conversion in it (because we do a bunch of racy updates on the
> log item flags with any serialisation and that needs to be fixed to
> get rid of log item descriptors).
>
> Change them all to
>
> if (lip->li_flags & XFS_LI_ABORTED)
>
> and it'll apply to the for-next tree.
>
This also needs comment updates for all of the unlock handlers that
currently explain why we free the item directly (which should now
explain that the caller is responsible for the additional reference).
Brian
> > But this is the same change that I came up with to solve the problem, so
> > I guess I'll go feed it to the test machines and see how they fare
> > overnight, and mash on it harder with generic/388 tomorrow.
>
> Been running generic/388 in a loop and nothing seems to have gone
> wrong yet.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
