Review: Fix dbflush panic in xfs_qm_sync

Review: Fix dbflush panic in xfs_qm_sync

[Donald Douwsma]

The recent behaviour layer removal dropped the check for quotas that have been
requested at mount time but have subsequently been turned off. This results
in a panic when accessing m_quotainfo which has been freed.

This patch adds the check originally made by xfs_qm_syncall() to xfs_qm_sync()


Signed-off-by: Donald Douwsma <donaldd@sgi.com>

--- 2.6.x-xfs.orig/fs/xfs/quota/xfs_qm.c
+++ 2.6.x-xfs/fs/xfs/quota/xfs_qm.c
@@ -1007,6 +1007,9 @@ xfs_qm_sync(
  	boolean_t	nowait;
  	int		error;

+	if (! XFS_IS_QUOTA_ON(mp))
+		return 0;
+
  	restarts = 0;
  	/*
  	 * We won't block unless we are asked to.

Re: Review: Fix dbflush panic in xfs_qm_sync

[Lachlan McIlroy]

Could mp->m_quotainfo become NULL after this check but before we
lock the list with xfs_qm_mplist_lock()?  There doesn't seem to
be any locking to protect changes to this field?

Donald Douwsma wrote:
> The recent behaviour layer removal dropped the check for quotas that 
> have been
> requested at mount time but have subsequently been turned off. This results
> in a panic when accessing m_quotainfo which has been freed.
> 
> This patch adds the check originally made by xfs_qm_syncall() to 
> xfs_qm_sync()
> 
> 
> Signed-off-by: Donald Douwsma <donaldd@sgi.com>
> 
> --- 2.6.x-xfs.orig/fs/xfs/quota/xfs_qm.c
> +++ 2.6.x-xfs/fs/xfs/quota/xfs_qm.c
> @@ -1007,6 +1007,9 @@ xfs_qm_sync(
>      boolean_t    nowait;
>      int        error;
> 
> +    if (! XFS_IS_QUOTA_ON(mp))
> +        return 0;
> +
>      restarts = 0;
>      /*
>       * We won't block unless we are asked to.
> 
> 
> 

Re: Review: Fix dbflush panic in xfs_qm_sync

[David Chinner]

On Tue, Oct 16, 2007 at 03:18:02PM +1000, Lachlan McIlroy wrote:
> Could mp->m_quotainfo become NULL after this check but before we
> lock the list with xfs_qm_mplist_lock()?  There doesn't seem to
> be any locking to protect changes to this field?

Possible - in theory. Likely - no. We do the same unlocked check in a
few places...

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: Fix dbflush panic in xfs_qm_sync

[Donald Douwsma]

David Chinner wrote:
> On Tue, Oct 16, 2007 at 03:18:02PM +1000, Lachlan McIlroy wrote:
>> Could mp->m_quotainfo become NULL after this check but before we
>> lock the list with xfs_qm_mplist_lock()?  There doesn't seem to
>> be any locking to protect changes to this field?
> 
> Possible - in theory. Likely - no. We do the same unlocked check in a
> few places...
> 

The mplist lock doesnt prevent the quotainfo going away while its held.
It's just guarding a hashlist that lives in quotainfo structure.
So none of this code prevents a quotaoff race.

In the short term this change restores the original checks. But in the
longer term we should review the locking in the quota system, possibly
adding a quotaoff lock to xfs_mount_t.

Don

Re: Review: Fix dbflush panic in xfs_qm_sync

[Christoph Hellwig]

On Tue, Oct 16, 2007 at 09:29:23AM +1000, Donald Douwsma wrote:
> The recent behaviour layer removal dropped the check for quotas that have 
> been
> requested at mount time but have subsequently been turned off. This results
> in a panic when accessing m_quotainfo which has been freed.
> 
> This patch adds the check originally made by xfs_qm_syncall() to 
> xfs_qm_sync()

Hmm.  We do the same check just a few lines below, but inbetween
we access ->m_quotainfo to take the lock.  I was under the impression
that's the only safe way to check anyway, but ->m_quotainfo might not
actually be present.  It might me better to move the lock into xfs_mount
directly, but it's embedded into a xfs_dqhash and there's some complex
mess with gazillions of macros around it.

So I suspect restoring it to the original state might be good enough.