| Message ID | 20180313150637.3913-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13.03.2018 17:06, Anand Jain wrote: > We aren't checking the SB csum when the device scanned, > instead we do that when mounting the device, and if the > csum fails we fail the mount. How if we check the csum > when the device is scanned, I can't see any reason for > why not? any idea? So what problems does this solve? Only net "gain" I see is making a function public (which is a minus in my book) thus expanding the public interface. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/disk-io.c | 9 +++++---- > fs/btrfs/disk-io.h | 2 ++ > fs/btrfs/volumes.c | 4 ++++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 156116655a32..28e71e2aaa92 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -393,8 +393,8 @@ static int verify_parent_transid(struct extent_io_tree *io_tree, > * Return 0 if the superblock checksum type matches the checksum value of that > * algorithm. Pass the raw disk superblock data. > */ > -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > - char *raw_disk_sb) > +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > + char *raw_disk_sb) > { > struct btrfs_super_block *disk_sb = > (struct btrfs_super_block *)raw_disk_sb; > @@ -420,8 +420,9 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > } > > if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) { > - btrfs_err(fs_info, "unsupported checksum algorithm %u", > - csum_type); > + if (fs_info) > + btrfs_err(fs_info, "unsupported checksum algorithm %u", > + csum_type); > ret = 1; > } > > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index 70a88d61b547..683e4b0c4e24 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -70,6 +70,8 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev); > int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num, > struct buffer_head **bh_ret); > int btrfs_commit_super(struct btrfs_fs_info *fs_info); > +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > + char *raw_disk_sb); > struct btrfs_root *btrfs_read_fs_root(struct btrfs_root *tree_root, > struct btrfs_key *location); > int btrfs_init_fs_root(struct btrfs_root *root); > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 1e72357bdfa8..af34b8a611a0 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -677,6 +677,10 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > if (ret) > return ret; > > + if (btrfs_check_super_csum(NULL, bh->b_data)) { > + pr_err("BTRFS: superblock checksum mismatch"); > + goto error_brelse; > + } > disk_super = (struct btrfs_super_block *)bh->b_data; > devid = btrfs_stack_device_id(&disk_super->dev_item); > if (devid != device->devid) > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-03-14 05:20, Nikolay Borisov wrote: > On 13.03.2018 17:06, Anand Jain wrote: >> We aren't checking the SB csum when the device scanned, >> instead we do that when mounting the device, and if the >> csum fails we fail the mount. How if we check the csum >> when the device is scanned, I can't see any reason for >> why not? any idea? > > So what problems does this solve? Only net "gain" I see is making a > function public (which is a minus in my book) thus expanding the public > interface. > A device with bogus SB's is effectively a missing device as far as the mount code is concerned. AFAICT, this patch results in it being treated more like a missing device right from the start. OTOH, I'm not really sure if this is an improvement or not, as I've never had to deal with devices with invalid SB's before. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 11:06:37PM +0800, Anand Jain wrote: > We aren't checking the SB csum when the device scanned, > instead we do that when mounting the device, and if the > csum fails we fail the mount. How if we check the csum > when the device is scanned, I can't see any reason for > why not? any idea? I think we should do some minimal checks of the sb also during scanning, as btrfs_open_one_device accesses a few items from unchecked 'disk_super' directly. So at least magic number and checksum plus validate the checksum type. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 14, 2018 at 11:20:41AM +0200, Nikolay Borisov wrote: > > > On 13.03.2018 17:06, Anand Jain wrote: > > We aren't checking the SB csum when the device scanned, > > instead we do that when mounting the device, and if the > > csum fails we fail the mount. How if we check the csum > > when the device is scanned, I can't see any reason for > > why not? any idea? > > So what problems does this solve? Devices with partially corrupted superblock with a valid UUID will be added to list of scanned devices. This will be rejected at mount time so it's not a problem in the end but there are structures tracking the device, so we'd get rid of that early. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 11:06:37PM +0800, Anand Jain wrote: > We aren't checking the SB csum when the device scanned, > instead we do that when mounting the device, and if the > csum fails we fail the mount. How if we check the csum > when the device is scanned, I can't see any reason for > why not? any idea? > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/disk-io.c | 9 +++++---- > fs/btrfs/disk-io.h | 2 ++ > fs/btrfs/volumes.c | 4 ++++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 156116655a32..28e71e2aaa92 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -393,8 +393,8 @@ static int verify_parent_transid(struct extent_io_tree *io_tree, > * Return 0 if the superblock checksum type matches the checksum value of that > * algorithm. Pass the raw disk superblock data. > */ > -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > - char *raw_disk_sb) > +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > + char *raw_disk_sb) > { > struct btrfs_super_block *disk_sb = > (struct btrfs_super_block *)raw_disk_sb; > @@ -420,8 +420,9 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > } > > if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) { > - btrfs_err(fs_info, "unsupported checksum algorithm %u", > - csum_type); > + if (fs_info) > + btrfs_err(fs_info, "unsupported checksum algorithm %u", > + csum_type); > ret = 1; > } > > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index 70a88d61b547..683e4b0c4e24 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -70,6 +70,8 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev); > int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num, > struct buffer_head **bh_ret); > int btrfs_commit_super(struct btrfs_fs_info *fs_info); > +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > + char *raw_disk_sb); > struct btrfs_root *btrfs_read_fs_root(struct btrfs_root *tree_root, > struct btrfs_key *location); > int btrfs_init_fs_root(struct btrfs_root *root); > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 1e72357bdfa8..af34b8a611a0 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -677,6 +677,10 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > if (ret) > return ret; > > + if (btrfs_check_super_csum(NULL, bh->b_data)) { I think the error reports during scan should be minimized, in this case each time the scan will be run on that device, the checksum mismatch will be reported. I don't really like to see the NULL passed to btrfs_check_super_csum but it's still better than adding an extra parameter to set the verbosity. Documenting that at btrfs_check_super_csum would be good, mentioning that the function can be called from mount or scan. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 156116655a32..28e71e2aaa92 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -393,8 +393,8 @@ static int verify_parent_transid(struct extent_io_tree *io_tree, * Return 0 if the superblock checksum type matches the checksum value of that * algorithm. Pass the raw disk superblock data. */ -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, - char *raw_disk_sb) +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, + char *raw_disk_sb) { struct btrfs_super_block *disk_sb = (struct btrfs_super_block *)raw_disk_sb; @@ -420,8 +420,9 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, } if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) { - btrfs_err(fs_info, "unsupported checksum algorithm %u", - csum_type); + if (fs_info) + btrfs_err(fs_info, "unsupported checksum algorithm %u", + csum_type); ret = 1; } diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index 70a88d61b547..683e4b0c4e24 100644 --- a/fs/btrfs/disk-io.h +++ b/fs/btrfs/disk-io.h @@ -70,6 +70,8 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev); int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num, struct buffer_head **bh_ret); int btrfs_commit_super(struct btrfs_fs_info *fs_info); +int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, + char *raw_disk_sb); struct btrfs_root *btrfs_read_fs_root(struct btrfs_root *tree_root, struct btrfs_key *location); int btrfs_init_fs_root(struct btrfs_root *root); diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 1e72357bdfa8..af34b8a611a0 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -677,6 +677,10 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, if (ret) return ret; + if (btrfs_check_super_csum(NULL, bh->b_data)) { + pr_err("BTRFS: superblock checksum mismatch"); + goto error_brelse; + } disk_super = (struct btrfs_super_block *)bh->b_data; devid = btrfs_stack_device_id(&disk_super->dev_item); if (devid != device->devid)
We aren't checking the SB csum when the device scanned, instead we do that when mounting the device, and if the csum fails we fail the mount. How if we check the csum when the device is scanned, I can't see any reason for why not? any idea? Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/disk-io.c | 9 +++++---- fs/btrfs/disk-io.h | 2 ++ fs/btrfs/volumes.c | 4 ++++ 3 files changed, 11 insertions(+), 4 deletions(-)