[patch] xfsprogs: unhandled error check in libxfs_trans_read_buf

[patch] xfsprogs: unhandled error check in libxfs_trans_read_buf

[Ajeet Yadav]

xfsprogs: unhandled error check in libxfs_trans_read_buf

libxfs_trans_read_buf() is used in both mkfs.xfs & xfs_repair.
During stability testing we found some time occur pagefault in mkfs.xfs,
code inspection shows that if libxfs_readbuf() fails then occurs a
page fault in xfs_buf_item_init() called in libxfs_trans_read_buf().

mkfs.xfs: unhandled page fault (11) at 0x00000070, code 0x017

Added NULL check and errno handling.

Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>

diff -Nurp xfsprogs/libxfs/rdwr.c xfsprogs-dirty/libxfs/rdwr.c
--- xfsprogs/libxfs/rdwr.c      2011-01-28 20:22:11.000000000 +0900
+++ xfsprogs-dirty/libxfs/rdwr.c        2011-02-02 16:59:16.000000000 +0900
@@ -207,10 +207,11 @@ libxfs_trace_readbuf(const char *func, c
 {
        xfs_buf_t       *bp = libxfs_readbuf(dev, blkno, len, flags);

-       bp->b_func = func;
-       bp->b_file = file;
-       bp->b_line = line;
-
+       if (bp){
+               bp->b_func = func;
+               bp->b_file = file;
+               bp->b_line = line;
+       }
        return bp;
 }

@@ -485,6 +486,7 @@ libxfs_readbuf(dev_t dev, xfs_daddr_t bl
                error = libxfs_readbufr(dev, blkno, bp, len, flags);
                if (error) {
                        libxfs_putbuf(bp);
+                       errno = error;
                        return NULL;
                }
        }
diff -Nurp xfsprogs/libxfs/trans.c xfsprogs-dirty/libxfs/trans.c
--- xfsprogs/libxfs/trans.c     2011-01-28 20:22:11.000000000 +0900
+++ xfsprogs-dirty/libxfs/trans.c       2011-02-02 17:00:42.000000000 +0900
@@ -508,6 +508,10 @@ libxfs_trans_read_buf(
        }

        bp = libxfs_readbuf(dev, blkno, len, flags);
+       if (!bp){
+               *bpp = NULL;
+               return errno;
+       }
 #ifdef XACT_DEBUG
        fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
 #endif

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

Re: [patch] xfsprogs: unhandled error check in libxfs_trans_read_buf

[Ajeet Yadav]

Dear Community,
Our testing team reported one issue, we found it was related to libxfs
and resolved it.
I thought to share my experience with you, but did not receive any comment.

On Thu, Feb 3, 2011 at 3:17 PM, Ajeet Yadav <ajeet.yadav.77@gmail.com> wrote:
> xfsprogs: unhandled error check in libxfs_trans_read_buf
>
> libxfs_trans_read_buf() is used in both mkfs.xfs & xfs_repair.
> During stability testing we found some time occur pagefault in mkfs.xfs,
> code inspection shows that if libxfs_readbuf() fails then occurs a
> page fault in xfs_buf_item_init() called in libxfs_trans_read_buf().
>
> mkfs.xfs: unhandled page fault (11) at 0x00000070, code 0x017
>
> Added NULL check and errno handling.
>
> Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
>
> diff -Nurp xfsprogs/libxfs/rdwr.c xfsprogs-dirty/libxfs/rdwr.c
> --- xfsprogs/libxfs/rdwr.c      2011-01-28 20:22:11.000000000 +0900
> +++ xfsprogs-dirty/libxfs/rdwr.c        2011-02-02 16:59:16.000000000 +0900
> @@ -207,10 +207,11 @@ libxfs_trace_readbuf(const char *func, c
>  {
>        xfs_buf_t       *bp = libxfs_readbuf(dev, blkno, len, flags);
>
> -       bp->b_func = func;
> -       bp->b_file = file;
> -       bp->b_line = line;
> -
> +       if (bp){
> +               bp->b_func = func;
> +               bp->b_file = file;
> +               bp->b_line = line;
> +       }
>        return bp;
>  }
>
> @@ -485,6 +486,7 @@ libxfs_readbuf(dev_t dev, xfs_daddr_t bl
>                error = libxfs_readbufr(dev, blkno, bp, len, flags);
>                if (error) {
>                        libxfs_putbuf(bp);
> +                       errno = error;
>                        return NULL;
>                }
>        }
> diff -Nurp xfsprogs/libxfs/trans.c xfsprogs-dirty/libxfs/trans.c
> --- xfsprogs/libxfs/trans.c     2011-01-28 20:22:11.000000000 +0900
> +++ xfsprogs-dirty/libxfs/trans.c       2011-02-02 17:00:42.000000000 +0900
> @@ -508,6 +508,10 @@ libxfs_trans_read_buf(
>        }
>
>        bp = libxfs_readbuf(dev, blkno, len, flags);
> +       if (!bp){
> +               *bpp = NULL;
> +               return errno;
> +       }
>  #ifdef XACT_DEBUG
>        fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
>  #endif
>

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

Re: [patch] xfsprogs: unhandled error check in libxfs_trans_read_buf

[Alex Elder]

On Thu, 2011-02-03 at 15:17 +0900, Ajeet Yadav wrote:
> xfsprogs: unhandled error check in libxfs_trans_read_buf

Sorry you didn't get a response earlier.  Reporting
problems like this--especially if they come with a
proposed fix--is always appreciated.

> libxfs_trans_read_buf() is used in both mkfs.xfs & xfs_repair.
> During stability testing we found some time occur pagefault in mkfs.xfs,
> code inspection shows that if libxfs_readbuf() fails then occurs a
> page fault in xfs_buf_item_init() called in libxfs_trans_read_buf().
> 
> mkfs.xfs: unhandled page fault (11) at 0x00000070, code 0x017
> 
> Added NULL check and errno handling.

I expect there are other similar problems lurking in
the libxfs code.

I think your change looks good, with one exception,
noted below.  I will gladly adjust your patch for
you if you consent to the change I suggest.

					-Alex

> Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
> 
> diff -Nurp xfsprogs/libxfs/rdwr.c xfsprogs-dirty/libxfs/rdwr.c
> --- xfsprogs/libxfs/rdwr.c      2011-01-28 20:22:11.000000000 +0900
> +++ xfsprogs-dirty/libxfs/rdwr.c        2011-02-02 16:59:16.000000000 +0900
> @@ -207,10 +207,11 @@ libxfs_trace_readbuf(const char *func, c
>  {
>         xfs_buf_t       *bp = libxfs_readbuf(dev, blkno, len, flags);
> 
> -       bp->b_func = func;
> -       bp->b_file = file;
> -       bp->b_line = line;
> -
> +       if (bp){
> +               bp->b_func = func;
> +               bp->b_file = file;
> +               bp->b_line = line;
> +       }
>         return bp;
>  }
> 
> @@ -485,6 +486,7 @@ libxfs_readbuf(dev_t dev, xfs_daddr_t bl
>                 error = libxfs_readbufr(dev, blkno, bp, len, flags);
>                 if (error) {
>                         libxfs_putbuf(bp);
> +                       errno = error;

I think that libxfs_readbuf() simply returns the
value of the special global "errno" if there is
an error.  And I believe that at this point it
will not have been changed, so there's no need
to assign it here.

In other words, I'd like to remove this one piece
of your patch.

>                         return NULL;
>                 }
>         }
> diff -Nurp xfsprogs/libxfs/trans.c xfsprogs-dirty/libxfs/trans.c
> --- xfsprogs/libxfs/trans.c     2011-01-28 20:22:11.000000000 +0900
> +++ xfsprogs-dirty/libxfs/trans.c       2011-02-02 17:00:42.000000000 +0900
> @@ -508,6 +508,10 @@ libxfs_trans_read_buf(
>         }
> 
>         bp = libxfs_readbuf(dev, blkno, len, flags);
> +       if (!bp){
> +               *bpp = NULL;
> +               return errno;
> +       }
>  #ifdef XACT_DEBUG
>         fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
>  #endif
> 
> _______________________________________________
> 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] xfsprogs: unhandled error check in libxfs_trans_read_buf

[Ajeet Yadav]

Thank you for providing review comment.

On Tue, Feb 8, 2011 at 10:17 AM, Alex Elder <aelder@sgi.com> wrote:
> On Thu, 2011-02-03 at 15:17 +0900, Ajeet Yadav wrote:
>> xfsprogs: unhandled error check in libxfs_trans_read_buf
>
> Sorry you didn't get a response earlier.  Reporting
> problems like this--especially if they come with a
> proposed fix--is always appreciated.
>
>> libxfs_trans_read_buf() is used in both mkfs.xfs & xfs_repair.
>> During stability testing we found some time occur pagefault in mkfs.xfs,
>> code inspection shows that if libxfs_readbuf() fails then occurs a
>> page fault in xfs_buf_item_init() called in libxfs_trans_read_buf().
>>
>> mkfs.xfs: unhandled page fault (11) at 0x00000070, code 0x017
>>
>> Added NULL check and errno handling.
>
> I expect there are other similar problems lurking in
> the libxfs code.
>
> I think your change looks good, with one exception,
> noted below.  I will gladly adjust your patch for
> you if you consent to the change I suggest.
>
>                                        -Alex
>
>> Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
>>
>> diff -Nurp xfsprogs/libxfs/rdwr.c xfsprogs-dirty/libxfs/rdwr.c
>> --- xfsprogs/libxfs/rdwr.c      2011-01-28 20:22:11.000000000 +0900
>> +++ xfsprogs-dirty/libxfs/rdwr.c        2011-02-02 16:59:16.000000000 +0900
>> @@ -207,10 +207,11 @@ libxfs_trace_readbuf(const char *func, c
>>  {
>>         xfs_buf_t       *bp = libxfs_readbuf(dev, blkno, len, flags);
>>
>> -       bp->b_func = func;
>> -       bp->b_file = file;
>> -       bp->b_line = line;
>> -
>> +       if (bp){
>> +               bp->b_func = func;
>> +               bp->b_file = file;
>> +               bp->b_line = line;
>> +       }
>>         return bp;
>>  }
>>
>> @@ -485,6 +486,7 @@ libxfs_readbuf(dev_t dev, xfs_daddr_t bl
>>                 error = libxfs_readbufr(dev, blkno, bp, len, flags);
>>                 if (error) {
>>                         libxfs_putbuf(bp);
>> +                       errno = error;
>
> I think that libxfs_readbuf() simply returns the
> value of the special global "errno" if there is
> an error.  And I believe that at this point it
> will not have been changed, so there's no need
> to assign it here.
>
> In other words, I'd like to remove this one piece
> of your patch.
>
>>                         return NULL;
>>                 }
>>         }
>> diff -Nurp xfsprogs/libxfs/trans.c xfsprogs-dirty/libxfs/trans.c
>> --- xfsprogs/libxfs/trans.c     2011-01-28 20:22:11.000000000 +0900
>> +++ xfsprogs-dirty/libxfs/trans.c       2011-02-02 17:00:42.000000000 +0900
>> @@ -508,6 +508,10 @@ libxfs_trans_read_buf(
>>         }
>>
>>         bp = libxfs_readbuf(dev, blkno, len, flags);
>> +       if (!bp){
>> +               *bpp = NULL;
>> +               return errno;
>> +       }
>>  #ifdef XACT_DEBUG
>>         fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
>>  #endif
>>
>> _______________________________________________
>> 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] xfsprogs: unhandled error check in libxfs_trans_read_buf

[Ajeet Yadav]

xfsprogs: unhandled error check in libxfs_trans_read_buf

libxfs_trans_read_buf() is used in both mkfs.xfs & xfs_repair.
During stability testing we found some time occur pagefault in mkfs.xfs,
code inspection shows that if libxfs_readbuf() fails then occurs a
page fault in xfs_buf_item_init() called in libxfs_trans_read_buf().

mkfs.xfs: unhandled page fault (11) at 0x00000070, code 0x017

Added NULL check and errno handling.

diff -Nurp xfsprogs/libxfs/rdwr.c xfsprogs-dirty/libxfs/rdwr.c
--- xfsprogs/libxfs/rdwr.c      2011-01-28 20:22:11.000000000 +0900
+++ xfsprogs-dirty/libxfs/rdwr.c        2011-02-02 16:59:16.000000000 +0900
@@ -207,10 +207,11 @@ libxfs_trace_readbuf(const char *func, c
 {
        xfs_buf_t       *bp = libxfs_readbuf(dev, blkno, len, flags);

-       bp->b_func = func;
-       bp->b_file = file;
-       bp->b_line = line;
-
+       if (bp){
+               bp->b_func = func;
+               bp->b_file = file;
+               bp->b_line = line;
+       }
        return bp;
 }

diff -Nurp xfsprogs/libxfs/trans.c xfsprogs-dirty/libxfs/trans.c
--- xfsprogs/libxfs/trans.c     2011-01-28 20:22:11.000000000 +0900
+++ xfsprogs-dirty/libxfs/trans.c       2011-02-02 17:00:42.000000000 +0900
@@ -508,6 +508,10 @@ libxfs_trans_read_buf(
        }

        bp = libxfs_readbuf(dev, blkno, len, flags);
+       if (!bp){
+               *bpp = NULL;
+               return errno;
+       }
 #ifdef XACT_DEBUG
        fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
 #endif

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