| Message ID | 93a080c7-5eb8-8ffe-ae5b-5152a7713828@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: log proper length of superblock | expand |
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); > } > > /* >
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.
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); > } > > /* >
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); } /*
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...