On Tue, Feb 28, 2023 at 04:12:50PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> The background inode inactivation can attached dquots to inodes, but
> this can race with a foreground quotacheck failure that leads to
> disabling quotas and freeing the mp->m_quotainfo structure. The
> background inode inactivation then tries to allocate a quota, tries
> to dereference mp->m_quotainfo, and crashes like so:
>
> XFS (loop1): Quotacheck: Unsuccessful (Error -5): Disabling quotas.
> xfs filesystem being mounted at /root/syzkaller.qCVHXV/0/file0 supports timestamps until 2038 (0x7fffffff)
> BUG: kernel NULL pointer dereference, address: 00000000000002a8
> ....
> CPU: 0 PID: 161 Comm: kworker/0:4 Not tainted 6.2.0-c9c3395d5e3d #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> Workqueue: xfs-inodegc/loop1 xfs_inodegc_worker
> RIP: 0010:xfs_dquot_alloc+0x95/0x1e0
> ....
> Call Trace:
> <TASK>
> xfs_qm_dqread+0x46/0x440
> xfs_qm_dqget_inode+0x154/0x500
> xfs_qm_dqattach_one+0x142/0x3c0
> xfs_qm_dqattach_locked+0x14a/0x170
> xfs_qm_dqattach+0x52/0x80
> xfs_inactive+0x186/0x340
> xfs_inodegc_worker+0xd3/0x430
> process_one_work+0x3b1/0x960
> worker_thread+0x52/0x660
> kthread+0x161/0x1a0
> ret_from_fork+0x29/0x50
> </TASK>
> ....
>
> Prevent this race by flushing all the queued background inode
> inactivations pending before purging all the cached dquots when
> quotacheck fails.
>
> Reported-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Makes sense,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_qm.c | 40 ++++++++++++++++++++++++++--------------
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index e2c542f6dcd4..78ca52e55f03 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1321,15 +1321,14 @@ xfs_qm_quotacheck(
>
> error = xfs_iwalk_threaded(mp, 0, 0, xfs_qm_dqusage_adjust, 0, true,
> NULL);
> - if (error) {
> - /*
> - * The inode walk may have partially populated the dquot
> - * caches. We must purge them before disabling quota and
> - * tearing down the quotainfo, or else the dquots will leak.
> - */
> - xfs_qm_dqpurge_all(mp);
> - goto error_return;
> - }
> +
> + /*
> + * On error, the inode walk may have partially populated the dquot
> + * caches. We must purge them before disabling quota and tearing down
> + * the quotainfo, or else the dquots will leak.
> + */
> + if (error)
> + goto error_purge;
>
> /*
> * We've made all the changes that we need to make incore. Flush them
> @@ -1363,10 +1362,8 @@ xfs_qm_quotacheck(
> * and turn quotaoff. The dquots won't be attached to any of the inodes
> * at this point (because we intentionally didn't in dqget_noattach).
> */
> - if (error) {
> - xfs_qm_dqpurge_all(mp);
> - goto error_return;
> - }
> + if (error)
> + goto error_purge;
>
> /*
> * If one type of quotas is off, then it will lose its
> @@ -1376,7 +1373,7 @@ xfs_qm_quotacheck(
> mp->m_qflags &= ~XFS_ALL_QUOTA_CHKD;
> mp->m_qflags |= flags;
>
> - error_return:
> +error_return:
> xfs_buf_delwri_cancel(&buffer_list);
>
> if (error) {
> @@ -1395,6 +1392,21 @@ xfs_qm_quotacheck(
> } else
> xfs_notice(mp, "Quotacheck: Done.");
> return error;
> +
> +error_purge:
> + /*
> + * On error, we may have inodes queued for inactivation. This may try
> + * to attach dquots to the inode before running cleanup operations on
> + * the inode and this can race with the xfs_qm_destroy_quotainfo() call
> + * below that frees mp->m_quotainfo. To avoid this race, flush all the
> + * pending inodegc operations before we purge the dquots from memory,
> + * ensuring that background inactivation is idle whilst we turn off
> + * quotas.
> + */
> + xfs_inodegc_flush(mp);
> + xfs_qm_dqpurge_all(mp);
> + goto error_return;
> +
> }
>
> /*
> --
> 2.39.2
>
