| Message ID | 20240823110439.1585041-5-leo.lilong@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: fix and cleanups for log item push | expand |
On Fri, Aug 23, 2024 at 07:04:38PM +0800, Long Li wrote: > If errors are encountered while pushing a dquot log item, the dquot dirty > flag is cleared. Without the protection of dqlock and dqflock locks, the > dquot reclaim thread may free the dquot. Accessing the log item in xfsaild > after this can trigger a UAF. > > CPU0 CPU1 > push item reclaim dquot > ----------------------- ----------------------- > xfsaild_push_item > xfs_qm_dquot_logitem_push(lip) > xfs_dqlock_nowait(dqp) > xfs_dqflock_nowait(dqp) > spin_unlock(&lip->li_ailp->ail_lock) > xfs_qm_dqflush(dqp, &bp) > <encountered some errors> > xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE) > dqp->q_flags &= ~XFS_DQFLAG_DIRTY > <dquot is not diry> > xfs_trans_ail_delete(lip, 0) > xfs_dqfunlock(dqp) > spin_lock(&lip->li_ailp->ail_lock) > xfs_dqunlock(dqp) > xfs_qm_shrink_scan > list_lru_shrink_walk > xfs_qm_dquot_isolate > xfs_dqlock_nowait(dqp) > xfs_dqfunlock(dqp) > //dquot is clean, not flush it > xfs_dqfunlock(dqp) > dqp->q_flags |= XFS_DQFLAG_FREEING > xfs_dqunlock(dqp) > //add dquot to dispose list > //free dquot in dispose list > xfs_qm_dqfree_one(dqp) > trace_xfs_ail_xxx(lip) //UAF > > Fix this by returning XFS_ITEM_UNSAFE in xfs_qm_dquot_logitem_push() when > dquot flush encounters errors (excluding EAGAIN error), ensuring xfsaild > does not access the log item after it is pushed. > > Fixes: 9e4c109ac822 ("xfs: add AIL pushing tracepoints") > Signed-off-by: Long Li <leo.lilong@huawei.com> > --- > fs/xfs/xfs_dquot_item.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c > index 7d19091215b0..afc7ad91ddef 100644 > --- a/fs/xfs/xfs_dquot_item.c > +++ b/fs/xfs/xfs_dquot_item.c > @@ -160,8 +160,16 @@ xfs_qm_dquot_logitem_push( > if (!xfs_buf_delwri_queue(bp, buffer_list)) > rval = XFS_ITEM_FLUSHING; > xfs_buf_relse(bp); > - } else if (error == -EAGAIN) > + } else if (error == -EAGAIN) { > rval = XFS_ITEM_LOCKED; > + } else { > + /* > + * The dirty flag has been cleared; the dquot may be reclaimed > + * after unlock. It's unsafe to access the item after it has > + * been pushed. > + */ > + rval = XFS_ITEM_UNSAFE; > + } > > spin_lock(&lip->li_ailp->ail_lock); Um, didn't we just establish that lip could have been freed? Why is it safe to continue accessing the AIL through the lip here? --D > out_unlock: > -- > 2.39.2 > >
On Fri, Aug 23, 2024 at 10:20:38AM -0700, Darrick J. Wong wrote: > On Fri, Aug 23, 2024 at 07:04:38PM +0800, Long Li wrote: > > If errors are encountered while pushing a dquot log item, the dquot dirty > > flag is cleared. Without the protection of dqlock and dqflock locks, the > > dquot reclaim thread may free the dquot. Accessing the log item in xfsaild > > after this can trigger a UAF. > > > > CPU0 CPU1 > > push item reclaim dquot > > ----------------------- ----------------------- > > xfsaild_push_item > > xfs_qm_dquot_logitem_push(lip) > > xfs_dqlock_nowait(dqp) > > xfs_dqflock_nowait(dqp) > > spin_unlock(&lip->li_ailp->ail_lock) > > xfs_qm_dqflush(dqp, &bp) > > <encountered some errors> > > xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE) > > dqp->q_flags &= ~XFS_DQFLAG_DIRTY > > <dquot is not diry> > > xfs_trans_ail_delete(lip, 0) > > xfs_dqfunlock(dqp) > > spin_lock(&lip->li_ailp->ail_lock) > > xfs_dqunlock(dqp) > > xfs_qm_shrink_scan > > list_lru_shrink_walk > > xfs_qm_dquot_isolate > > xfs_dqlock_nowait(dqp) > > xfs_dqfunlock(dqp) > > //dquot is clean, not flush it > > xfs_dqfunlock(dqp) > > dqp->q_flags |= XFS_DQFLAG_FREEING > > xfs_dqunlock(dqp) > > //add dquot to dispose list > > //free dquot in dispose list > > xfs_qm_dqfree_one(dqp) > > trace_xfs_ail_xxx(lip) //UAF > > > > Fix this by returning XFS_ITEM_UNSAFE in xfs_qm_dquot_logitem_push() when > > dquot flush encounters errors (excluding EAGAIN error), ensuring xfsaild > > does not access the log item after it is pushed. > > > > Fixes: 9e4c109ac822 ("xfs: add AIL pushing tracepoints") > > Signed-off-by: Long Li <leo.lilong@huawei.com> > > --- > > fs/xfs/xfs_dquot_item.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c > > index 7d19091215b0..afc7ad91ddef 100644 > > --- a/fs/xfs/xfs_dquot_item.c > > +++ b/fs/xfs/xfs_dquot_item.c > > @@ -160,8 +160,16 @@ xfs_qm_dquot_logitem_push( > > if (!xfs_buf_delwri_queue(bp, buffer_list)) > > rval = XFS_ITEM_FLUSHING; > > xfs_buf_relse(bp); > > - } else if (error == -EAGAIN) > > + } else if (error == -EAGAIN) { > > rval = XFS_ITEM_LOCKED; > > + } else { > > + /* > > + * The dirty flag has been cleared; the dquot may be reclaimed > > + * after unlock. It's unsafe to access the item after it has > > + * been pushed. > > + */ > > + rval = XFS_ITEM_UNSAFE; > > + } > > > > spin_lock(&lip->li_ailp->ail_lock); > > Um, didn't we just establish that lip could have been freed? Why is it > safe to continue accessing the AIL through the lip here? > > --D > We are still within the dqlock context here, and the dquot will only be released after the dqlock is released. In contrast, during the inode item push, the inode log item may be released within xfs_iflush_cluster() - this is where the two cases differ. However, for the sake of code consistency, should we also avoid accessing the AIL through lip here? Thanks, Long Li
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c index 7d19091215b0..afc7ad91ddef 100644 --- a/fs/xfs/xfs_dquot_item.c +++ b/fs/xfs/xfs_dquot_item.c @@ -160,8 +160,16 @@ xfs_qm_dquot_logitem_push( if (!xfs_buf_delwri_queue(bp, buffer_list)) rval = XFS_ITEM_FLUSHING; xfs_buf_relse(bp); - } else if (error == -EAGAIN) + } else if (error == -EAGAIN) { rval = XFS_ITEM_LOCKED; + } else { + /* + * The dirty flag has been cleared; the dquot may be reclaimed + * after unlock. It's unsafe to access the item after it has + * been pushed. + */ + rval = XFS_ITEM_UNSAFE; + } spin_lock(&lip->li_ailp->ail_lock); out_unlock:
If errors are encountered while pushing a dquot log item, the dquot dirty flag is cleared. Without the protection of dqlock and dqflock locks, the dquot reclaim thread may free the dquot. Accessing the log item in xfsaild after this can trigger a UAF. CPU0 CPU1 push item reclaim dquot ----------------------- ----------------------- xfsaild_push_item xfs_qm_dquot_logitem_push(lip) xfs_dqlock_nowait(dqp) xfs_dqflock_nowait(dqp) spin_unlock(&lip->li_ailp->ail_lock) xfs_qm_dqflush(dqp, &bp) <encountered some errors> xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE) dqp->q_flags &= ~XFS_DQFLAG_DIRTY <dquot is not diry> xfs_trans_ail_delete(lip, 0) xfs_dqfunlock(dqp) spin_lock(&lip->li_ailp->ail_lock) xfs_dqunlock(dqp) xfs_qm_shrink_scan list_lru_shrink_walk xfs_qm_dquot_isolate xfs_dqlock_nowait(dqp) xfs_dqfunlock(dqp) //dquot is clean, not flush it xfs_dqfunlock(dqp) dqp->q_flags |= XFS_DQFLAG_FREEING xfs_dqunlock(dqp) //add dquot to dispose list //free dquot in dispose list xfs_qm_dqfree_one(dqp) trace_xfs_ail_xxx(lip) //UAF Fix this by returning XFS_ITEM_UNSAFE in xfs_qm_dquot_logitem_push() when dquot flush encounters errors (excluding EAGAIN error), ensuring xfsaild does not access the log item after it is pushed. Fixes: 9e4c109ac822 ("xfs: add AIL pushing tracepoints") Signed-off-by: Long Li <leo.lilong@huawei.com> --- fs/xfs/xfs_dquot_item.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)