Re: [PATCH v3 3/4] xfs: avoid dereferencing log items after push callbacks

[Yuto Ohnuki <ytohnuki@amazon.com>]

> How difficult would it be to add a refcount to xfs_log_item so that any
> other code walking through the AIL's log item list can't accidentally
> suffer from this UAF?  I keep seeing periodic log item UAF bugfixes on
> the list, which (to me anyway) suggests we ought to think about a
> struct(ural) fix to this problem.
> 
> I /think/ the answer to that is "sort of nasty" because of things like
> xfs_dquot embedding its own log item.  The other log item types might
> not be so bad because at least they're allocated separately.  However,
> refcount_t accesses also aren't free.

Agreed that a structural fix would be the right long-term approach.
As you noted, the dquot embedding makes it non-trivial. I'd like to
keep this series focused on the immediate syzbot fix and explore a
refcount-based approach as a separate effort.

> This is true after the xfsaild_push_item call, correct?  If so then I
> think the comment for the call needs updating too:
> 
>       /*
>        * Note that iop_push may unlock and reacquire the AIL lock.  We
>        * rely on the AIL cursor implementation to be able to deal with
>        * the dropped lock.
>        *
>        * The log item may have been freed by the push, so it must not
>        * be accessed or dereferenced below this line.
>        */
>       lock_result = xfsaild_push_item(ailp, lip);
> 
> Otherwise this looks ok to me.
> 
> --D

Thank you. In v4, I have added the comments you suggested.

Yuto



Amazon Web Services EMEA SARL, 38 avenue John F. Kennedy, L-1855 Luxembourg, R.C.S. Luxembourg B186284

Amazon Web Services EMEA SARL, Irish Branch, One Burlington Plaza, Burlington Road, Dublin 4, Ireland, branch registration number 908705