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

[Ben Myers <bpm@sgi.com>]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs