[PATCH] xfs: log proper length of superblock

[PATCH] xfs: log proper length of superblock

[Eric Sandeen]

xfs_trans_log_buf takes first byte, last byte as args.  In this
case, it should be from 0 to sizeof() - 1.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

I should have audited everything when I sent the last patch for
this type of error.  hch suggested changing the interface but it's
all pretty grotty and I'm hesitant for now.

I think maybe a new/separate function to take start, len might
make sense so that not every caller needs to be munged into a new
format, because some of the existing callers would then become more
complex...

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index a08dd8f40346..ac6cdca63e15 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -928,7 +928,7 @@ xfs_log_sb(
 
 	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &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));
+	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb) - 1);
 }
 
 /*

Re: [PATCH] xfs: log proper length of superblock

[Eric Sandeen]

On 9/23/19 4:18 PM, Eric Sandeen wrote:
> xfs_trans_log_buf takes first byte, last byte as args.  In this
> case, it should be from 0 to sizeof() - 1.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

if you want, you could put a 

Fixes: 4d11a40239405 ("xfs: remove bitfield based superblock updates")

on this, I guess it was technically a regression in v4.0, even
if it has no net effect like last time...

-Eric

> ---
> 
> I should have audited everything when I sent the last patch for
> this type of error.  hch suggested changing the interface but it's
> all pretty grotty and I'm hesitant for now.
> 
> I think maybe a new/separate function to take start, len might
> make sense so that not every caller needs to be munged into a new
> format, because some of the existing callers would then become more
> complex...
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index a08dd8f40346..ac6cdca63e15 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -928,7 +928,7 @@ xfs_log_sb(
>  
>  	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &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));
> +	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb) - 1);
>  }
>  
>  /*
> 

Re: [PATCH] xfs: log proper length of superblock

[Dave Chinner]

On Mon, Sep 23, 2019 at 04:44:14PM -0500, Eric Sandeen wrote:
> On 9/23/19 4:18 PM, Eric Sandeen wrote:
> > xfs_trans_log_buf takes first byte, last byte as args.  In this
> > case, it should be from 0 to sizeof() - 1.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Well spotted!

> if you want, you could put a 
> 
> Fixes: 4d11a40239405 ("xfs: remove bitfield based superblock updates")
> 
> on this, I guess it was technically a regression in v4.0, even
> if it has no net effect like last time...

Yeah, it doesn't expose any issue at all. The buffer logging rounds
out to CHUNK_SIZE - 128 bytes - and when we look at the size of the
superblock w/ pahole:

struct xfs_dsb {
        __be32                     sb_magicnum;          /*     0     4 */
	__be32                     sb_blocksize;         /*     4     4 */
....
        uuid_t                     sb_meta_uuid;         /*   248    16 */

        /* size: 264, cachelines: 5, members: 55 */
        /* last cacheline: 8 bytes */
};

Well be logging the first three chunks in the sb regardless of
whether we pass 263 or 264 as the size of the modified area to
xfs_trans_log_buf() (i.e. first 384 bytes of the buffer get logged
either way).

So, yeah, the code is wrong, but it does not result in any
observable incorrect behaviour. That said, it still needs fixing.

> -Eric
> 
> > ---
> > 
> > I should have audited everything when I sent the last patch for
> > this type of error.  hch suggested changing the interface but it's
> > all pretty grotty and I'm hesitant for now.
> > 
> > I think maybe a new/separate function to take start, len might
> > make sense so that not every caller needs to be munged into a new
> > format, because some of the existing callers would then become more
> > complex...
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index a08dd8f40346..ac6cdca63e15 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -928,7 +928,7 @@ xfs_log_sb(
> >  
> >  	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &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));
> > +	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb) - 1);
> >  }
> >  
> >  /*

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

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

Re: [PATCH] xfs: log proper length of superblock

[Darrick J. Wong]

On Mon, Sep 23, 2019 at 04:18:44PM -0500, Eric Sandeen wrote:
> xfs_trans_log_buf takes first byte, last byte as args.  In this
> case, it should be from 0 to sizeof() - 1.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> I should have audited everything when I sent the last patch for
> this type of error.  hch suggested changing the interface but it's
> all pretty grotty and I'm hesitant for now.
> 
> I think maybe a new/separate function to take start, len might
> make sense so that not every caller needs to be munged into a new
> format, because some of the existing callers would then become more
> complex...
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index a08dd8f40346..ac6cdca63e15 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -928,7 +928,7 @@ xfs_log_sb(
>  
>  	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &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));
> +	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb) - 1);
>  }
>  
>  /*
>