[PATCH] xfs: skip dquot attach when m_quotainfo is not set up

[PATCH] xfs: skip dquot attach when m_quotainfo is not set up

[Mikhail Lobanov]

XFS quota accounting flags (XFS_*QUOTA_ACCT in mp->m_qflags) are parsed
from the mount options before xfs_mountfs() runs, but the quota
subsystem itself (mp->m_quotainfo) is only allocated later, in
xfs_qm_mount_quotas().  Background inode inactivation (inodegc) is
enabled earlier still, in xfs_mountfs() right after log recovery.

As a result there is a window during mount where XFS_IS_QUOTA_ON() is
already true while mp->m_quotainfo is still NULL.  The same inconsistency
exists during teardown, when m_quotainfo is freed before the quota flags
are cleared.  If a background xfs_inodegc_worker inactivates an inode in
that window -- easily triggered by mounting a crafted/corrupt image where
the mount aborts after inodegc has been enabled (e.g. xfs_rtmount_inodes()
failing with "failed to read RT inodes") -- xfs_inactive() calls
xfs_qm_dqattach(), which trusts XFS_IS_QUOTA_ON() and goes on to
dereference the NULL mp->m_quotainfo in xfs_qm_dqget_inode():

  XFS (loop0): failed to read RT inodes
  Oops: general protection fault, probably for non-canonical address
        0xdffffc000000002a: 0000 [#1] PREEMPT SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000150-0x0000000000000157]
  Workqueue: xfs-inodegc/loop0 xfs_inodegc_worker
  RIP: 0010:__mutex_lock+0xfe/0x930
  Call Trace:
   xfs_qm_dqget_cache_lookup+0x63/0x7f0
   xfs_qm_dqget_inode+0x336/0x860
   xfs_qm_dqattach_one+0x232/0x4e0
   xfs_qm_dqattach_locked+0x2c6/0x470
   xfs_qm_dqattach+0x46/0x70
   xfs_inactive+0x988/0xe80
   xfs_inodegc_worker+0x27c/0x730

mutex_lock() faults on &qi->qi_tree_lock (offset 0x150) with qi == NULL.

When mp->m_quotainfo is NULL there are no in-core dquots to attach, so
the attach must simply be skipped.  Guard xfs_qm_need_dqattach() -- the
single gate every xfs_qm_dqattach()/xfs_qm_dqattach_locked() caller
passes through before reaching xfs_qm_dqget_inode() -- with an explicit
mp->m_quotainfo check.  Once quotas are fully set up m_quotainfo stays
non-NULL until quotas are turned off (which also clears the quota flags),
so the new check is a no-op for a normally mounted quota filesystem;
quota accounting and enforcement are unaffected.

The existing xfs_inodegc_flush() added by commit 0c7273e494dd ("xfs:
quotacheck failure can race with background inode inactivation") only
covers tearing down quotas after a failed quotacheck and does not help
here, because in this case quotacheck is never reached.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller. 

Fixes: ab23a7768739 ("xfs: per-cpu deferred inode inactivation queues")
Signed-off-by: Mikhail Lobanov <m.lobanov@rosa.ru>
---
 fs/xfs/xfs_qm.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index ee2530fabeeb..6a4e8d6fb806 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -310,6 +310,19 @@ xfs_qm_need_dqattach(

 	if (!XFS_IS_QUOTA_ON(mp))
 		return false;
+	/*
+	 * The quota accounting flags in m_qflags are set from the mount options
+	 * before xfs_mountfs() runs, but the quota subsystem (mp->m_quotainfo)
+	 * is only initialised later in xfs_qm_mount_quotas(); it is also torn
+	 * down (and m_quotainfo set to NULL) before those flags are cleared when
+	 * quotas are turned off.  During those windows a background inodegc
+	 * worker inactivating an inode can reach here with XFS_IS_QUOTA_ON()
+	 * true but no quotainfo to look dquots up in, which would lead to a NULL
+	 * pointer dereference of mp->m_quotainfo in xfs_qm_dqget_inode().  There
+	 * are no dquots to attach in that state, so skip the attach.
+	 */
+	if (!mp->m_quotainfo)
+		return false;
 	if (!XFS_NOT_DQATTACHED(mp, ip))
 		return false;
 	if (xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
--
2.43.0

Re: [PATCH] xfs: skip dquot attach when m_quotainfo is not set up

[Christoph Hellwig]

Hi Mikhail,

this also seems to be somewhat related to Mikhail's fixes in a similar
area.  I just replied there, and my gut feeling here is that if we
are in a mount failure path, we're probably better not doing any
inode inactivation, because well, we failed to mount the file system,
so we better don't do anything to the persistent data structures.