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

[Dave Chinner <david@fromorbit.com>]

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