[PATCH] mkfs.xfs: fix segmentation fault caused by accessing a null pointer

[PATCH] mkfs.xfs: fix segmentation fault caused by accessing a null pointer

[Wu Guanghao]

We encountered a segfault while testing the mkfs.xfs + iscsi.

(gdb) bt
#0 libxfs_log_sb (tp=0xaaaafaea0630) at xfs_sb.c:810
#1 0x0000aaaaca991468 in __xfs_trans_commit (tp=<optimized out>, tp@entry=0xaaaafaea0630, regrant=regrant@entry=true) at trans.c:995
#2 0x0000aaaaca991790 in libxfs_trans_roll (tpp=tpp@entry=0xfffffe1f3018) at trans.c:103
#3 0x0000aaaaca9bcde8 in xfs_dialloc_roll (agibp=0xaaaafaea2fa0, tpp=0xfffffe1f31c8) at xfs_ialloc.c:1561
#4 xfs_dialloc_try_ag (ok_alloc=true, new_ino=<synthetic pointer>, parent=0, pag=0xaaaafaea0210, tpp=0xfffffe1f31c8) at xfs_ialloc.c:1698
#5 xfs_dialloc (tpp=tpp@entry=0xfffffe1f31c8, parent=0, mode=mode@entry=16877, new_ino=new_ino@entry=0xfffffe1f3128) at xfs_ialloc.c:1776
#6 0x0000aaaaca9925b0 in libxfs_dir_ialloc (tpp=tpp@entry=0xfffffe1f31c8, dp=dp@entry=0x0, mode=mode@entry=16877, nlink=nlink@entry=1, rdev=rdev@entry=0, cr=cr@entry=0xfffffe1f31d0,
    fsx=fsx@entry=0xfffffe1f36a4, ipp=ipp@entry=0xfffffe1f31c0) at util.c:525
#7 0x0000aaaaca988fac in parseproto (mp=0xfffffe1f36c8, pip=0x0, fsxp=0xfffffe1f36a4, pp=0xfffffe1f3370, name=0x0) at proto.c:552
#8 0x0000aaaaca9867a4 in main (argc=<optimized out>, argv=<optimized out>) at xfs_mkfs.c:4217

(gdb) p bp
$1 = 0x0

```
void
xfs_log_sb(
        struct xfs_trans        *tp)
{
        // iscsi offline
        ...
        // failed to read sb, bp = NULL
        struct xfs_buf          *bp = xfs_trans_getsb(tp);
        ...
}
```

When writing data to sb, if the device is abnormal at this time,
the bp may be empty. Using it without checking will result in
a segfault.

So before using it, we need to check if the bp is empty and return
the error.

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
---
 libxfs/trans.c         |  4 +++-
 libxfs/xfs_attr_leaf.c |  2 +-
 libxfs/xfs_bmap.c      |  7 +++++--
 libxfs/xfs_sb.c        | 23 ++++++++++++++++++++---
 libxfs/xfs_sb.h        |  2 +-
 mkfs/proto.c           | 15 ++++++++++++---
 6 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/libxfs/trans.c b/libxfs/trans.c
index 553f9471..b37e92d2 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -992,7 +992,9 @@ __xfs_trans_commit(
                        sbp->sb_fdblocks += tp->t_fdblocks_delta;
                if (tp->t_frextents_delta)
                        sbp->sb_frextents += tp->t_frextents_delta;
-               xfs_log_sb(tp);
+               error = xfs_log_sb(tp);
+               if (error)
+                       goto out_unreserve;
        }

        trans_committed(tp);
diff --git a/libxfs/xfs_attr_leaf.c b/libxfs/xfs_attr_leaf.c
index 6cac2531..73079df1 100644
--- a/libxfs/xfs_attr_leaf.c
+++ b/libxfs/xfs_attr_leaf.c
@@ -668,7 +668,7 @@ xfs_sbversion_add_attr2(
        spin_lock(&mp->m_sb_lock);
        xfs_add_attr2(mp);
        spin_unlock(&mp->m_sb_lock);
-       xfs_log_sb(tp);
+       ASSERT(!xfs_log_sb(tp));
 }

 /*
diff --git a/libxfs/xfs_bmap.c b/libxfs/xfs_bmap.c
index 76591d07..63c3cf7f 100644
--- a/libxfs/xfs_bmap.c
+++ b/libxfs/xfs_bmap.c
@@ -1053,8 +1053,11 @@ xfs_bmap_add_attrfork(
                        log_sb = true;
                }
                spin_unlock(&mp->m_sb_lock);
-               if (log_sb)
-                       xfs_log_sb(tp);
+               if (log_sb) {
+                       error = xfs_log_sb(tp);
+                       if (error)
+                               goto trans_cancel;
+               }
        }

        error = xfs_trans_commit(tp);
diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index 64b4e7be..21611427 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -949,13 +949,15 @@ xfs_sb_mount_common(
  * level of locking that is needed to protect the in-core superblock from
  * concurrent access.
  */
-void
+int
 xfs_log_sb(
        struct xfs_trans        *tp)
 {
        struct xfs_mount        *mp = tp->t_mountp;
        struct xfs_buf          *bp = xfs_trans_getsb(tp);

+       if (!bp)
+               return -EIO;
        /*
         * Lazy sb counters don't update the in-core superblock so do that now.
         * If this is at unmount, the counters will be exactly correct, but at
@@ -980,6 +982,8 @@ xfs_log_sb(
        xfs_sb_to_disk(bp->b_addr, &mp->m_sb);
        xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
        xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb) - 1);
+
+       return 0;
 }

 /*
@@ -1006,7 +1010,12 @@ xfs_sync_sb(
        if (error)
                return error;

-       xfs_log_sb(tp);
+       error = xfs_log_sb(tp);
+       if (error) {
+               xfs_trans_cancel(tp);
+               return error;
+       }
+
        if (wait)
                xfs_trans_set_sync(tp);
        return xfs_trans_commit(tp);
@@ -1103,7 +1112,15 @@ xfs_sync_sb_buf(
                return error;

        bp = xfs_trans_getsb(tp);
-       xfs_log_sb(tp);
+       if (!bp) {
+               xfs_trans_cancel(tp);
+               return -EIO;
+       }
+       error = xfs_log_sb(tp);
+       if (error) {
+               xfs_trans_cancel(tp);
+               return error;
+       }
        xfs_trans_bhold(tp, bp);
        xfs_trans_set_sync(tp);
        error = xfs_trans_commit(tp);
diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
index a5e14740..fa7d4496 100644
--- a/libxfs/xfs_sb.h
+++ b/libxfs/xfs_sb.h
@@ -13,7 +13,7 @@ struct xfs_trans;
 struct xfs_fsop_geom;
 struct xfs_perag;

-extern void    xfs_log_sb(struct xfs_trans *tp);
+extern int     xfs_log_sb(struct xfs_trans *tp);
 extern int     xfs_sync_sb(struct xfs_mount *mp, bool wait);
 extern int     xfs_sync_sb_buf(struct xfs_mount *mp);
 extern void    xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
diff --git a/mkfs/proto.c b/mkfs/proto.c
index ea31cfe5..1e7ad915 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -589,7 +589,10 @@ parseproto(
                if (!pip) {
                        pip = ip;
                        mp->m_sb.sb_rootino = ip->i_ino;
-                       libxfs_log_sb(tp);
+                       error = -libxfs_log_sb(tp);
+                       if (error) {
+                               fail(_("Log sb failed"), error);
+                       }
                        isroot = 1;
                } else {
                        libxfs_trans_ijoin(tp, pip, 0);
@@ -690,7 +693,10 @@ rtinit(
        rbmip->i_diflags = XFS_DIFLAG_NEWRTBM;
        *(uint64_t *)&VFS_I(rbmip)->i_atime = 0;
        libxfs_trans_log_inode(tp, rbmip, XFS_ILOG_CORE);
-       libxfs_log_sb(tp);
+       error = -libxfs_log_sb(tp);
+       if (error) {
+               fail(_("Log sb failed"), error);
+       }
        mp->m_rbmip = rbmip;
        error = -libxfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0,
                                        &creds, &fsxattrs, &rsumip);
@@ -700,7 +706,10 @@ rtinit(
        mp->m_sb.sb_rsumino = rsumip->i_ino;
        rsumip->i_disk_size = mp->m_rsumsize;
        libxfs_trans_log_inode(tp, rsumip, XFS_ILOG_CORE);
-       libxfs_log_sb(tp);
+       error = -libxfs_log_sb(tp);
+       if (error) {
+               fail(_("Log sb failed"), error);
+       }
        error = -libxfs_trans_commit(tp);
        if (error)
                fail(_("Completion of the realtime summary inode failed"),
--
2.27.0

Re: [PATCH] mkfs.xfs: fix segmentation fault caused by accessing a null pointer

[Dave Chinner]

On Wed, Jun 21, 2023 at 05:25:27PM +0800, Wu Guanghao wrote:
> We encountered a segfault while testing the mkfs.xfs + iscsi.
> 
> (gdb) bt
> #0 libxfs_log_sb (tp=0xaaaafaea0630) at xfs_sb.c:810
> #1 0x0000aaaaca991468 in __xfs_trans_commit (tp=<optimized out>, tp@entry=0xaaaafaea0630, regrant=regrant@entry=true) at trans.c:995
> #2 0x0000aaaaca991790 in libxfs_trans_roll (tpp=tpp@entry=0xfffffe1f3018) at trans.c:103
> #3 0x0000aaaaca9bcde8 in xfs_dialloc_roll (agibp=0xaaaafaea2fa0, tpp=0xfffffe1f31c8) at xfs_ialloc.c:1561
> #4 xfs_dialloc_try_ag (ok_alloc=true, new_ino=<synthetic pointer>, parent=0, pag=0xaaaafaea0210, tpp=0xfffffe1f31c8) at xfs_ialloc.c:1698
> #5 xfs_dialloc (tpp=tpp@entry=0xfffffe1f31c8, parent=0, mode=mode@entry=16877, new_ino=new_ino@entry=0xfffffe1f3128) at xfs_ialloc.c:1776
> #6 0x0000aaaaca9925b0 in libxfs_dir_ialloc (tpp=tpp@entry=0xfffffe1f31c8, dp=dp@entry=0x0, mode=mode@entry=16877, nlink=nlink@entry=1, rdev=rdev@entry=0, cr=cr@entry=0xfffffe1f31d0,
>     fsx=fsx@entry=0xfffffe1f36a4, ipp=ipp@entry=0xfffffe1f31c0) at util.c:525
> #7 0x0000aaaaca988fac in parseproto (mp=0xfffffe1f36c8, pip=0x0, fsxp=0xfffffe1f36a4, pp=0xfffffe1f3370, name=0x0) at proto.c:552
> #8 0x0000aaaaca9867a4 in main (argc=<optimized out>, argv=<optimized out>) at xfs_mkfs.c:4217
> 
> (gdb) p bp
> $1 = 0x0
> 
> ```
> void
> xfs_log_sb(
>         struct xfs_trans        *tp)
> {
>         // iscsi offline
>         ...
>         // failed to read sb, bp = NULL
>         struct xfs_buf          *bp = xfs_trans_getsb(tp);
>         ...
> }
> ```
> 
> When writing data to sb, if the device is abnormal at this time,
> the bp may be empty. Using it without checking will result in
> a segfault.

xfs_trans_getsb() is not supposed to fail. In the kernel code (which
this is a copy of) it can't fail because the superblock buffer is
always pinned in memory at mount time and so is *never read from the
storage* after mount.

Hence something similar needs to be in userspace with libxfs_getsb()
so that the superblock is only read when setting up the initial
mount state in libxfs....

> diff --git a/libxfs/xfs_attr_leaf.c b/libxfs/xfs_attr_leaf.c
> index 6cac2531..73079df1 100644
> --- a/libxfs/xfs_attr_leaf.c
> +++ b/libxfs/xfs_attr_leaf.c
> @@ -668,7 +668,7 @@ xfs_sbversion_add_attr2(
>         spin_lock(&mp->m_sb_lock);
>         xfs_add_attr2(mp);
>         spin_unlock(&mp->m_sb_lock);
> -       xfs_log_sb(tp);
> +       ASSERT(!xfs_log_sb(tp));

FWIW, that's never a valid conversion nor a valid way to handle
something that can fail. That turns the code into code that is only
executed on debug builds, and it will panic the debug build rather
than handle the error.....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] mkfs.xfs: fix segmentation fault caused by accessing a null pointer

[Wu Guanghao]

在 2023/6/22 5:54, Dave Chinner 写道:
> On Wed, Jun 21, 2023 at 05:25:27PM +0800, Wu Guanghao wrote:
>> We encountered a segfault while testing the mkfs.xfs + iscsi.
>>
>> (gdb) bt
>> #0 libxfs_log_sb (tp=0xaaaafaea0630) at xfs_sb.c:810
>> #1 0x0000aaaaca991468 in __xfs_trans_commit (tp=<optimized out>, tp@entry=0xaaaafaea0630, regrant=regrant@entry=true) at trans.c:995
>> #2 0x0000aaaaca991790 in libxfs_trans_roll (tpp=tpp@entry=0xfffffe1f3018) at trans.c:103
>> #3 0x0000aaaaca9bcde8 in xfs_dialloc_roll (agibp=0xaaaafaea2fa0, tpp=0xfffffe1f31c8) at xfs_ialloc.c:1561
>> #4 xfs_dialloc_try_ag (ok_alloc=true, new_ino=<synthetic pointer>, parent=0, pag=0xaaaafaea0210, tpp=0xfffffe1f31c8) at xfs_ialloc.c:1698
>> #5 xfs_dialloc (tpp=tpp@entry=0xfffffe1f31c8, parent=0, mode=mode@entry=16877, new_ino=new_ino@entry=0xfffffe1f3128) at xfs_ialloc.c:1776
>> #6 0x0000aaaaca9925b0 in libxfs_dir_ialloc (tpp=tpp@entry=0xfffffe1f31c8, dp=dp@entry=0x0, mode=mode@entry=16877, nlink=nlink@entry=1, rdev=rdev@entry=0, cr=cr@entry=0xfffffe1f31d0,
>>     fsx=fsx@entry=0xfffffe1f36a4, ipp=ipp@entry=0xfffffe1f31c0) at util.c:525
>> #7 0x0000aaaaca988fac in parseproto (mp=0xfffffe1f36c8, pip=0x0, fsxp=0xfffffe1f36a4, pp=0xfffffe1f3370, name=0x0) at proto.c:552
>> #8 0x0000aaaaca9867a4 in main (argc=<optimized out>, argv=<optimized out>) at xfs_mkfs.c:4217
>>
>> (gdb) p bp
>> $1 = 0x0
>>
>> ```
>> void
>> xfs_log_sb(
>>         struct xfs_trans        *tp)
>> {
>>         // iscsi offline
>>         ...
>>         // failed to read sb, bp = NULL
>>         struct xfs_buf          *bp = xfs_trans_getsb(tp);
>>         ...
>> }
>> ```
>>
>> When writing data to sb, if the device is abnormal at this time,
>> the bp may be empty. Using it without checking will result in
>> a segfault.
> 
> xfs_trans_getsb() is not supposed to fail. In the kernel code (which
> this is a copy of) it can't fail because the superblock buffer is
> always pinned in memory at mount time and so is *never read from the
> storage* after mount.
> 
Thank you for your suggestion. Later, I will send a patch for the V2 patch.

Thanks,
Guanghao
> Hence something similar needs to be in userspace with libxfs_getsb()
> so that the superblock is only read when setting up the initial
> mount state in libxfs....
> 
>> diff --git a/libxfs/xfs_attr_leaf.c b/libxfs/xfs_attr_leaf.c
>> index 6cac2531..73079df1 100644
>> --- a/libxfs/xfs_attr_leaf.c
>> +++ b/libxfs/xfs_attr_leaf.c
>> @@ -668,7 +668,7 @@ xfs_sbversion_add_attr2(
>>         spin_lock(&mp->m_sb_lock);
>>         xfs_add_attr2(mp);
>>         spin_unlock(&mp->m_sb_lock);
>> -       xfs_log_sb(tp);
>> +       ASSERT(!xfs_log_sb(tp));
> 
> FWIW, that's never a valid conversion nor a valid way to handle
> something that can fail. That turns the code into code that is only
> executed on debug builds, and it will panic the debug build rather
> than handle the error.....
> 
> -Dave.
>