| Message ID | 20180716192655.27792-1-billodo@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > Add sanity checks for these parameters. > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > --- > v2: make extra sanity checks exclusive to writes (allow read) It looks mostly ok for me, I'm still wondering if it wouldn't be better to avoid adding a write_flag argument for xfs_sb_verify, but I think avoiding it would require some refactoring of the sb verify code, once we convert from BE to CPU in xfs_sb_verify, and I'm not sure if it is worth. So, Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index 350119eeaecb..6a98ec68e8ad 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > xfs_mount_t *mp, > xfs_sb_t *sbp, > bool check_inprogress, > - bool check_version) > + bool check_version, > + bool write_flag) > { > uint32_t agcount = 0; > uint32_t rem; > @@ -266,6 +267,15 @@ xfs_mount_validate_sb( > return -EFSCORRUPTED; > } > > + /* Additional sb sanity checks for writes */ > + if (write_flag) { > + if (sbp->sb_fdblocks > sbp->sb_dblocks || > + sbp->sb_ifree > sbp->sb_icount) { > + xfs_notice(mp, "SB sanity check failed"); > + return -EFSCORRUPTED; > + } > + } > + > if (sbp->sb_unit) { > if (!xfs_sb_version_hasdalign(sbp) || > sbp->sb_unit > sbp->sb_width || > @@ -599,7 +609,9 @@ xfs_sb_to_disk( > static int > xfs_sb_verify( > struct xfs_buf *bp, > - bool check_version) > + bool check_version, > + bool write_flag) > + > { > struct xfs_mount *mp = bp->b_target->bt_mount; > struct xfs_sb sb; > @@ -616,7 +628,7 @@ xfs_sb_verify( > */ > return xfs_mount_validate_sb(mp, &sb, > bp->b_maps[0].bm_bn == XFS_SB_DADDR, > - check_version); > + check_version, write_flag); > } > > /* > @@ -657,7 +669,7 @@ xfs_sb_read_verify( > } > } > } > - error = xfs_sb_verify(bp, true); > + error = xfs_sb_verify(bp, true, false); > > out_error: > if (error == -EFSCORRUPTED || error == -EFSBADCRC) > @@ -695,7 +707,7 @@ xfs_sb_write_verify( > struct xfs_buf_log_item *bip = bp->b_log_item; > int error; > > - error = xfs_sb_verify(bp, false); > + error = xfs_sb_verify(bp, false, true); > if (error) { > xfs_verifier_error(bp, error, __this_address); > return; > -- > 2.17.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > Add sanity checks for these parameters. > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > --- > v2: make extra sanity checks exclusive to writes (allow read) > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index 350119eeaecb..6a98ec68e8ad 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > xfs_mount_t *mp, > xfs_sb_t *sbp, > bool check_inprogress, > - bool check_version) > + bool check_version, > + bool write_flag) I notice that check_version and write_flag are always xor -- either we're reading the sb and set check_version, or we're writing the sb and set write_flag. Perhaps we can combine these two as write_flag? if (check_version) check version stuff... becomes: if (!write_flag) check version stuff... and we only have to pass around one flag. > { > uint32_t agcount = 0; > uint32_t rem; > @@ -266,6 +267,15 @@ xfs_mount_validate_sb( > return -EFSCORRUPTED; > } > > + /* Additional sb sanity checks for writes */ > + if (write_flag) { > + if (sbp->sb_fdblocks > sbp->sb_dblocks || > + sbp->sb_ifree > sbp->sb_icount) { Hmm, we still need something that will detect this on read and set a flag to force recalculation of the summary counters... though since a patch to implement that flag is sitting in my tree I'll take care of that part separately. --D > + xfs_notice(mp, "SB sanity check failed"); > + return -EFSCORRUPTED; > + } > + } > + > if (sbp->sb_unit) { > if (!xfs_sb_version_hasdalign(sbp) || > sbp->sb_unit > sbp->sb_width || > @@ -599,7 +609,9 @@ xfs_sb_to_disk( > static int > xfs_sb_verify( > struct xfs_buf *bp, > - bool check_version) > + bool check_version, > + bool write_flag) > + > { > struct xfs_mount *mp = bp->b_target->bt_mount; > struct xfs_sb sb; > @@ -616,7 +628,7 @@ xfs_sb_verify( > */ > return xfs_mount_validate_sb(mp, &sb, > bp->b_maps[0].bm_bn == XFS_SB_DADDR, > - check_version); > + check_version, write_flag); > } > > /* > @@ -657,7 +669,7 @@ xfs_sb_read_verify( > } > } > } > - error = xfs_sb_verify(bp, true); > + error = xfs_sb_verify(bp, true, false); > > out_error: > if (error == -EFSCORRUPTED || error == -EFSBADCRC) > @@ -695,7 +707,7 @@ xfs_sb_write_verify( > struct xfs_buf_log_item *bip = bp->b_log_item; > int error; > > - error = xfs_sb_verify(bp, false); > + error = xfs_sb_verify(bp, false, true); > if (error) { > xfs_verifier_error(bp, error, __this_address); > return; > -- > 2.17.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote: > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > > Add sanity checks for these parameters. > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > --- > > v2: make extra sanity checks exclusive to writes (allow read) > > > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > index 350119eeaecb..6a98ec68e8ad 100644 > > --- a/fs/xfs/libxfs/xfs_sb.c > > +++ b/fs/xfs/libxfs/xfs_sb.c > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > > xfs_mount_t *mp, > > xfs_sb_t *sbp, > > bool check_inprogress, > > - bool check_version) > > + bool check_version, > > + bool write_flag) > > I notice that check_version and write_flag are always xor -- either > we're reading the sb and set check_version, or we're writing the sb and > set write_flag. Perhaps we can combine these two as write_flag? > > if (check_version) > check version stuff... > > becomes: > > if (!write_flag) > check version stuff... > > and we only have to pass around one flag. I suppose that makes sense, but my notion is that 2 unique flags is preferable for clarity and mutual exclusiveness for anyone doing subsequent patches. > > > { > > uint32_t agcount = 0; > > uint32_t rem; > > @@ -266,6 +267,15 @@ xfs_mount_validate_sb( > > return -EFSCORRUPTED; > > } > > > > + /* Additional sb sanity checks for writes */ > > + if (write_flag) { > > + if (sbp->sb_fdblocks > sbp->sb_dblocks || > > + sbp->sb_ifree > sbp->sb_icount) { > > Hmm, we still need something that will detect this on read and set a > flag to force recalculation of the summary counters... though since a > patch to implement that flag is sitting in my tree I'll take care of > that part separately. That sounds good, thanks! -Bill > > --D > > > + xfs_notice(mp, "SB sanity check failed"); > > + return -EFSCORRUPTED; > > + } > > + } > > + > > if (sbp->sb_unit) { > > if (!xfs_sb_version_hasdalign(sbp) || > > sbp->sb_unit > sbp->sb_width || > > @@ -599,7 +609,9 @@ xfs_sb_to_disk( > > static int > > xfs_sb_verify( > > struct xfs_buf *bp, > > - bool check_version) > > + bool check_version, > > + bool write_flag) > > + > > { > > struct xfs_mount *mp = bp->b_target->bt_mount; > > struct xfs_sb sb; > > @@ -616,7 +628,7 @@ xfs_sb_verify( > > */ > > return xfs_mount_validate_sb(mp, &sb, > > bp->b_maps[0].bm_bn == XFS_SB_DADDR, > > - check_version); > > + check_version, write_flag); > > } > > > > /* > > @@ -657,7 +669,7 @@ xfs_sb_read_verify( > > } > > } > > } > > - error = xfs_sb_verify(bp, true); > > + error = xfs_sb_verify(bp, true, false); > > > > out_error: > > if (error == -EFSCORRUPTED || error == -EFSBADCRC) > > @@ -695,7 +707,7 @@ xfs_sb_write_verify( > > struct xfs_buf_log_item *bip = bp->b_log_item; > > int error; > > > > - error = xfs_sb_verify(bp, false); > > + error = xfs_sb_verify(bp, false, true); > > if (error) { > > xfs_verifier_error(bp, error, __this_address); > > return; > > -- > > 2.17.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote: > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote: > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > > > Add sanity checks for these parameters. > > > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > > --- > > > v2: make extra sanity checks exclusive to writes (allow read) > > > > > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > index 350119eeaecb..6a98ec68e8ad 100644 > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > > > xfs_mount_t *mp, > > > xfs_sb_t *sbp, > > > bool check_inprogress, > > > - bool check_version) > > > + bool check_version, > > > + bool write_flag) > > > > I notice that check_version and write_flag are always xor -- either > > we're reading the sb and set check_version, or we're writing the sb and > > set write_flag. Perhaps we can combine these two as write_flag? > > > > if (check_version) > > check version stuff... > > > > becomes: > > > > if (!write_flag) > > check version stuff... > > > > and we only have to pass around one flag. > > I suppose that makes sense, but my notion is that 2 unique flags > is preferable for clarity and mutual exclusiveness for anyone doing > subsequent patches. I'm all for simplifying and saving stack space, but is it ok to turn a single purpose flag into a dual purpose one? > > > > > > { > > > uint32_t agcount = 0; > > > uint32_t rem; > > > @@ -266,6 +267,15 @@ xfs_mount_validate_sb( > > > return -EFSCORRUPTED; > > > } > > > > > > + /* Additional sb sanity checks for writes */ > > > + if (write_flag) { > > > + if (sbp->sb_fdblocks > sbp->sb_dblocks || > > > + sbp->sb_ifree > sbp->sb_icount) { > > > > Hmm, we still need something that will detect this on read and set a > > flag to force recalculation of the summary counters... though since a > > patch to implement that flag is sitting in my tree I'll take care of > > that part separately. > > That sounds good, thanks! > -Bill > > > > > --D > > > > > + xfs_notice(mp, "SB sanity check failed"); > > > + return -EFSCORRUPTED; > > > + } > > > + } > > > + > > > if (sbp->sb_unit) { > > > if (!xfs_sb_version_hasdalign(sbp) || > > > sbp->sb_unit > sbp->sb_width || > > > @@ -599,7 +609,9 @@ xfs_sb_to_disk( > > > static int > > > xfs_sb_verify( > > > struct xfs_buf *bp, > > > - bool check_version) > > > + bool check_version, > > > + bool write_flag) > > > + > > > { > > > struct xfs_mount *mp = bp->b_target->bt_mount; > > > struct xfs_sb sb; > > > @@ -616,7 +628,7 @@ xfs_sb_verify( > > > */ > > > return xfs_mount_validate_sb(mp, &sb, > > > bp->b_maps[0].bm_bn == XFS_SB_DADDR, > > > - check_version); > > > + check_version, write_flag); > > > } > > > > > > /* > > > @@ -657,7 +669,7 @@ xfs_sb_read_verify( > > > } > > > } > > > } > > > - error = xfs_sb_verify(bp, true); > > > + error = xfs_sb_verify(bp, true, false); > > > > > > out_error: > > > if (error == -EFSCORRUPTED || error == -EFSBADCRC) > > > @@ -695,7 +707,7 @@ xfs_sb_write_verify( > > > struct xfs_buf_log_item *bip = bp->b_log_item; > > > int error; > > > > > > - error = xfs_sb_verify(bp, false); > > > + error = xfs_sb_verify(bp, false, true); > > > if (error) { > > > xfs_verifier_error(bp, error, __this_address); > > > return; > > > -- > > > 2.17.1 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 17, 2018 at 02:12:53PM -0500, Bill O'Donnell wrote: > On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote: > > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote: > > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > > > > Add sanity checks for these parameters. > > > > > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > > > --- > > > > v2: make extra sanity checks exclusive to writes (allow read) > > > > > > > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > > index 350119eeaecb..6a98ec68e8ad 100644 > > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > > > > xfs_mount_t *mp, > > > > xfs_sb_t *sbp, > > > > bool check_inprogress, > > > > - bool check_version) > > > > + bool check_version, > > > > + bool write_flag) > > > > > > I notice that check_version and write_flag are always xor -- either > > > we're reading the sb and set check_version, or we're writing the sb and > > > set write_flag. Perhaps we can combine these two as write_flag? > > > > > > if (check_version) > > > check version stuff... > > > > > > becomes: > > > > > > if (!write_flag) > > > check version stuff... > > > > > > and we only have to pass around one flag. > > > > I suppose that makes sense, but my notion is that 2 unique flags > > is preferable for clarity and mutual exclusiveness for anyone doing > > subsequent patches. > > I'm all for simplifying and saving stack space, but is it ok > to turn a single purpose flag into a dual purpose one? That depends on the flag involved -- if they're mutually exclusive, then I think it's ok to do that, so long as there's a comment nearby to document the argument semantics. In the case of this particular flag (check_version) it is set by the read verifier so that we reject versions that we don't recognize; it is not set by the write verifier because we don't change the v5 feature masks at runtime* and we never write anything if the fs won't mount. For write_flag, the read verifier never sets it because we have to be able to mount the fs in case the log contains an sb with an updated set of summary counters or for lazysbcount filesystems we'll recalculate the counter after recovery; and we set write_flag at write time, obviously. So having come this far, you could meld them into a single parameter so long as you note that write_flag == true means that we're writing the fs and write_flag == false means we want to check the v5 feature flags at mount time to reject features bits that we don't recognize. * Oh, but what about that pesky asterisk? My sense of paranoia wonders why we don't check the v5 feature flags on write too, just in case memory gets corrupted. I think the reason is that we don't allow feature flag changes at runtime, we'll check the changes at ioctl time if we ever do support runtime feature flag updates, and we implicitly trust memory not to corrupt memory on us (ha ha ha). At this point my tldr opinion is "seems fine to me, let's see if any of the lurking vacationers have anything to say? We're still ~3 weeks to the next merge window. --D > > > > > > > > > { > > > > uint32_t agcount = 0; > > > > uint32_t rem; > > > > @@ -266,6 +267,15 @@ xfs_mount_validate_sb( > > > > return -EFSCORRUPTED; > > > > } > > > > > > > > + /* Additional sb sanity checks for writes */ > > > > + if (write_flag) { > > > > + if (sbp->sb_fdblocks > sbp->sb_dblocks || > > > > + sbp->sb_ifree > sbp->sb_icount) { > > > > > > Hmm, we still need something that will detect this on read and set a > > > flag to force recalculation of the summary counters... though since a > > > patch to implement that flag is sitting in my tree I'll take care of > > > that part separately. > > > > That sounds good, thanks! > > -Bill > > > > > > > > --D > > > > > > > + xfs_notice(mp, "SB sanity check failed"); > > > > + return -EFSCORRUPTED; > > > > + } > > > > + } > > > > + > > > > if (sbp->sb_unit) { > > > > if (!xfs_sb_version_hasdalign(sbp) || > > > > sbp->sb_unit > sbp->sb_width || > > > > @@ -599,7 +609,9 @@ xfs_sb_to_disk( > > > > static int > > > > xfs_sb_verify( > > > > struct xfs_buf *bp, > > > > - bool check_version) > > > > + bool check_version, > > > > + bool write_flag) > > > > + > > > > { > > > > struct xfs_mount *mp = bp->b_target->bt_mount; > > > > struct xfs_sb sb; > > > > @@ -616,7 +628,7 @@ xfs_sb_verify( > > > > */ > > > > return xfs_mount_validate_sb(mp, &sb, > > > > bp->b_maps[0].bm_bn == XFS_SB_DADDR, > > > > - check_version); > > > > + check_version, write_flag); > > > > } > > > > > > > > /* > > > > @@ -657,7 +669,7 @@ xfs_sb_read_verify( > > > > } > > > > } > > > > } > > > > - error = xfs_sb_verify(bp, true); > > > > + error = xfs_sb_verify(bp, true, false); > > > > > > > > out_error: > > > > if (error == -EFSCORRUPTED || error == -EFSBADCRC) > > > > @@ -695,7 +707,7 @@ xfs_sb_write_verify( > > > > struct xfs_buf_log_item *bip = bp->b_log_item; > > > > int error; > > > > > > > > - error = xfs_sb_verify(bp, false); > > > > + error = xfs_sb_verify(bp, false, true); > > > > if (error) { > > > > xfs_verifier_error(bp, error, __this_address); > > > > return; > > > > -- > > > > 2.17.1 > > > > > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 17, 2018 at 01:33:01PM -0700, Darrick J. Wong wrote: > On Tue, Jul 17, 2018 at 02:12:53PM -0500, Bill O'Donnell wrote: > > On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote: > > > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote: > > > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > > > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > > > > > Add sanity checks for these parameters. > > > > > > > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > > > > --- > > > > > v2: make extra sanity checks exclusive to writes (allow read) > > > > > > > > > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > > > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > > > index 350119eeaecb..6a98ec68e8ad 100644 > > > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > > > > > xfs_mount_t *mp, > > > > > xfs_sb_t *sbp, > > > > > bool check_inprogress, > > > > > - bool check_version) > > > > > + bool check_version, > > > > > + bool write_flag) > > > > > > > > I notice that check_version and write_flag are always xor -- either > > > > we're reading the sb and set check_version, or we're writing the sb and > > > > set write_flag. Perhaps we can combine these two as write_flag? > > > > > > > > if (check_version) > > > > check version stuff... > > > > > > > > becomes: > > > > > > > > if (!write_flag) > > > > check version stuff... > > > > > > > > and we only have to pass around one flag. > > > > > > I suppose that makes sense, but my notion is that 2 unique flags > > > is preferable for clarity and mutual exclusiveness for anyone doing > > > subsequent patches. > > > > I'm all for simplifying and saving stack space, but is it ok > > to turn a single purpose flag into a dual purpose one? > > That depends on the flag involved -- if they're mutually exclusive, then > I think it's ok to do that, so long as there's a comment nearby to > document the argument semantics. > > In the case of this particular flag (check_version) it is set by the > read verifier so that we reject versions that we don't recognize; it is > not set by the write verifier because we don't change the v5 feature > masks at runtime* and we never write anything if the fs won't mount. > > For write_flag, the read verifier never sets it because we have to be > able to mount the fs in case the log contains an sb with an updated set > of summary counters or for lazysbcount filesystems we'll recalculate the > counter after recovery; and we set write_flag at write time, obviously. > > So having come this far, you could meld them into a single parameter so > long as you note that write_flag == true means that we're writing the fs > and write_flag == false means we want to check the v5 feature flags at > mount time to reject features bits that we don't recognize. > > * Oh, but what about that pesky asterisk? My sense of paranoia wonders > why we don't check the v5 feature flags on write too, just in case > memory gets corrupted. I think the reason is that we don't allow > feature flag changes at runtime, we'll check the changes at ioctl time > if we ever do support runtime feature flag updates, and we implicitly > trust memory not to corrupt memory on us (ha ha ha). > > At this point my tldr opinion is "seems fine to me, let's see if any of > the lurking vacationers have anything to say? We're still ~3 weeks to > the next merge window. My initial reaction was "urk!". I think we should consider putting this check in xfs_sb_verify_write(), not xfs_mount_validate_sb(). We've kinda taken all the mount time checks (which have to be liberal because we can a) be handed non-XFS filesystems, and b) handed filesystems that need recovery) and applied them at write time, too, then special cased the read side primary superblock stuff with a flag. The thing is, write time checks (obviously) need to be stricter than the mount time checks, and we have to check different things. Hence I think we need to do slightly more work here to clean this up. i.e. stop calling xfs_sb_verify() in xfs_sb_write_verify() and open code it instead, then add all these write-only verifier cases into xfs_sb_write_verify(). Similarly, open code xfs_sb_verify() and move all the read-side checks into xfs_sb_read_verify(). Then rename xfs_mount_validate_sb() to xfs_sb_verify_common(), as it only contains the checks that both the read and write side do, and it doesn't need any extra parameters at all... Cheers, Dave.
On Wed, Jul 18, 2018 at 09:26:16AM +1000, Dave Chinner wrote: > On Tue, Jul 17, 2018 at 01:33:01PM -0700, Darrick J. Wong wrote: > > On Tue, Jul 17, 2018 at 02:12:53PM -0500, Bill O'Donnell wrote: > > > On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote: > > > > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote: > > > > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote: > > > > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. > > > > > > Add sanity checks for these parameters. > > > > > > > > > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > > > > > --- > > > > > > v2: make extra sanity checks exclusive to writes (allow read) > > > > > > > > > > > > fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- > > > > > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > > > > index 350119eeaecb..6a98ec68e8ad 100644 > > > > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb( > > > > > > xfs_mount_t *mp, > > > > > > xfs_sb_t *sbp, > > > > > > bool check_inprogress, > > > > > > - bool check_version) > > > > > > + bool check_version, > > > > > > + bool write_flag) > > > > > > > > > > I notice that check_version and write_flag are always xor -- either > > > > > we're reading the sb and set check_version, or we're writing the sb and > > > > > set write_flag. Perhaps we can combine these two as write_flag? > > > > > > > > > > if (check_version) > > > > > check version stuff... > > > > > > > > > > becomes: > > > > > > > > > > if (!write_flag) > > > > > check version stuff... > > > > > > > > > > and we only have to pass around one flag. > > > > > > > > I suppose that makes sense, but my notion is that 2 unique flags > > > > is preferable for clarity and mutual exclusiveness for anyone doing > > > > subsequent patches. > > > > > > I'm all for simplifying and saving stack space, but is it ok > > > to turn a single purpose flag into a dual purpose one? > > > > That depends on the flag involved -- if they're mutually exclusive, then > > I think it's ok to do that, so long as there's a comment nearby to > > document the argument semantics. > > > > In the case of this particular flag (check_version) it is set by the > > read verifier so that we reject versions that we don't recognize; it is > > not set by the write verifier because we don't change the v5 feature > > masks at runtime* and we never write anything if the fs won't mount. > > > > For write_flag, the read verifier never sets it because we have to be > > able to mount the fs in case the log contains an sb with an updated set > > of summary counters or for lazysbcount filesystems we'll recalculate the > > counter after recovery; and we set write_flag at write time, obviously. > > > > So having come this far, you could meld them into a single parameter so > > long as you note that write_flag == true means that we're writing the fs > > and write_flag == false means we want to check the v5 feature flags at > > mount time to reject features bits that we don't recognize. > > > > * Oh, but what about that pesky asterisk? My sense of paranoia wonders > > why we don't check the v5 feature flags on write too, just in case > > memory gets corrupted. I think the reason is that we don't allow > > feature flag changes at runtime, we'll check the changes at ioctl time > > if we ever do support runtime feature flag updates, and we implicitly > > trust memory not to corrupt memory on us (ha ha ha). > > > > At this point my tldr opinion is "seems fine to me, let's see if any of > > the lurking vacationers have anything to say? We're still ~3 weeks to > > the next merge window. > > My initial reaction was "urk!". I think we should consider putting > this check in xfs_sb_verify_write(), not xfs_mount_validate_sb(). > We've kinda taken all the mount time checks (which have to be > liberal because we can a) be handed non-XFS filesystems, and b) > handed filesystems that need recovery) and applied them at write > time, too, then special cased the read side primary superblock stuff > with a flag. > > The thing is, write time checks (obviously) need to be stricter than > the mount time checks, and we have to check different things. Hence > I think we need to do slightly more work here to clean this up. i.e. > stop calling xfs_sb_verify() in xfs_sb_write_verify() and open code > it instead, then add all these write-only verifier cases into > xfs_sb_write_verify(). > > Similarly, open code xfs_sb_verify() and move all the read-side > checks into xfs_sb_read_verify(). > > Then rename xfs_mount_validate_sb() to xfs_sb_verify_common(), as it > only contains the checks that both the read and write side do, and > it doesn't need any extra parameters at all... Ok, IIUC, you suggest a partial gutting of xfs_mount_validate_sb(), and open coding all the write and read checks in xfs_sb_write_verify() and xfs_sb_read_verify(), respectively (keeping common checks in xfs_mount_validate_sb(). What's not intuitively obvious to me: which sanity checks in the current xfs_mount_validate_sb() are common to read and write (and which are exlusive). Thanks- Bill > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c index 350119eeaecb..6a98ec68e8ad 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -104,7 +104,8 @@ xfs_mount_validate_sb( xfs_mount_t *mp, xfs_sb_t *sbp, bool check_inprogress, - bool check_version) + bool check_version, + bool write_flag) { uint32_t agcount = 0; uint32_t rem; @@ -266,6 +267,15 @@ xfs_mount_validate_sb( return -EFSCORRUPTED; } + /* Additional sb sanity checks for writes */ + if (write_flag) { + if (sbp->sb_fdblocks > sbp->sb_dblocks || + sbp->sb_ifree > sbp->sb_icount) { + xfs_notice(mp, "SB sanity check failed"); + return -EFSCORRUPTED; + } + } + if (sbp->sb_unit) { if (!xfs_sb_version_hasdalign(sbp) || sbp->sb_unit > sbp->sb_width || @@ -599,7 +609,9 @@ xfs_sb_to_disk( static int xfs_sb_verify( struct xfs_buf *bp, - bool check_version) + bool check_version, + bool write_flag) + { struct xfs_mount *mp = bp->b_target->bt_mount; struct xfs_sb sb; @@ -616,7 +628,7 @@ xfs_sb_verify( */ return xfs_mount_validate_sb(mp, &sb, bp->b_maps[0].bm_bn == XFS_SB_DADDR, - check_version); + check_version, write_flag); } /* @@ -657,7 +669,7 @@ xfs_sb_read_verify( } } } - error = xfs_sb_verify(bp, true); + error = xfs_sb_verify(bp, true, false); out_error: if (error == -EFSCORRUPTED || error == -EFSBADCRC) @@ -695,7 +707,7 @@ xfs_sb_write_verify( struct xfs_buf_log_item *bip = bp->b_log_item; int error; - error = xfs_sb_verify(bp, false); + error = xfs_sb_verify(bp, false, true); if (error) { xfs_verifier_error(bp, error, __this_address); return;
Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. Add sanity checks for these parameters. Signed-off-by: Bill O'Donnell <billodo@redhat.com> --- v2: make extra sanity checks exclusive to writes (allow read) fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)