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

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jun 29, 2023 at 10:20:30AM +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.
> 
> So it's necessary to ensure that the superblock has been cached.
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
>  mkfs/xfs_mkfs.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 7b3c2304..8d0ec4b5 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -4406,6 +4406,15 @@ main(
>                 exit(1);
>         }
> 
> +       /*
> +        *  Cached superblock to ensure that xfs_trans_getsb() will not return NULL.
> +        */
> +       buf = libxfs_getsb(mp);

prepare_devices() already creates an uncached xfs_buf for the
superblock.  Why not reuse that instead of rereading the buffer here?

> +       if (!buf || buf->b_error) {
> +               fprintf(stderr, _("%s: read superblock failed, err=%d\n"),
> +                               progname, !buf ? EIO : -buf->b_error);
> +               exit(1);
> +       }
>         /*
>          * Initialise the freespace freelists (i.e. AGFLs) in each AG.
>          */
> @@ -4433,6 +4442,7 @@ main(
>          * Need to drop references to inodes we still hold, first.
>          */
>         libxfs_rtmount_destroy(mp);
> +       libxfs_buf_relse(buf);
>         libxfs_bcache_purge();
> 
>         /*

...and then we don't have read it yet again down here to clear the
sb_inprogress field?

--D

> --
> 2.27.0