| Message ID | 20230228051250.1238353-1-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: quotacheck failure can race with background inode inactivation | expand |
On Tue, Feb 28, 2023 at 04:12:50PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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@intel.com> > Signed-off-by: Dave Chinner <dchinner@redhat.com> Makes sense, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --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 >
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; + } /*