xfs: shutdown filesystem if xfs_perag_get fails

This problem happened locally with a bad inode number from xfs recovery. xfs_perag_get() can return NULL if given a bad agno. Most callers of xfs_perag_get() do not check for a NULL before using the pointer. This patch forces a shutdown of the filesystem for those callers that do not check the return value rather than crashing on a dereferenced NULL pointer.

Signed-off-by Mark Tinguely <[email protected]>

---
 fs/xfs/xfs_icache.c |    2 +-
 fs/xfs/xfs_mount.c  |    4 ++--
 fs/xfs/xfs_mount.h  |   17 ++++++++++++++++-
 3 files changed, 19 insertions(+), 4 deletions(-)

Index: b/fs/xfs/xfs_icache.c
===================================================================

fs/xfs/xfs_icache.c +1 -1

@@ -656,7 +656,7 @@ xfs_inode_ag_iterator(
 	xfs_agnumber_t		ag;
 
 	ag = 0;
-	while ((pag = xfs_perag_get(mp, ag))) {
+	while ((pag = __xfs_perag_get(mp, ag))) {
 		ag = pag->pag_agno + 1;
 		error = xfs_inode_ag_walk(mp, pag, execute, flags, args, -1);
 		xfs_perag_put(pag);

Index: b/fs/xfs/xfs_mount.c
===================================================================

fs/xfs/xfs_mount.c +2 -2

@@ -193,7 +193,7 @@ xfs_uuid_unmount(
  * have to protect against changes is the tree structure itself.
  */
 struct xfs_perag *
-xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
+__xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
 {
 	struct xfs_perag	*pag;
 	int			ref = 0;

@@ -442,7 +442,7 @@ xfs_initialize_perag(
 	 * AGs we don't find ready for initialisation.
 	 */
 	for (index = 0; index < agcount; index++) {
-		pag = xfs_perag_get(mp, index);
+		pag = __xfs_perag_get(mp, index);
 		if (pag) {
 			xfs_perag_put(pag);
 			continue;

Index: b/fs/xfs/xfs_mount.h
===================================================================

fs/xfs/xfs_mount.h +16 -1

@@ -336,12 +336,27 @@ xfs_daddr_to_agbno(struct xfs_mount *mp,
 /*
  * perag get/put wrappers for ref counting
  */
-struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno);
+struct xfs_perag *__xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno);
 struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *mp, xfs_agnumber_t agno,
 					int tag);
 void	xfs_perag_put(struct xfs_perag *pag);
 
 /*
+ * Ensure the per AG entry was found. Shutting down the filesystem
+ * is better than crashing the OS.
+ */
+static inline struct xfs_perag *
+xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
+{
+	struct xfs_perag	*pag;
+
+	pag = __xfs_perag_get(mp, agno);
+	if (!pag)
+		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+	return pag;
+}
+
+/*
  * Per-cpu superblock locking functions
  */
 #ifdef HAVE_PERCPU_SB

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 6 lines · Mark Tinguely

This problem happened locally with a bad inode number from xfs recovery. xfs_perag_get() can return NULL if given a bad agno. Most callers of xfs_perag_get() do not check for a NULL before using the pointer. This patch forces a shutdown of the filesystem for those callers that do not check the return value rather than crashing on a dereferenced NULL pointer.

Hi Mark -

I'm curious, what was the callchain when this happened? Was it during recovery? If so, would aborting recovery be more prudent?

I might be missing something, but I'm not sure how shutting down avoids a subsequent null ptr deref& crash.

i.e. if a caller does something like:

         pag = xfs_perag_get(mp, agno);
         spin_lock(&pag->pagb_lock);

shutting down in xfs_perag_get doesn't save us from a null pag pointer, would it?

Thanks,
-Eric

Quoted 25 lines · Eric Sandeen

Quoted 6 lines · Mark Tinguely

This problem happened locally with a bad inode number from xfs recovery. xfs_perag_get() can return NULL if given a bad agno. Most callers of xfs_perag_get() do not check for a NULL before using the pointer. This patch forces a shutdown of the filesystem for those callers that do not check the return value rather than crashing on a dereferenced NULL pointer.

Hi Mark -

I'm curious, what was the callchain when this happened? Was it during recovery? If so, would aborting recovery be more prudent?

I might be missing something, but I'm not sure how shutting down avoids a subsequent null ptr deref& crash.

i.e. if a caller does something like:

         pag = xfs_perag_get(mp, agno);
         spin_lock(&pag->pagb_lock);

shutting down in xfs_perag_get doesn't save us from a null pag pointer, would it?

Quoted 2 lines

Thanks,
-Eric

You are correct, we have to exit the routine(s) to avoid the dereference. Let the callers handle the error.

Sorry for the noise.

Quoted 31 lines · Mark Tinguely

Quoted 19 lines · Eric Sandeen

Quoted 6 lines · Mark Tinguely

This problem happened locally with a bad inode number from xfs recovery. xfs_perag_get() can return NULL if given a bad agno. Most callers of xfs_perag_get() do not check for a NULL before using the pointer. This patch forces a shutdown of the filesystem for those callers that do not check the return value rather than crashing on a dereferenced NULL pointer.

Hi Mark -

I'm curious, what was the callchain when this happened? Was it during recovery? If so, would aborting recovery be more prudent?

I might be missing something, but I'm not sure how shutting
down avoids a subsequent null ptr deref& crash.

i.e. if a caller does something like:

Quoted 8 lines

          pag = xfs_perag_get(mp, agno);
          spin_lock(&pag->pagb_lock);

shutting down in xfs_perag_get doesn't save us from a null pag pointer, would it?

Thanks,
-Eric

You are correct, we have to exit the routine(s) to avoid the dereference. Let the callers handle the error.

Quoted 1 line

Sorry for the noise.

No problem, glad I'm useful on the rare occasion. ;)

Can you share the backtrace on the null deref you saw?

Thanks,
-Eric

#6 [ffff880135603980] _xfs_buf_find at ffffffffa01a7fef [xfs] #7 [ffff8801356039f0] xfs_buf_get at ffffffffa01a824a [xfs] #8 [ffff880135603a30] xfs_buf_read at ffffffffa01a83a4 [xfs] #9 [ffff880135603a60] xlog_recover_inode_pass2 at ffffffffa0193629 [xfs]

The recovery value is bad and is a problem on its own, but XFS does not verify the validity of ag number when doing a xfs_perag_get().

Quoted 4 lines · Mark Tinguely

#6 [ffff880135603980] _xfs_buf_find at ffffffffa01a7fef [xfs] #7 [ffff8801356039f0] xfs_buf_get at ffffffffa01a824a [xfs] #8 [ffff880135603a30] xfs_buf_read at ffffffffa01a83a4 [xfs] #9 [ffff880135603a60] xlog_recover_inode_pass2 at ffffffffa0193629 [xfs]

So it's the same problem as this bug fix addresses:

commit 10616b806d1d7835b1d23b8d75ef638f92cb98b6

Author: Dave Chinner<[email protected]>
Date:   Mon Jan 21 23:53:52 2013 +1100

xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

When _xfs_buf_find is passed an out of range address, it will fail to find a relevant struct xfs_perag and oops with a null dereference. This can happen when trying to walk a filesystem with a metadata inode that has a partially corrupted extent map (i.e. the block number returned is corrupt, but is otherwise intact) and we try to read from the corrupted block address.

In this case, just fail the lookup. If it is readahead being issued, it will simply not be done, but if it is real read that fails we will get an error being reported. Ideally this case should result in an EFSCORRUPTED error being reported, but we cannot return an error through xfs_buf_read() or xfs_buf_get() so this lookup failure may result in ENOMEM or EIO errors being reported instead.

Signed-off-by Dave Chinner<[email protected]>

Reviewed-by Brian Foster<[email protected]>

Reviewed-by Ben Myers<[email protected]>

Signed-off-by Ben Myers<[email protected]>

Quoted 2 lines

The recovery value is bad and is a problem on its own, but XFS does not verify the validity of ag number when doing a xfs_perag_get().

Right, that's what the above fix does, but it can't be done on older kernels because grwofs relies on being able to get buffers beyond the existing filesystem limits...

Cheers,

Dave.

Quoted 40 lines · Dave Chinner

Quoted 4 lines · Mark Tinguely

#6 [ffff880135603980] _xfs_buf_find at ffffffffa01a7fef [xfs] #7 [ffff8801356039f0] xfs_buf_get at ffffffffa01a824a [xfs] #8 [ffff880135603a30] xfs_buf_read at ffffffffa01a83a4 [xfs] #9 [ffff880135603a60] xlog_recover_inode_pass2 at ffffffffa0193629 [xfs]

So it's the same problem as this bug fix addresses:

commit 10616b806d1d7835b1d23b8d75ef638f92cb98b6

Author: Dave Chinner<[email protected]>
Date:   Mon Jan 21 23:53:52 2013 +1100

xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

When _xfs_buf_find is passed an out of range address, it will fail to find a relevant struct xfs_perag and oops with a null dereference. This can happen when trying to walk a filesystem with a metadata inode that has a partially corrupted extent map (i.e. the block number returned is corrupt, but is otherwise intact) and we try to read from the corrupted block address.

In this case, just fail the lookup. If it is readahead being issued, it will simply not be done, but if it is real read that fails we will get an error being reported. Ideally this case should result in an EFSCORRUPTED error being reported, but we cannot return an error through xfs_buf_read() or xfs_buf_get() so this lookup failure may result in ENOMEM or EIO errors being reported instead.

Signed-off-by Dave Chinner<[email protected]>

Reviewed-by Brian Foster<[email protected]>

Reviewed-by Ben Myers<[email protected]>

Signed-off-by Ben Myers<[email protected]>

Quoted 2 lines

The recovery value is bad and is a problem on its own, but XFS does not verify the validity of ag number when doing a xfs_perag_get().

Right, that's what the above fix does, but it can't be done on older kernels because grwofs relies on being able to get buffers beyond the existing filesystem limits...

Quoted 3 lines

Cheers,

Dave.

Thank-you, that make sense.

I still do not like assuming xfs_perag_get() will always return a valid perag pointer.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 29 lines · Mark Tinguely

Quoted 28 lines · Dave Chinner

Quoted 4 lines · Mark Tinguely

#6 [ffff880135603980] _xfs_buf_find at ffffffffa01a7fef [xfs] #7 [ffff8801356039f0] xfs_buf_get at ffffffffa01a824a [xfs] #8 [ffff880135603a30] xfs_buf_read at ffffffffa01a83a4 [xfs] #9 [ffff880135603a60] xlog_recover_inode_pass2 at ffffffffa0193629 [xfs]

So it's the same problem as this bug fix addresses:

commit 10616b806d1d7835b1d23b8d75ef638f92cb98b6

Author: Dave Chinner<[email protected]>
Date:   Mon Jan 21 23:53:52 2013 +1100

xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

When _xfs_buf_find is passed an out of range address, it will fail to find a relevant struct xfs_perag and oops with a null dereference. This can happen when trying to walk a filesystem with a metadata inode that has a partially corrupted extent map (i.e. the block number returned is corrupt, but is otherwise intact) and we try to read from the corrupted block address.

In this case, just fail the lookup. If it is readahead being issued, it will simply not be done, but if it is real read that fails we will get an error being reported. Ideally this case should result in an EFSCORRUPTED error being reported, but we cannot return an error through xfs_buf_read() or xfs_buf_get() so this lookup failure may result in ENOMEM or EIO errors being reported instead.

Quoted 7 lines

Quoted 7 lines

Signed-off-by Dave Chinner<[email protected]>

Reviewed-by Brian Foster<[email protected]>

Reviewed-by Ben Myers<[email protected]>

Signed-off-by Ben Myers<[email protected]>

Quoted 2 lines

The recovery value is bad and is a problem on its own, but XFS does not verify the validity of ag number when doing a xfs_perag_get().

Quoted 6 lines

Quoted 3 lines

Right, that's what the above fix does, but it can't be done on older kernels because grwofs relies on being able to get buffers beyond the existing filesystem limits...

Thank-you, that make sense.

Quoted 2 lines

I still do not like assuming xfs_perag_get() will always return a valid perag pointer.

I second that.

Is there any reason we should _not_ check the return value from xfs_perag_get() for NULL ?

Quoted 30 lines · Chandra Seetharaman

Quoted 29 lines · Mark Tinguely

Quoted 28 lines · Dave Chinner

Quoted 4 lines · Mark Tinguely

#6 [ffff880135603980] _xfs_buf_find at ffffffffa01a7fef [xfs] #7 [ffff8801356039f0] xfs_buf_get at ffffffffa01a824a [xfs] #8 [ffff880135603a30] xfs_buf_read at ffffffffa01a83a4 [xfs] #9 [ffff880135603a60] xlog_recover_inode_pass2 at ffffffffa0193629 [xfs]

So it's the same problem as this bug fix addresses:

commit 10616b806d1d7835b1d23b8d75ef638f92cb98b6

Author: Dave Chinner<[email protected]>
Date:   Mon Jan 21 23:53:52 2013 +1100

xfs: fix _xfs_buf_find oops on blocks beyond the filesystem end

When _xfs_buf_find is passed an out of range address, it will fail to find a relevant struct xfs_perag and oops with a null dereference. This can happen when trying to walk a filesystem with a metadata inode that has a partially corrupted extent map (i.e. the block number returned is corrupt, but is otherwise intact) and we try to read from the corrupted block address.

In this case, just fail the lookup. If it is readahead being issued, it will simply not be done, but if it is real read that fails we will get an error being reported. Ideally this case should result in an EFSCORRUPTED error being reported, but we cannot return an error through xfs_buf_read() or xfs_buf_get() so this lookup failure may result in ENOMEM or EIO errors being reported instead.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Signed-off-by Dave Chinner<[email protected]>

Reviewed-by Brian Foster<[email protected]>

Reviewed-by Ben Myers<[email protected]>

Signed-off-by Ben Myers<[email protected]>

Quoted 2 lines

The recovery value is bad and is a problem on its own, but XFS does not verify the validity of ag number when doing a xfs_perag_get().

I'd be interested to know why the inode in recovery is bad - is this on a kernel that CRCs the log records? Or a result of some other bug or hardware corruption? i.e. xfs_perag_get is not the problem here, it's a corruption of a trusted inode number and we failed to detect that corruption....

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Right, that's what the above fix does, but it can't be done on older kernels because grwofs relies on being able to get buffers beyond the existing filesystem limits...

Thank-you, that make sense.

Quoted 7 lines

Quoted 2 lines

I still do not like assuming xfs_perag_get() will always return a valid perag pointer.

I second that.

Is there any reason we should _not_ check the return value from xfs_perag_get() for NULL ?

Yes. The input AG should already be bounds checked before the perag is looked up. If we are asking for an invalid AG, then the bug is not in xfs_perag_get(), it is in the code that is calling it. i.e. error checking the xfs_perag_get() function is a band-aid for improper object validation, not a solution to the problem.

That is, this function was designed to be extremely low overhead and only to be handed validated data. If it is only handed validated data, then it is guaranteed to return a valid per-ag structure, and therefore error checking the return value is not necessary.

Because xfs_perag_get is not designed to handle untrusted data it is up to the calling code to first validate the AGNO that is passed to xfs_perag_get(). If we aren't first validating the object that the AGNO is derived from, then the calling code has failed to validate it's inputs sufficiently, and lots of other things can go wrong (not just the xfs_perag_get() call).

For example, the above commit is a catchall for bad block numbers being looked up in extent records. It was a quick fix, not a targeted fix for the reported problems. For bad block numbers in extents, we should be doing is validating block numbers when they are looked up are within the filesystem bounds (eg. inside xfs_bmapi_read/xfs_bmapi_write) so that a bad block number is caught at lookup time, not at IO time. We only do that for extents that point to block 0.

Cheers,

Dave.

In which case something along the lines of

---

fs/xfs/xfs_mount.c +0 -0

index 3806088..3fb2fa6 100644
@@ -203,7 +203,13 @@ xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t
 agno)
        if (pag) {

        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).

In which case something along the lines of

---

fs/xfs/xfs_mount.c +7 -1

index 3806088..3fb2fa6 100644
@@ -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).

Quoted 23 lines · Chandra Seetharaman

In which case something along the lines of

---

fs/xfs/xfs_mount.c +7 -1

index 3806088..3fb2fa6 100644
@@ -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;

Quoted 7 lines

--------

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.

Hi Mark and Chandra,

Quoted 36 lines · Mark Tinguely

Quoted 23 lines · Chandra Seetharaman

In which case something along the lines of

---

fs/xfs/xfs_mount.c +7 -1

index 3806088..3fb2fa6 100644
@@ -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;

Quoted 7 lines

--------

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.

Quoted 4 lines

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.