xfs
[Top] [All Lists]

Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails
From: Ben Myers <bpm@xxxxxxx>
Date: Fri, 26 Apr 2013 11:07:04 -0500
Cc: sekharan@xxxxxxxxxx, Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <517A9E12.1030907@xxxxxxx>
References: <5174603A.8030208@xxxxxxxxxxx> <51753EDE.6000301@xxxxxxx> <51754A13.5000808@xxxxxxxxxxx> <5175532B.3050509@xxxxxxx> <20130422233033.GK30622@dastard> <51769111.3050103@xxxxxxx> <1366732475.3762.32402.camel@xxxxxxxxxxxxxxxxxx> <20130423204956.GM10481@dastard> <1366929706.4098.6.camel@xxxxxxxxxxxxxxxxxx> <517A9E12.1030907@xxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
Hi Mark and Chandra,

On Fri, Apr 26, 2013 at 10:32:34AM -0500, Mark Tinguely wrote:
> On 04/25/13 17:41, Chandra Seetharaman wrote:
> >In which case something along the lines of
> >
> >---
> >diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> >index 3806088..3fb2fa6 100644
> >--- a/fs/xfs/xfs_mount.c
> >+++ b/fs/xfs/xfs_mount.c
> >@@ -203,7 +203,13 @@ xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t
> >agno)
> >         if (pag) {
> >                 ASSERT(atomic_read(&pag->pag_ref)>= 0);
> >                 ref = atomic_inc_return(&pag->pag_ref);
> >-       }
> >+       } else
> >+               /*
> >+                * xfs_perag_get() is called with invalid agno,
> >+                * which cannot happen. This indicates a problem
> >+                * in the calling code.
> >+                */
> >+               BUG();
> >         rcu_read_unlock();
> >         trace_xfs_perag_get(mp, agno, ref, _RET_IP_);
> >         return pag;
> >--------
> >
> >would be useful ?. Since we have a NULL pag, we will trip somewhere
> >else. At least with this, there is a pointer to the debugger/sysadmin
> >about where/what to look for (may be with more valuable/correct comment
> >than above).
> >
> 
> We will have to make sure the callers of xfs_perag_get() handle the NULL
> before dereferencing it. Sometimes the NULL is normal and just means the
> perag structure has not been initialize yet.
> 
> Properly handling the NULL from xfs_perag_get() in the caller will also
> mean that the callers of the callers of xfs_perag_get() have to handle
> the NULL returned to them. I will come back to this once the CRC stuff
> has been put to rest.

I agree that we want to address this.  Our worst case should be a forced
shutdown, rather than a NULL ptr deref, or a BUG().  Ideally one corrupted
filesystem does not result in a full system outage, right?  ;)

There are some others like this.  e.g.  xfs_da_read_buf can return 0 with a
null buffer pointer, and we rarely check for that before using bp.

-Ben

<Prev in Thread] Current Thread [Next in Thread>