[PATCH] xfs: shutdown filesystem if xfs_perag_get fails

[PATCH] xfs: shutdown filesystem if xfs_perag_get fails

[Mark Tinguely]

[-- Attachment #1: xfs_force_shutdown_if_pag_is_NULL.patch --]
[-- Type: text/plain, Size: 2817 bytes --]

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 <tinguely@sgi.com>
---
 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
===================================================================
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -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
===================================================================
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -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
===================================================================
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

On 4/21/13 12:41 PM, Mark Tinguely wrote:

> 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

> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  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
> ===================================================================
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -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
> ===================================================================
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -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
> ===================================================================
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

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

[Mark Tinguely]

On 04/21/13 16:55, Eric Sandeen wrote:
> On 4/21/13 12:41 PM, Mark Tinguely wrote:
>
>> 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

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

Sorry for the noise.

--Mark.

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

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

[Eric Sandeen]

On 4/22/13 8:45 AM, Mark Tinguely wrote:
> On 04/21/13 16:55, Eric Sandeen wrote:
>> On 4/21/13 12:41 PM, Mark Tinguely wrote:
>>
>>> 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
> 
> You are correct, we have to exit the routine(s) to avoid the dereference. Let the callers handle the error.
> 
> 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

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

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

[Mark Tinguely]

On 04/22/13 09:32, Eric Sandeen wrote:
> On 4/22/13 8:45 AM, Mark Tinguely wrote:
>> On 04/21/13 16:55, Eric Sandeen wrote:
>>> On 4/21/13 12:41 PM, Mark Tinguely wrote:
>>>
>>>> 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
>>
>> You are correct, we have to exit the routine(s) to avoid the dereference. Let the callers handle the error.
>>
>> 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
>
PID: 7965   TASK: ffff88013566c040  CPU: 0   COMMAND: "mount"
  #0 [ffff8801356035d0] machine_kexec at ffffffff810265ce
  #1 [ffff880135603620] crash_kexec at ffffffff810a3b5a
  #2 [ffff8801356036f0] oops_end at ffffffff81442de8
  #3 [ffff880135603710] __bad_area_nosemaphore at ffffffff81032995
  #4 [ffff8801356037d0] do_page_fault at ffffffff8144558b
  #5 [ffff8801356038d0] page_fault at ffffffff81442065
     [exception RIP: _raw_spin_lock+5]
     RIP: ffffffff81441985  RSP: ffff880135603980  RFLAGS: 00010206
     RAX: 0000000000010000  RBX: ffff880135530340  RCX: 0000000000000000
     RDX: 0000000000000001  RSI: 0000000000000013  RDI: 0000000000000090
     RBP: 0000000000000000   R8: 0000000000000000   R9: 0000000000000003
     R10: ffff880134d96d00  R11: ffffffff8101ff60  R12: 0000000000000000
     R13: 00000042bb4c2000  R14: 0000000000002000  R15: ffff880135530340
     ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
  #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]
#10 [ffff880135603ac0] xlog_recover_commit_trans at ffffffffa0194166 [xfs]
#11 [ffff880135603af0] xlog_recover_process_data at ffffffffa0194351 [xfs]
#12 [ffff880135603b50] xlog_do_recovery_pass at ffffffffa0194804 [xfs]
#13 [ffff880135603c80] xlog_do_log_recovery at ffffffffa0194aa7 [xfs]
#14 [ffff880135603cb0] xlog_do_recover at ffffffffa0194aea [xfs]
#15 [ffff880135603cd0] xlog_recover at ffffffffa019592e [xfs]
#16 [ffff880135603cf0] xfs_log_mount at ffffffffa018d524 [xfs]
#17 [ffff880135603d20] xfs_mountfs at ffffffffa0198a13 [xfs]
#18 [ffff880135603d70] xfs_fs_fill_super at ffffffffa01b0ac9 [xfs]
#19 [ffff880135603db0] mount_bdev at ffffffff811544fe
#20 [ffff880135603e20] mount_fs at ffffffff81153ece
#21 [ffff880135603e60] vfs_kern_mount at ffffffff8116f975
#22 [ffff880135603e90] do_kern_mount at ffffffff8116fa63
#23 [ffff880135603ed0] do_mount at ffffffff81170a7d
#24 [ffff880135603f30] sys_mount at ffffffff81170b80
#25 [ffff880135603f80] system_call_fastpath at ffffffff81449692
     RIP: 00007fd46cac51ea  RSP: 00007fff910bdde8  RFLAGS: 00010206
     RAX: 00000000000000a5  RBX: ffffffff81449692  RCX: ffffffffc0ed0000
     RDX: 0000000000626fb0  RSI: 000000000061de90  RDI: 000000000061de70
     RBP: 00007fff910be0cc   R8: 0000000000000000   R9: 00007fff910bdad0
     R10: ffffffffc0ed0000  R11: 0000000000000206  R12: 0000000000626fb0
     R13: 0000000000000000  R14: 00000000c0ed0000  R15: 00007fff910be040
     ORIG_RAX: 00000000000000a5  CS: 0033  SS: 002b


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

--Mark.

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

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

[Dave Chinner]

On Mon, Apr 22, 2013 at 10:11:39AM -0500, Mark Tinguely wrote:
>  #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 <dchinner@redhat.com>
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 <dchinner@redhat.com>
    Reviewed-by: Brian Foster <bfoster@redhat.com>
    Reviewed-by: Ben Myers <bpm@sgi.com>
    Signed-off-by: Ben Myers <bpm@sgi.com>

> 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.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Mark Tinguely]

On 04/22/13 18:30, Dave Chinner wrote:
> On Mon, Apr 22, 2013 at 10:11:39AM -0500, Mark Tinguely wrote:
>>   #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<dchinner@redhat.com>
> 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<dchinner@redhat.com>
>      Reviewed-by: Brian Foster<bfoster@redhat.com>
>      Reviewed-by: Ben Myers<bpm@sgi.com>
>      Signed-off-by: Ben Myers<bpm@sgi.com>
>
>> 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.

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Chandra Seetharaman]

On Tue, 2013-04-23 at 08:48 -0500, Mark Tinguely wrote:
> On 04/22/13 18:30, Dave Chinner wrote:
> > On Mon, Apr 22, 2013 at 10:11:39AM -0500, Mark Tinguely wrote:
> >>   #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<dchinner@redhat.com>
> > 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<dchinner@redhat.com>
> >      Reviewed-by: Brian Foster<bfoster@redhat.com>
> >      Reviewed-by: Ben Myers<bpm@sgi.com>
> >      Signed-off-by: Ben Myers<bpm@sgi.com>
> >
> >> 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.
> 
> Thank-you, that make sense.
> 
> 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 ?
> 
> --Mark.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 


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

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

[Dave Chinner]

On Tue, Apr 23, 2013 at 10:54:35AM -0500, Chandra Seetharaman wrote:
> On Tue, 2013-04-23 at 08:48 -0500, Mark Tinguely wrote:
> > On 04/22/13 18:30, Dave Chinner wrote:
> > > On Mon, Apr 22, 2013 at 10:11:39AM -0500, Mark Tinguely wrote:
> > >>   #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<dchinner@redhat.com>
> > > 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<dchinner@redhat.com>
> > >      Reviewed-by: Brian Foster<bfoster@redhat.com>
> > >      Reviewed-by: Ben Myers<bpm@sgi.com>
> > >      Signed-off-by: Ben Myers<bpm@sgi.com>
> > >
> > >> 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....

> > > 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.
> > 
> > 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.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Chandra Seetharaman]

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

On Wed, 2013-04-24 at 06:49 +1000, Dave Chinner wrote:
> On Tue, Apr 23, 2013 at 10:54:35AM -0500, Chandra Seetharaman wrote:
> > On Tue, 2013-04-23 at 08:48 -0500, Mark Tinguely wrote:
> > > On 04/22/13 18:30, Dave Chinner wrote:
> > > > On Mon, Apr 22, 2013 at 10:11:39AM -0500, Mark Tinguely wrote:
> > > >>   #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<dchinner@redhat.com>
> > > > 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<dchinner@redhat.com>
> > > >      Reviewed-by: Brian Foster<bfoster@redhat.com>
> > > >      Reviewed-by: Ben Myers<bpm@sgi.com>
> > > >      Signed-off-by: Ben Myers<bpm@sgi.com>
> > > >
> > > >> 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....
> 
> > > > 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.
> > > 
> > > 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.


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

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

[Dave Chinner]

On Thu, Apr 25, 2013 at 05:41:46PM -0500, 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();

That's btrfs-style error handling. ;)

In reality, your patch is much worse than just letting the caller
dereference a null pointer, because it happens inside an RCU read
lock. That's guaranteed to hang the system, not just have the thread
that triggers a null pointer dereference go away. And further....

>         rcu_read_unlock();
>         trace_xfs_perag_get(mp, agno, ref, _RET_IP_);

... it means we can't catch the tracepoint with the bad agno in it.

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

The current failure case is pretty damn obvious, so adding a BUG
doesn't make it any better. In fact, it makes it worse because it
prevents a caller from being able to handle a NULL perag return....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Mark Tinguely]

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.

--Mark.

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

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

[Ben Myers]

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

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

[Dave Chinner]

On Fri, Apr 26, 2013 at 11:07:04AM -0500, Ben Myers wrote:
> 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?  ;)

A BUG() or null pointer deref simply segv's the current process. It
doesn't cause a reboot, hang or crash dump unless the system is
configured to do so.

But, as I've already stated, checking the return of xfs_perag_get()
is not answer to this problem - it's just a band-aid. Lots of them,
and mostly unneccessary, too. We need to address the input
validation problem prior to calling xfs_perag_get() to catch the
error at the source, not somewhere in the downstream call chain when
an invalid agno is tripped over.

Indeed, the design of the code is such that the agno is *trusted* to
be correct, just like we trust inode numbers coming from on-disk
structures to be correct. We validate inode numbers properly, but we
aren't validating block numbers returned from extent records
completely. That's the source of the problem we are seeing -
xfs_perag_get() returning NULL is just a symptom.  Put simply:
no-one should *ever* pass an invalid agno to xfs_perag_get().

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

I've also pointed out that the case where that can occur is handled
by the callers that trigger it. It does not need to be checked by
every caller, because most of them can't trigger this return case.
Let's not make a mountain out of a molehill....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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