libxfs: add more bounds checking to sb sanity checks
PATCH 1/1 libxfs: add more bounds checking to sb sanity checks
Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. Add sanity checks for these parameters.
--- fs/xfs/libxfs/xfs_sb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_sb.c +3 -1
@@ -261,7 +261,9 @@ xfs_mount_validate_sb( sbp->sb_dblocks == 0 || sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp) || sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp) || - sbp->sb_shared_vn != 0)) { + sbp->sb_shared_vn != 0 || + sbp->sb_fdblocks > sbp->sb_dblocks || + sbp->sb_ifree > sbp->sb_icount)) {xfs_notice (mp,"SB sanity check failed" );return -EFSCORRUPTED ; }
Signature · 1 line
2.17.1
Quoted 13 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>--- fs/xfs/libxfs/xfs_sb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)fs/xfs/libxfs/xfs_sb.c +0
-0
fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 266
Quoted 7 lines
fs/xfs/libxfs/xfs_sb.c +3
-1@@ -261,7 +261,9 @@ xfs_mount_validate_sb( sbp->sb_dblocks ==0 || sbp->sb_dblocks >XFS_MAX_DBLOCKS (sbp) || sbp->sb_dblocks <XFS_MIN_DBLOCKS (sbp) || - sbp->sb_shared_vn != 0)) { + sbp->sb_shared_vn !=0 || + sbp->sb_fdblocks > sbp->sb_dblocks || + sbp->sb_ifree > sbp->sb_icount)) {
Hmm. On its face this seems reasonable for the superblock verifier, but then I started wondering, since these are /summary/ counters.
If the free counts are off by this much, the admin won't be able to mount the fs, and xfs_repair is the only other tool that can fix the summary counts. However, if the log is dirty, the mount won't succeed to recover the fs, which is too bad since we can reinitialize the summary counts after log recovery. xfs_repair -L will be the only way out, which will wreak havoc on the filesystem from discarding the log contents.
So, would it be preferable to split this into two parts? For example, have this as a corruption check in _sb_write_verify to prevent us from writing out garbage counters and a clamp in _reinit_percpu_counters so that we never present ridiculous free counts to users?
(Does any of this make sense with !haslazysbcount filesystems?)
Bonus question: What about checking frextents/rextents?
--D
Quoted 10 lines
fs/xfs/libxfs/xfs_sb.c +0
-0xfs_notice (mp,"SB sanity check failed" );return -EFSCORRUPTED ; }Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · Darrick J. Wong
Quoted 8 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>--- fs/xfs/libxfs/xfs_sb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 15 lines
fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 266
Quoted 12 lines
fs/xfs/libxfs/xfs_sb.c +3
-1@@ -261,7 +261,9 @@ xfs_mount_validate_sb( sbp->sb_dblocks ==0 || sbp->sb_dblocks >XFS_MAX_DBLOCKS (sbp) || sbp->sb_dblocks <XFS_MIN_DBLOCKS (sbp) || - sbp->sb_shared_vn != 0)) { + sbp->sb_shared_vn !=0 || + sbp->sb_fdblocks > sbp->sb_dblocks || + sbp->sb_ifree > sbp->sb_icount)) {Hmm. On its face this seems reasonable for the superblock verifier, but then I started wondering, since these are /summary/ counters.
FWIW, I'm proposing a rudimentary bounds check to prevent this sort of issue from even happening in the first place:
https://www.spinics.net/lists/linux-xfs/msg20592.html
Quoted 8 lines
If the free counts are off by this much, the admin won't be able to mount the fs, and
xfs_repairis the only other tool that can fix the summary counts. However, if the log is dirty, the mount won't succeed to recover the fs, which is too bad since we can reinitialize the summary counts after log recovery.xfs_repair-L will be the only way out, which will wreak havoc on the filesystem from discarding the log contents.
agreed, but again, I want to prevent the aforementioned use case where corruption gets introduced.
Quoted 9 lines
So, would it be preferable to split this into two parts? For example, have this as a corruption check in
_sb_write_verifyto prevent us from writing out garbage counters and a clamp in_reinit_percpu_countersso that we never present ridiculous free counts to users?(Does any of this make sense with !haslazysbcount filesystems?)
Bonus question: What about checking frextents/rextents?
Hrmm, perhaps. It should definitely be considered.
Thanks-
Bill
Quoted 12 lines
--D
Quoted 10 lines
fs/xfs/libxfs/xfs_sb.c +0
-0xfs_notice (mp,"SB sanity check failed" );return -EFSCORRUPTED ; }Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 25 lines · Darrick J. Wong
Quoted 13 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>--- fs/xfs/libxfs/xfs_sb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)fs/xfs/libxfs/xfs_sb.c +0
-0fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 266
Quoted 7 lines
fs/xfs/libxfs/xfs_sb.c +3
-1@@ -261,7 +261,9 @@ xfs_mount_validate_sb( sbp->sb_dblocks ==0 || sbp->sb_dblocks >XFS_MAX_DBLOCKS (sbp) || sbp->sb_dblocks <XFS_MIN_DBLOCKS (sbp) || - sbp->sb_shared_vn != 0)) { + sbp->sb_shared_vn !=0 || + sbp->sb_fdblocks > sbp->sb_dblocks || + sbp->sb_ifree > sbp->sb_icount)) {Hmm. On its face this seems reasonable for the superblock verifier, but then I started wondering, since these are /summary/ counters.
Quoted 7 lines
If the free counts are off by this much, the admin won't be able to mount the fs, and
xfs_repairis the only other tool that can fix the summary counts. However, if the log is dirty, the mount won't succeed to recover the fs, which is too bad since we can reinitialize the summary counts after log recovery.xfs_repair-L will be the only way out, which will wreak havoc on the filesystem from discarding the log contents.
Yup, that's why I said "catch this on /write/", not "always reject bad counter values".
i.e. we should never be writing a bad value, but we most definitely need to be able to mount the filesystem to reconstruct them.
Quoted 3 lines
So, would it be preferable to split this into two parts? For example, have this as a corruption check in
_sb_write_verifyto prevent us from writing out garbage counters
yes.
Quoted 2 lines
and a clamp in
_reinit_percpu_countersso that we never present ridiculous free counts to users?
percpu_counter_{read,sum}_positive() should be used for anything that is userspace facing. xfs_fs_counts() gets this right, but xfs_fs_statfs() doesn't - it should use percpu_counter_sum_positive().
Quoted 1 line
(Does any of this make sense with !haslazysbcount filesystems?)
Same thing - we can't verify the counters on read until after log recovery as all the changes are journalled.
Quoted 1 line
Bonus question: What about checking frextents/rextents?
Same as !lazycount - all changes are journalled.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 46 lines · Dave Chinner
Quoted 25 lines · Darrick J. Wong
Quoted 13 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>--- fs/xfs/libxfs/xfs_sb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)fs/xfs/libxfs/xfs_sb.c +0
-0fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 266
Quoted 7 lines
fs/xfs/libxfs/xfs_sb.c +3
-1@@ -261,7 +261,9 @@ xfs_mount_validate_sb( sbp->sb_dblocks ==0 || sbp->sb_dblocks >XFS_MAX_DBLOCKS (sbp) || sbp->sb_dblocks <XFS_MIN_DBLOCKS (sbp) || - sbp->sb_shared_vn != 0)) { + sbp->sb_shared_vn !=0 || + sbp->sb_fdblocks > sbp->sb_dblocks || + sbp->sb_ifree > sbp->sb_icount)) {Hmm. On its face this seems reasonable for the superblock verifier, but then I started wondering, since these are /summary/ counters.
Quoted 7 lines
If the free counts are off by this much, the admin won't be able to mount the fs, and
xfs_repairis the only other tool that can fix the summary counts. However, if the log is dirty, the mount won't succeed to recover the fs, which is too bad since we can reinitialize the summary counts after log recovery.xfs_repair-L will be the only way out, which will wreak havoc on the filesystem from discarding the log contents.Yup, that's why I said "catch this on /write/", not "always reject bad counter values".
i.e. we should never be writing a bad value, but we most definitely need to be able to mount the filesystem to reconstruct them.
Quoted 3 lines
So, would it be preferable to split this into two parts? For example, have this as a corruption check in
_sb_write_verifyto prevent us from writing out garbage countersyes.
Quoted 7 lines
Quoted 2 lines
and a clamp in
_reinit_percpu_countersso that we never present ridiculous free counts to users?percpu_counter_{read,sum}_positive() should be used for anything that is userspace facing.
xfs_fs_counts()gets this right, butxfs_fs_statfs()doesn't - it should usepercpu_counter_sum_positive().
I don't think that will solve this problem -- although sb_fdblocks is larger than sb_dblocks, sd_fdblocks is not so insanely large that percpu_counter_{read,sum} return negative values; returning to Eric's analysis of the original complaint:
Quoted 8 lines
sb_fdblocks4461713825, counted 166746529
- found root inode chunkthat
sb_fdblocksreally is ~17T which indicates the problem really is on disk.4461713825
100001001111100000101100110100001
^-- bit 32 of a signed 64-bit quantity
Quoted 2 lines
166746529
1001111100000101100110100001
So we still need a separate sb_fdblocks <= sb_dblocks clamp and/or forced recalculation somewhere.
I agree that _fs_statfs should only return positive free blocks to avoid reporting total garbage to userspace, but that's not the problme here. I'll toss that onto my pile for 4.19 stuff.
Quoted 8 lines
Quoted 1 line
(Does any of this make sense with !haslazysbcount filesystems?)
Same thing - we can't verify the counters on read until after log recovery as all the changes are journalled.
Quoted 1 line
Bonus question: What about checking frextents/rextents?
Same as !lazycount - all changes are journalled.
Ok.
--D
Quoted 11 lines
Cheers,
Dave.
Signature · 6 lines
Dave Chinner [email protected] -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 libxfs: add more bounds checking to sb sanity checks
In reply to Bill O'DonnellCurrent sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. Add sanity checks for these parameters.
---
v2: make extra sanity checks exclusive to writes (allow read)
fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
fs/xfs/libxfs/xfs_sb.c +17 -5
@@ -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 ;
Signature · 1 line
2.17.1
Quoted 6 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
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,
Quoted 79 lines
fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)fs/xfs/libxfs/xfs_sb.c +17
-5@@ -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 ;Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Carlos
Quoted 15 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v2: make extra sanity checks exclusive to writes (allow read)fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)fs/xfs/libxfs/xfs_sb.c +0
-0
fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 108
Quoted 6 lines
fs/xfs/libxfs/xfs_sb.c +2
-1@@ -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.
fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 273
Quoted 11 lines
fs/xfs/libxfs/xfs_sb.c +4
-0{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
Quoted 53 lines
fs/xfs/libxfs/xfs_sb.c +11
-4+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 ;Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 30 lines · Darrick J. Wong
Quoted 15 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v2: make extra sanity checks exclusive to writes (allow read)fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)fs/xfs/libxfs/xfs_sb.c +0
-0fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 108
Quoted 6 lines
fs/xfs/libxfs/xfs_sb.c +2
-1@@ -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_versionandwrite_flagare always xor -- either we're reading the sb and setcheck_version, or we're writing the sb and setwrite_flag. Perhaps we can combine these two aswrite_flag?if (
check_version)
check version stuff...
Quoted 6 lines
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.
Quoted 8 lines
Quoted 8 lines
fs/xfs/libxfs/xfs_sb.c +0
-0{uint32_t agcount =0 ;uint32_t rem;@@ -266,6 +267,15 @@ xfs_mount_validate_sb(return -EFSCORRUPTED ; }
Quoted 9 lines
fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 273
Quoted 4 lines
fs/xfs/libxfs/xfs_sb.c +4
-0+/* 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
Quoted 56 lines
--D
Quoted 53 lines
fs/xfs/libxfs/xfs_sb.c +11
-4+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 ;Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 33 lines · Bill O'Donnell
Quoted 32 lines · Darrick J. Wong
Quoted 15 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v2: make extra sanity checks exclusive to writes (allow read)fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)fs/xfs/libxfs/xfs_sb.c +0
-0fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 108
Quoted 6 lines
fs/xfs/libxfs/xfs_sb.c +2
-1@@ -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_versionandwrite_flagare always xor -- either we're reading the sb and setcheck_version, or we're writing the sb and setwrite_flag. Perhaps we can combine these two aswrite_flag?if (
check_version)
check version stuff...becomes:
Quoted 8 lines
Quoted 4 lines
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?
Quoted 82 lines
Quoted 9 lines
Quoted 9 lines
fs/xfs/libxfs/xfs_sb.c +0
-0{uint32_t agcount =0 ;uint32_t rem;@@ -266,6 +267,15 @@ xfs_mount_validate_sb(return -EFSCORRUPTED ; }Quoted 9 lines
fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 273
Quoted 4 lines
fs/xfs/libxfs/xfs_sb.c +4
-0+/* 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!
-BillQuoted 52 lines
--D
Quoted 49 lines
fs/xfs/libxfs/xfs_sb.c +11
-4+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 ;Signature · 1 line
2.17.1Quoted 4 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 37 lines · Bill O'Donnell
Quoted 36 lines · Bill O'Donnell
Quoted 35 lines · Darrick J. Wong
Quoted 15 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v2: make extra sanity checks exclusive to writes (allow read)fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)fs/xfs/libxfs/xfs_sb.c +0
-0fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 108
Quoted 6 lines
fs/xfs/libxfs/xfs_sb.c +2
-1@@ -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_versionandwrite_flagare always xor -- either we're reading the sb and setcheck_version, or we're writing the sb and setwrite_flag. Perhaps we can combine these two aswrite_flag?if (
check_version)
check version stuff...becomes:
if (!write_flag)
check version stuff...
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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
Quoted 86 lines
Quoted 74 lines
Quoted 9 lines
Quoted 9 lines
fs/xfs/libxfs/xfs_sb.c +0
-0{uint32_t agcount =0 ;uint32_t rem;@@ -266,6 +267,15 @@ xfs_mount_validate_sb(return -EFSCORRUPTED ; }Quoted 9 lines
fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 273
Quoted 4 lines
fs/xfs/libxfs/xfs_sb.c +4
-0+/* 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!
-BillQuoted 52 lines
--D
Quoted 49 lines
fs/xfs/libxfs/xfs_sb.c +11
-4+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 ;Signature · 1 line
2.17.1Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 73 lines · Darrick J. Wong
Quoted 37 lines · Bill O'Donnell
Quoted 36 lines · Bill O'Donnell
Quoted 35 lines · Darrick J. Wong
Quoted 15 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v2: make extra sanity checks exclusive to writes (allow read)fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)fs/xfs/libxfs/xfs_sb.c +0
-0fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 108
Quoted 6 lines
fs/xfs/libxfs/xfs_sb.c +2
-1@@ -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_versionandwrite_flagare always xor -- either we're reading the sb and setcheck_version, or we're writing the sb and setwrite_flag. Perhaps we can combine these two aswrite_flag?if (
check_version)
check version stuff...becomes:
if (!write_flag)
check version stuff...Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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 setwrite_flagat 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 andwrite_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).
Quoted 3 lines
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.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 94 lines · Dave Chinner
Quoted 73 lines · Darrick J. Wong
Quoted 37 lines · Bill O'Donnell
Quoted 36 lines · Bill O'Donnell
Quoted 35 lines · Darrick J. Wong
Quoted 15 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v2: make extra sanity checks exclusive to writes (allow read)fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)fs/xfs/libxfs/xfs_sb.c +0
-0fs/xfs/libxfs/xfs_sb.c xfs_mount_validate_sb( · line 108
Quoted 6 lines
fs/xfs/libxfs/xfs_sb.c +2
-1@@ -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_versionandwrite_flagare always xor -- either we're reading the sb and setcheck_version, or we're writing the sb and setwrite_flag. Perhaps we can combine these two aswrite_flag?if (
check_version)
check version stuff...becomes:
if (!write_flag)
check version stuff...Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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 setwrite_flagat 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 andwrite_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).
Quoted 3 lines
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(), notxfs_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()inxfs_sb_write_verify()and open code it instead, then add all these write-only verifier cases intoxfs_sb_write_verify().
Quoted 6 lines
Similarly, open code
xfs_sb_verify()and move all the read-side checks intoxfs_sb_read_verify().Then rename
xfs_mount_validate_sb()toxfs_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
Quoted 7 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH v3 1/1 libxfs: add more bounds checking to sb sanity checks
In reply to Bill O'DonnellCurrent sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. Add sanity checks for these parameters.
---
v3: eliminate need for additional write_flag, doing those
unique checks in xfs_sb_write_verify()
v2: make extra sanity checks exclusive to writes
fs/xfs/libxfs/xfs_sb.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-)
fs/xfs/libxfs/xfs_sb.c +28 -10
@@ -599,22 +599,16 @@ xfs_sb_to_disk(static int xfs_sb_verify (struct xfs_buf *bp, +struct xfs_sb *sb,bool check_version) {struct xfs_mount *mp = bp->b_target->bt_mount; -struct xfs_sb sb; - -/* -* Use call variant which doesn't convert quota flags from disk -* format, because xfs_mount_validate_sb checks the on-disk flags. -*/ -__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false);/* * Only check the in progress field for the primary superblock as * mkfs.xfs doesn't clear it from secondary superblocks. */ -return xfs_mount_validate_sb (mp, &sb, +return xfs_mount_validate_sb (mp, sb, bp->b_maps[0 ].bm_bn ==XFS_SB_DADDR , check_version); }
@@ -637,6 +631,7 @@ xfs_sb_read_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_dsb *dsb =XFS_BUF_TO_SBP (bp); +struct xfs_sb sb;int error;/*
@@ -657,7 +652,13 @@ xfs_sb_read_verify( } } } - error =xfs_sb_verify (bp, true); + +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + error =xfs_sb_verify (bp, &sb, true); out_error:if (error == -EFSCORRUPTED || error == -EFSBADCRC )
@@ -693,9 +694,26 @@ xfs_sb_write_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_buf_log_item *bip = bp->b_log_item; +struct xfs_sb sb;int error; - error =xfs_sb_verify (bp, false); +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + + error =xfs_sb_verify (bp, &sb, false); + +/* Additional sb sanity checks for writes */ +if (!error) { +if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + } + } +if (error) {xfs_verifier_error (bp, error, __this_address);return ;
Signature · 1 line
2.17.1
Quoted 79 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v3: eliminate need for additionalwrite_flag, doing those
unique checks inxfs_sb_write_verify()
v2: make extra sanity checks exclusive to writesfs/xfs/libxfs/xfs_sb.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-)fs/xfs/libxfs/xfs_sb.c +17
-10@@ -599,22 +599,16 @@ xfs_sb_to_disk(static int xfs_sb_verify (struct xfs_buf *bp, +struct xfs_sb *sb,bool check_version) {struct xfs_mount *mp = bp->b_target->bt_mount; -struct xfs_sb sb; - -/* -* Use call variant which doesn't convert quota flags from disk -* format, because xfs_mount_validate_sb checks the on-disk flags. -*/ -__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false);/* * Only check the in progress field for the primary superblock as * mkfs.xfs doesn't clear it from secondary superblocks. */ -return xfs_mount_validate_sb (mp, &sb, +return xfs_mount_validate_sb (mp, sb, bp->b_maps[0 ].bm_bn ==XFS_SB_DADDR , check_version); }@@ -637,6 +631,7 @@ xfs_sb_read_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_dsb *dsb =XFS_BUF_TO_SBP (bp); +struct xfs_sb sb;int error;/* @@ -657,7 +652,13 @@ xfs_sb_read_verify( } } } - error =xfs_sb_verify (bp, true); + +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + error =xfs_sb_verify (bp, &sb, true); out_error:if (error == -EFSCORRUPTED || error == -EFSBADCRC )@@ -693,9 +694,26 @@ xfs_sb_write_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_buf_log_item *bip = bp->b_log_item; +struct xfs_sb sb;int error; - error =xfs_sb_verify (bp, false); +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); +
fs/xfs/libxfs/xfs_sb.c xfs_sb_write_verify( · line 715
Quoted 10 lines
fs/xfs/libxfs/xfs_sb.c +10
-0+ error =xfs_sb_verify (bp, &sb, false); + +/* Additional sb sanity checks for writes */ +if (!error) { +if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + } + }
On the off chance that some day we add more write-time checks, could you please structure this the usual way?
error = xfs_sb_verify(bp, &sb, false);
if (error)
goto err;
if (sb.sb_fdblocks > sb.sb_dblocks || sb.sb_ifree > sb.sb_icount) {
xfs_notice(mp, "SB summary counter sanity check failed");
error = -EFSCORRUPTED;
goto err;
}err:
if (error) {
xfs_verifier_error(bp, error, __this_address);
return;
}
}Other than that, this looks ok to me.
--D
Quoted 11 lines
fs/xfs/libxfs/xfs_sb.c +1
-0+if (error) {xfs_verifier_error (bp, error, __this_address);return ;Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 85 lines · Darrick J. Wong
Quoted 84 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v3: eliminate need for additionalwrite_flag, doing those
unique checks inxfs_sb_write_verify()
v2: make extra sanity checks exclusive to writesfs/xfs/libxfs/xfs_sb.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-)fs/xfs/libxfs/xfs_sb.c +22
-10@@ -599,22 +599,16 @@ xfs_sb_to_disk(static int xfs_sb_verify (struct xfs_buf *bp, +struct xfs_sb *sb,bool check_version) {struct xfs_mount *mp = bp->b_target->bt_mount; -struct xfs_sb sb; - -/* -* Use call variant which doesn't convert quota flags from disk -* format, because xfs_mount_validate_sb checks the on-disk flags. -*/ -__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false);/* * Only check the in progress field for the primary superblock as * mkfs.xfs doesn't clear it from secondary superblocks. */ -return xfs_mount_validate_sb (mp, &sb, +return xfs_mount_validate_sb (mp, sb, bp->b_maps[0 ].bm_bn ==XFS_SB_DADDR , check_version); }@@ -637,6 +631,7 @@ xfs_sb_read_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_dsb *dsb =XFS_BUF_TO_SBP (bp); +struct xfs_sb sb;int error;/* @@ -657,7 +652,13 @@ xfs_sb_read_verify( } } } - error =xfs_sb_verify (bp, true); + +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + error =xfs_sb_verify (bp, &sb, true); out_error:if (error == -EFSCORRUPTED || error == -EFSBADCRC )@@ -693,9 +694,26 @@ xfs_sb_write_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_buf_log_item *bip = bp->b_log_item; +struct xfs_sb sb;int error; - error =xfs_sb_verify (bp, false); +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + + error =xfs_sb_verify (bp, &sb, false); + +/* Additional sb sanity checks for writes */ +if (!error) { + if (sb.sb_fdblocks > sb.sb_dblocks ||
Quoted 8 lines
fs/xfs/libxfs/xfs_sb.c xfs_sb_write_verify( · line 715
Quoted 5 lines
fs/xfs/libxfs/xfs_sb.c +5
-0+ sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + } + }On the off chance that some day we add more write-time checks, could you please structure this the usual way?
ah, good idea... will do!
Thanks-
Bill
Quoted 33 lines
error = xfs_sb_verify(bp, &sb, false); if (error) goto err; if (sb.sb_fdblocks > sb.sb_dblocks || sb.sb_ifree > sb.sb_icount) { xfs_notice(mp, "SB summary counter sanity check failed"); error = -EFSCORRUPTED; goto err; }err:
if (error) { xfs_verifier_error(bp, error, __this_address); return; } }Other than that, this looks ok to me.
--D
Quoted 11 lines
fs/xfs/libxfs/xfs_sb.c +1
-0+if (error) {xfs_verifier_error (bp, error, __this_address);return ;Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
fs/xfs/libxfs/xfs_sb.c xfs_sb_write_verify( · line 715
Quoted 8 lines · Bill O'Donnell
fs/xfs/libxfs/xfs_sb.c +8
-0+/* Additional sb sanity checks for writes */ +if (!error) { +if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + } + }
I had kind of had it in my head that Dave suggested testing not only sb_fdblocks & sb_ifree but also validating sb_icount against sb_dblocks ... would that make sense? something like:
+ if (sb.sb_fdblocks > sb.sb_dblocks ||
+ sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks) ||
+ sb.sb_ifree > sb.sb_icount) {
+ xfs_notice(mp, "SB sanity check failed");
because all 3 go into the statfs calculations which went wonky in the original report? (xfs_sb_verify has done some sanity checks on sb_inopblock by the time we get here.)
Also, a comment about why these checks are only for write, and are not simply in xfs_mount_validate_sb() would be good, since that obviously wasn't obvious to me at first. o_O :)
-Eric
Quoted 16 lines · Eric Sandeen
fs/xfs/libxfs/xfs_sb.c xfs_sb_write_verify( · line 715
Quoted 8 lines · Bill O'Donnell
fs/xfs/libxfs/xfs_sb.c +8
-0+/* Additional sb sanity checks for writes */ +if (!error) { +if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + } + }I had kind of had it in my head that Dave suggested testing not only
sb_fdblocks&sb_ifreebut also validatingsb_icountagainstsb_dblocks... would that make sense? something like:
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks) ||
That would make sense, but perhaps we should have a xfs_verify_icount instead of open-coding a 64-bit division? :)
Granted, I /was/ planning to add all that as part of fs summary counter scrubbing next cycle.
--D
Quoted 16 lines
fs/xfs/libxfs/xfs_sb.c +2
-0+ sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" );because all 3 go into the statfs calculations which went wonky in the original report? (
xfs_sb_verifyhas done some sanity checks onsb_inopblockby the time we get here.)Also, a comment about why these checks are only for write, and are not simply in
xfs_mount_validate_sb()would be good, since that obviously wasn't obvious to me at first.o_O:)-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v4 1/1 libxfs: add more bounds checking to sb sanity checks
In reply to Bill O'DonnellCurrent sb verifier doesn't check bounds on sb_fdblocks and sb_ifree. Add sanity checks for these parameters.
---
v4: adjust control flow to standard error checking. Add
more descriptive comment. Add validation for sb_icount.
v3: eliminate need for additional write_flag, doing those
unique checks in xfs_sb_write_verify()
v2: make extra sanity checks exclusive to writes
fs/xfs/libxfs/xfs_sb.c | 46 +++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 10 deletions(-)
fs/xfs/libxfs/xfs_sb.c +36 -10
@@ -599,22 +599,16 @@ xfs_sb_to_disk(static int xfs_sb_verify (struct xfs_buf *bp, +struct xfs_sb *sb,bool check_version) {struct xfs_mount *mp = bp->b_target->bt_mount; -struct xfs_sb sb; - -/* -* Use call variant which doesn't convert quota flags from disk -* format, because xfs_mount_validate_sb checks the on-disk flags. -*/ -__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false);/* * Only check the in progress field for the primary superblock as * mkfs.xfs doesn't clear it from secondary superblocks. */ -return xfs_mount_validate_sb (mp, &sb, +return xfs_mount_validate_sb (mp, sb, bp->b_maps[0 ].bm_bn ==XFS_SB_DADDR , check_version); }
@@ -637,6 +631,7 @@ xfs_sb_read_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_dsb *dsb =XFS_BUF_TO_SBP (bp); +struct xfs_sb sb;int error;/*
@@ -657,7 +652,13 @@ xfs_sb_read_verify( } } } - error =xfs_sb_verify (bp, true); + +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + error =xfs_sb_verify (bp, &sb, true); out_error:if (error == -EFSCORRUPTED || error == -EFSBADCRC )
@@ -693,9 +694,34 @@ xfs_sb_write_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_buf_log_item *bip = bp->b_log_item; +struct xfs_sb sb;int error; - error =xfs_sb_verify (bp, false); +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + + error =xfs_sb_verify (bp, &sb, false); +if (error) + goto err; + +/* +* Carry out additional sb sanity checks exclusively for writes. +* We don't do these checks for reads, since faulty parameters could +* already be on disk, and we shouldn't preclude reads for those +* cases. +*/ +if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks || + sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + goto err; + } + +err:if (error) {xfs_verifier_error (bp, error, __this_address);return ;
Signature · 1 line
2.17.1
Quoted 83 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v4: adjust control flow to standard error checking. Add
more descriptive comment. Add validation forsb_icount.
v3: eliminate need for additionalwrite_flag, doing those
unique checks inxfs_sb_write_verify()
v2: make extra sanity checks exclusive to writesfs/xfs/libxfs/xfs_sb.c | 46 +++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 10 deletions(-)fs/xfs/libxfs/xfs_sb.c +19
-10@@ -599,22 +599,16 @@ xfs_sb_to_disk(static int xfs_sb_verify (struct xfs_buf *bp, +struct xfs_sb *sb,bool check_version) {struct xfs_mount *mp = bp->b_target->bt_mount; -struct xfs_sb sb; - -/* -* Use call variant which doesn't convert quota flags from disk -* format, because xfs_mount_validate_sb checks the on-disk flags. -*/ -__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false);/* * Only check the in progress field for the primary superblock as * mkfs.xfs doesn't clear it from secondary superblocks. */ -return xfs_mount_validate_sb (mp, &sb, +return xfs_mount_validate_sb (mp, sb, bp->b_maps[0 ].bm_bn ==XFS_SB_DADDR , check_version); }@@ -637,6 +631,7 @@ xfs_sb_read_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_dsb *dsb =XFS_BUF_TO_SBP (bp); +struct xfs_sb sb;int error;/* @@ -657,7 +652,13 @@ xfs_sb_read_verify( } } } - error =xfs_sb_verify (bp, true); + +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + error =xfs_sb_verify (bp, &sb, true); out_error:if (error == -EFSCORRUPTED || error == -EFSBADCRC )@@ -693,9 +694,34 @@ xfs_sb_write_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_buf_log_item *bip = bp->b_log_item; +struct xfs_sb sb;int error; - error =xfs_sb_verify (bp, false); +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + + error =xfs_sb_verify (bp, &sb, false); +if (error)
fs/xfs/libxfs/xfs_sb.c xfs_sb_write_verify( · line 717
Quoted 10 lines
fs/xfs/libxfs/xfs_sb.c +10
-0+ goto err; + +/* +* Carry out additional sb sanity checks exclusively for writes. +* We don't do these checks for reads, since faulty parameters could +* already be on disk, and we shouldn't preclude reads for those +* cases. +*/ + if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks ||
This is a 64-bit division, which won't work on 32-bit builds.
That said, I also wrote a xfs_verify_icount function for the fs summary scrub patch, so perhaps we should collaborate to land both of these? Send a v5 without this test and I'll immediately send a patch adding both the _verify_icount and putting it to use here.
Actually, I could just modify your patch, add mine, and send both of them. That'd be easier...
--D
Quoted 17 lines
fs/xfs/libxfs/xfs_sb.c +7
-0+ sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + goto err; + } + +err:if (error) {xfs_verifier_error (bp, error, __this_address);return ;Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 97 lines · Darrick J. Wong
Quoted 83 lines · Bill O'Donnell
Current sb verifier doesn't check bounds on
sb_fdblocksandsb_ifree. Add sanity checks for these parameters.Signed-off-by Bill O'Donnell <[email protected]>---
v4: adjust control flow to standard error checking. Add
more descriptive comment. Add validation forsb_icount.
v3: eliminate need for additionalwrite_flag, doing those
unique checks inxfs_sb_write_verify()
v2: make extra sanity checks exclusive to writesfs/xfs/libxfs/xfs_sb.c | 46 +++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 10 deletions(-)fs/xfs/libxfs/xfs_sb.c +19
-10@@ -599,22 +599,16 @@ xfs_sb_to_disk(static int xfs_sb_verify (struct xfs_buf *bp, +struct xfs_sb *sb,bool check_version) {struct xfs_mount *mp = bp->b_target->bt_mount; -struct xfs_sb sb; - -/* -* Use call variant which doesn't convert quota flags from disk -* format, because xfs_mount_validate_sb checks the on-disk flags. -*/ -__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false);/* * Only check the in progress field for the primary superblock as * mkfs.xfs doesn't clear it from secondary superblocks. */ -return xfs_mount_validate_sb (mp, &sb, +return xfs_mount_validate_sb (mp, sb, bp->b_maps[0 ].bm_bn ==XFS_SB_DADDR , check_version); }@@ -637,6 +631,7 @@ xfs_sb_read_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_dsb *dsb =XFS_BUF_TO_SBP (bp); +struct xfs_sb sb;int error;/* @@ -657,7 +652,13 @@ xfs_sb_read_verify( } } } - error =xfs_sb_verify (bp, true); + +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + error =xfs_sb_verify (bp, &sb, true); out_error:if (error == -EFSCORRUPTED || error == -EFSBADCRC )@@ -693,9 +694,34 @@ xfs_sb_write_verify( {struct xfs_mount *mp = bp->b_target->bt_mount;struct xfs_buf_log_item *bip = bp->b_log_item; +struct xfs_sb sb;int error; - error =xfs_sb_verify (bp, false); +/* +* Use call variant which doesn't convert quota flags from disk +* format, because xfs_mount_validate_sb checks the on-disk flags. +*/ +__xfs_sb_from_disk (&sb,XFS_BUF_TO_SBP (bp), false); + + error =xfs_sb_verify (bp, &sb, false); +if (error)fs/xfs/libxfs/xfs_sb.c xfs_sb_write_verify( · line 717
Quoted 10 lines
fs/xfs/libxfs/xfs_sb.c +10
-0+ goto err; + +/* +* Carry out additional sb sanity checks exclusively for writes. +* We don't do these checks for reads, since faulty parameters could +* already be on disk, and we shouldn't preclude reads for those +* cases. +*/ + if (sb.sb_fdblocks > sb.sb_dblocks || + sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks ||This is a 64-bit division, which won't work on 32-bit builds.
Quoted 7 lines
That said, I also wrote a
xfs_verify_icountfunction for the fs summary scrub patch, so perhaps we should collaborate to land both of these? Send a v5 without this test and I'll immediately send a patch adding both the_verify_icountand putting it to use here.Actually, I could just modify your patch, add mine, and send both of them. That'd be easier...
That sounds fine. Thanks!
Bill
Quoted 20 lines
--D
Quoted 17 lines
fs/xfs/libxfs/xfs_sb.c +7
-0+ sb.sb_ifree > sb.sb_icount) { +xfs_notice (mp,"SB sanity check failed" ); + error = -EFSCORRUPTED ; + goto err; + } + +err:if (error) {xfs_verifier_error (bp, error, __this_address);return ;Signature · 6 lines
2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html