| Message ID | 1462212951-28113-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Oh Dave, I got to use a wrong email address of yours and got some failures. Thanks, -liubo On Mon, May 02, 2016 at 11:15:50AM -0700, Liu Bo wrote: > This adds valid checks for super_total_bytes, super_bytes_used and > super_stripesize. > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/disk-io.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 4e47849..988d03f 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > * Hint to catch really bogus numbers, bitflips or so, more exact checks are > * done later > */ > + if (btrfs_super_total_bytes(sb) == 0) { > + printk(KERN_ERR "BTRFS: total bytes is zero\n"); > + ret = -EINVAL; > + } > + if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) { > + printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n", > + btrfs_super_bytes_used(sb)); > + ret = -EINVAL; > + } > + if (btrfs_super_stripesize(sb) != 4096) { > + printk(KERN_ERR "BTRFS: invalid stripesize %u\n", > + btrfs_super_stripesize(sb)); > + ret = -EINVAL; > + } > if (btrfs_super_num_devices(sb) > (1UL << 31)) > printk(KERN_WARNING "BTRFS: suspicious number of devices: %llu\n", > btrfs_super_num_devices(sb)); > -- > 2.5.5 > > -- > 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 -- 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
Liu Bo wrote on 2016/05/02 11:15 -0700: > This adds valid checks for super_total_bytes, super_bytes_used and > super_stripesize. > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/disk-io.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 4e47849..988d03f 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > * Hint to catch really bogus numbers, bitflips or so, more exact checks are > * done later > */ > + if (btrfs_super_total_bytes(sb) == 0) { > + printk(KERN_ERR "BTRFS: total bytes is zero\n"); > + ret = -EINVAL; > + } Would it be better if using "6 * nodesize"? I'd like to use a precious low limit on total bytes, but we don't have such value, so 6 nodesize would be good. Thanks, Qu > + if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) { > + printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n", > + btrfs_super_bytes_used(sb)); > + ret = -EINVAL; > + } > + if (btrfs_super_stripesize(sb) != 4096) { > + printk(KERN_ERR "BTRFS: invalid stripesize %u\n", > + btrfs_super_stripesize(sb)); > + ret = -EINVAL; > + } > if (btrfs_super_num_devices(sb) > (1UL << 31)) > printk(KERN_WARNING "BTRFS: suspicious number of devices: %llu\n", > btrfs_super_num_devices(sb)); > -- 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, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote: > > > Liu Bo wrote on 2016/05/02 11:15 -0700: > >This adds valid checks for super_total_bytes, super_bytes_used and > >super_stripesize. > > > >Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > >Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > >Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > >--- > > fs/btrfs/disk-io.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > >diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > >index 4e47849..988d03f 100644 > >--- a/fs/btrfs/disk-io.c > >+++ b/fs/btrfs/disk-io.c > >@@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > > * Hint to catch really bogus numbers, bitflips or so, more exact checks are > > * done later > > */ > >+ if (btrfs_super_total_bytes(sb) == 0) { > >+ printk(KERN_ERR "BTRFS: total bytes is zero\n"); > >+ ret = -EINVAL; > >+ } > > Would it be better if using "6 * nodesize"? > > I'd like to use a precious low limit on total bytes, but we don't have such > value, so 6 nodesize would be good. That's good, besides that I'm going to do another check between btrfs_super_total_bytes(sb) and sb->dev_item.total_bytes. Thanks, -liubo > > Thanks, > Qu > > >+ if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) { > >+ printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n", > >+ btrfs_super_bytes_used(sb)); > >+ ret = -EINVAL; > >+ } > >+ if (btrfs_super_stripesize(sb) != 4096) { > >+ printk(KERN_ERR "BTRFS: invalid stripesize %u\n", > >+ btrfs_super_stripesize(sb)); > >+ ret = -EINVAL; > >+ } > > if (btrfs_super_num_devices(sb) > (1UL << 31)) > > printk(KERN_WARNING "BTRFS: suspicious number of devices: %llu\n", > > btrfs_super_num_devices(sb)); > > > > -- 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, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote: > > > Liu Bo wrote on 2016/05/02 11:15 -0700: > > This adds valid checks for super_total_bytes, super_bytes_used and > > super_stripesize. > > > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > fs/btrfs/disk-io.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index 4e47849..988d03f 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > > * Hint to catch really bogus numbers, bitflips or so, more exact checks are > > * done later > > */ > > + if (btrfs_super_total_bytes(sb) == 0) { > > + printk(KERN_ERR "BTRFS: total bytes is zero\n"); > > + ret = -EINVAL; > > + } > > Would it be better if using "6 * nodesize"? > > I'd like to use a precious low limit on total bytes, but we don't have > such value, so 6 nodesize would be good. An early check can compare against some reasonable value, but the total_bytes value must be equal to the sum of all device sizes (disk_total_bytes). I'm not sure if we have enough information to verify that at this point though. -- 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 Mon, May 02, 2016 at 11:15:50AM -0700, Liu Bo wrote: > This adds valid checks for super_total_bytes, super_bytes_used and > super_stripesize. > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/disk-io.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 4e47849..988d03f 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > * Hint to catch really bogus numbers, bitflips or so, more exact checks are > * done later > */ > + if (btrfs_super_total_bytes(sb) == 0) { > + printk(KERN_ERR "BTRFS: total bytes is zero\n"); > + ret = -EINVAL; > + } > + if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) { Similar to total_bytes (sum of device->total_bytes), bytes_used is sum of of all device->used_bytes, which in turn is sum of all block group sizes on the device. > + printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n", > + btrfs_super_bytes_used(sb)); > + ret = -EINVAL; > + } > + if (btrfs_super_stripesize(sb) != 4096) { This is too strict. The stripesize is unused, but we not force it to be 4k, a multiple of nodesize/sectorsize should be enough. > + printk(KERN_ERR "BTRFS: invalid stripesize %u\n", > + btrfs_super_stripesize(sb)); > + ret = -EINVAL; > + } > if (btrfs_super_num_devices(sb) > (1UL << 31)) > printk(KERN_WARNING "BTRFS: suspicious number of devices: %llu\n", > btrfs_super_num_devices(sb)); > -- > 2.5.5 > > -- > 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 -- 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, May 04, 2016 at 03:29:35PM +0200, David Sterba wrote: > On Mon, May 02, 2016 at 11:15:50AM -0700, Liu Bo wrote: > > This adds valid checks for super_total_bytes, super_bytes_used and > > super_stripesize. > > > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > fs/btrfs/disk-io.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index 4e47849..988d03f 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > > * Hint to catch really bogus numbers, bitflips or so, more exact checks are > > * done later > > */ > > + if (btrfs_super_total_bytes(sb) == 0) { > > + printk(KERN_ERR "BTRFS: total bytes is zero\n"); > > + ret = -EINVAL; > > + } > > + if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) { > > Similar to total_bytes (sum of device->total_bytes), bytes_used is sum > of of all device->used_bytes, which in turn is sum of all block group > sizes on the device. super_bytes_used has different meanings with device->used_bytes, device->used_bytes is space that has been allocated to block groups, super_bytes_used is space that has been consumed by data/metadata. > > > + printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n", > > + btrfs_super_bytes_used(sb)); > > + ret = -EINVAL; > > + } > > + if (btrfs_super_stripesize(sb) != 4096) { > > This is too strict. The stripesize is unused, but we not force it to be > 4k, a multiple of nodesize/sectorsize should be enough. Hmm, in fact stripesize is used in find_free_extent(), find_free_extent() { ... search_start = ALIGN(offset, root->stripesize); ... } and in open_ctree(), open_ctree() { ... stripesize = btrfs_super_stripesize(disk_super); ... tree_root->stripesize = stripesize; ... } btrfs_read_roots() { ... btrfs_read_tree_root() --> __setup_root(..., tree_root->stripesize, ...) } Thus, this stripesize has to be sectorsize at least. Thanks, -liubo > > > + printk(KERN_ERR "BTRFS: invalid stripesize %u\n", > > + btrfs_super_stripesize(sb)); > > + ret = -EINVAL; > > + } > > if (btrfs_super_num_devices(sb) > (1UL << 31)) > > printk(KERN_WARNING "BTRFS: suspicious number of devices: %llu\n", > > btrfs_super_num_devices(sb)); > > -- > > 2.5.5 > > > > -- > > 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 -- 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, May 04, 2016 at 03:23:29PM +0200, David Sterba wrote: > On Tue, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote: > > > > > > Liu Bo wrote on 2016/05/02 11:15 -0700: > > > This adds valid checks for super_total_bytes, super_bytes_used and > > > super_stripesize. > > > > > > Reported-by: Vegard Nossum <vegard.nossum@oracle.com> > > > Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > > --- > > > fs/btrfs/disk-io.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > > index 4e47849..988d03f 100644 > > > --- a/fs/btrfs/disk-io.c > > > +++ b/fs/btrfs/disk-io.c > > > @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > > > * Hint to catch really bogus numbers, bitflips or so, more exact checks are > > > * done later > > > */ > > > + if (btrfs_super_total_bytes(sb) == 0) { > > > + printk(KERN_ERR "BTRFS: total bytes is zero\n"); > > > + ret = -EINVAL; > > > + } > > > > Would it be better if using "6 * nodesize"? > > > > I'd like to use a precious low limit on total bytes, but we don't have > > such value, so 6 nodesize would be good. > > An early check can compare against some reasonable value, but the > total_bytes value must be equal to the sum of all device sizes > (disk_total_bytes). I'm not sure if we have enough information to verify > that at this point though. That's what I had in mind, the problem is that only the first device information is recorded in superblock. At this moment We have device_num but we don't know the size of other devices. Thanks, -liubo -- 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
Liu Bo wrote on 2016/05/04 10:44 -0700: > On Wed, May 04, 2016 at 03:23:29PM +0200, David Sterba wrote: >> On Tue, May 03, 2016 at 09:02:56AM +0800, Qu Wenruo wrote: >>> >>> >>> Liu Bo wrote on 2016/05/02 11:15 -0700: >>>> This adds valid checks for super_total_bytes, super_bytes_used and >>>> super_stripesize. >>>> >>>> Reported-by: Vegard Nossum <vegard.nossum@oracle.com> >>>> Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> >>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com> >>>> --- >>>> fs/btrfs/disk-io.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>>> index 4e47849..988d03f 100644 >>>> --- a/fs/btrfs/disk-io.c >>>> +++ b/fs/btrfs/disk-io.c >>>> @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, >>>> * Hint to catch really bogus numbers, bitflips or so, more exact checks are >>>> * done later >>>> */ >>>> + if (btrfs_super_total_bytes(sb) == 0) { >>>> + printk(KERN_ERR "BTRFS: total bytes is zero\n"); >>>> + ret = -EINVAL; >>>> + } >>> >>> Would it be better if using "6 * nodesize"? >>> >>> I'd like to use a precious low limit on total bytes, but we don't have >>> such value, so 6 nodesize would be good. >> >> An early check can compare against some reasonable value, but the >> total_bytes value must be equal to the sum of all device sizes >> (disk_total_bytes). I'm not sure if we have enough information to verify >> that at this point though. > > That's what I had in mind, the problem is that only the first device information is recorded in superblock. > > At this moment We have device_num but we don't know the size of other devices. > > Thanks, > > -liubo > > What about error out if we found sb->total_bytes < sb->dev_item->total_bytes? As we are just doing early check, no need to be comprehensive, but spot obvious problem. For exact device_num and sb->total_bytes, we may do post check when device tree are loaded? Splitting early_check() and post_check() seems valid for me. (Also I prefer post_check() just warning, not forced exit) Thanks, Qu -- 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 Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote: > >> An early check can compare against some reasonable value, but the > >> total_bytes value must be equal to the sum of all device sizes > >> (disk_total_bytes). I'm not sure if we have enough information to verify > >> that at this point though. > > > > That's what I had in mind, the problem is that only the first device information is recorded in superblock. > > > > At this moment We have device_num but we don't know the size of other devices. > > > > Thanks, > > > > -liubo > > > > > What about error out if we found sb->total_bytes < > sb->dev_item->total_bytes? > > As we are just doing early check, no need to be comprehensive, but spot > obvious problem. Ok. > For exact device_num and sb->total_bytes, we may do post check when > device tree are loaded? > Splitting early_check() and post_check() seems valid for me. > (Also I prefer post_check() just warning, not forced exit) Why just warning? Superblock total_bytes and device sizes must be correct, otherwise all sorts of operations can fail randomly. -- 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, May 04, 2016 at 10:40:02AM -0700, Liu Bo wrote: > > > + printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n", > > > + btrfs_super_bytes_used(sb)); > > > + ret = -EINVAL; > > > + } > > > + if (btrfs_super_stripesize(sb) != 4096) { > > > > This is too strict. The stripesize is unused, but we not force it to be > > 4k, a multiple of nodesize/sectorsize should be enough. > > Hmm, in fact stripesize is used in find_free_extent(), > > find_free_extent() { > ... > search_start = ALIGN(offset, root->stripesize); > ... > } > > and in open_ctree(), > > open_ctree() { > ... > stripesize = btrfs_super_stripesize(disk_super); > ... > tree_root->stripesize = stripesize; > ... > } > > btrfs_read_roots() { > ... > btrfs_read_tree_root() --> __setup_root(..., tree_root->stripesize, ...) > } > > Thus, this stripesize has to be sectorsize at least. Yes, and must be power of two because of ALIGN for now. -- 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
David Sterba wrote on 2016/05/06 16:35 +0200: > On Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote: >>>> An early check can compare against some reasonable value, but the >>>> total_bytes value must be equal to the sum of all device sizes >>>> (disk_total_bytes). I'm not sure if we have enough information to verify >>>> that at this point though. >>> >>> That's what I had in mind, the problem is that only the first device information is recorded in superblock. >>> >>> At this moment We have device_num but we don't know the size of other devices. >>> >>> Thanks, >>> >>> -liubo >>> >>> >> What about error out if we found sb->total_bytes < >> sb->dev_item->total_bytes? >> >> As we are just doing early check, no need to be comprehensive, but spot >> obvious problem. > > Ok. > >> For exact device_num and sb->total_bytes, we may do post check when >> device tree are loaded? >> Splitting early_check() and post_check() seems valid for me. >> (Also I prefer post_check() just warning, not forced exit) > > Why just warning? Superblock total_bytes and device sizes must be > correct, otherwise all sorts of operations can fail randomly. > > Because if we exit, we can't even do fsck. Maybe we need a new flag to control whether exit or warn at post_check(). Thanks, Qu -- 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 Mon, May 09, 2016 at 09:31:37AM +0800, Qu Wenruo wrote: > > > David Sterba wrote on 2016/05/06 16:35 +0200: > > On Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote: > > > > > An early check can compare against some reasonable value, but the > > > > > total_bytes value must be equal to the sum of all device sizes > > > > > (disk_total_bytes). I'm not sure if we have enough information to verify > > > > > that at this point though. > > > > > > > > That's what I had in mind, the problem is that only the first device information is recorded in superblock. > > > > > > > > At this moment We have device_num but we don't know the size of other devices. > > > > > > > > Thanks, > > > > > > > > -liubo > > > > > > > > > > > What about error out if we found sb->total_bytes < > > > sb->dev_item->total_bytes? > > > > > > As we are just doing early check, no need to be comprehensive, but spot > > > obvious problem. > > > > Ok. I'm gonna check for total_bytes and num_devices after loading chunk tree. > > > > > For exact device_num and sb->total_bytes, we may do post check when > > > device tree are loaded? > > > Splitting early_check() and post_check() seems valid for me. > > > (Also I prefer post_check() just warning, not forced exit) > > > > Why just warning? Superblock total_bytes and device sizes must be > > correct, otherwise all sorts of operations can fail randomly. > > > > > Because if we exit, we can't even do fsck. > > Maybe we need a new flag to control whether exit or warn at post_check(). > > Thanks, > Qu > IMHO for kernel part, we have to exit in order to avoid any panic due to those invalid value. For fsck code, we can go forth and back to fix them. In fact I don't think fsck could work out anything, as superblock checksum has _matched_ but the values inside superblock are invalid, in this case we cannot trust other parts in this FS image, then how can we expect fsck to fix it by reading other parts? Thanks, -liubo -- 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 05/14/2016 02:14 AM, Liu Bo wrote: > On Mon, May 09, 2016 at 09:31:37AM +0800, Qu Wenruo wrote: >> >> >> David Sterba wrote on 2016/05/06 16:35 +0200: >>> On Thu, May 05, 2016 at 09:08:54AM +0800, Qu Wenruo wrote: >>>>>> An early check can compare against some reasonable value, but the >>>>>> total_bytes value must be equal to the sum of all device sizes >>>>>> (disk_total_bytes). I'm not sure if we have enough information to verify >>>>>> that at this point though. >>>>> >>>>> That's what I had in mind, the problem is that only the first device information is recorded in superblock. >>>>> >>>>> At this moment We have device_num but we don't know the size of other devices. >>>>> >>>>> Thanks, >>>>> >>>>> -liubo >>>>> >>>>> >>>> What about error out if we found sb->total_bytes < >>>> sb->dev_item->total_bytes? >>>> >>>> As we are just doing early check, no need to be comprehensive, but spot >>>> obvious problem. >>> >>> Ok. > > I'm gonna check for total_bytes and num_devices after loading chunk > tree. > >>> >>>> For exact device_num and sb->total_bytes, we may do post check when >>>> device tree are loaded? >>>> Splitting early_check() and post_check() seems valid for me. >>>> (Also I prefer post_check() just warning, not forced exit) >>> >>> Why just warning? Superblock total_bytes and device sizes must be >>> correct, otherwise all sorts of operations can fail randomly. >>> >>> >> Because if we exit, we can't even do fsck. >> >> Maybe we need a new flag to control whether exit or warn at post_check(). >> > >> Thanks, >> Qu >> > > IMHO for kernel part, we have to exit in order to avoid any panic due to those invalid value. I'm OK with this. > > For fsck code, we can go forth and back to fix them. In fact I don't > think fsck could work out anything, as superblock checksum has _matched_ > but the values inside superblock are invalid, in this case we cannot trust > other parts in this FS image, then how can we expect fsck to fix it by reading > other parts? For rw fsck, that may cause huge problem and I agree with you on error out. But for ro fsck, it's a little overkilled for me. Currently, if we found error in extent tree, we still continue checking fstree, to shows what is really wrong. And for case like btrfs-image restored images, its dev extents doesn't even match with its chunk (may be it's already fixed?), but that's not a big problem for ro btrfsck, and we can go on without problem. So the same case is here for ro btrfsck. As long as that's ro btrfsck, we could just continue as we don't really need the total_bytes in superblock. Thanks, Qu > > Thanks, > > -liubo > -- > 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 > -- 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 Sat, May 14, 2016 at 07:42:26AM +0800, Qu Wenruo wrote: > > For fsck code, we can go forth and back to fix them. In fact I don't > > think fsck could work out anything, as superblock checksum has _matched_ > > but the values inside superblock are invalid, in this case we cannot trust > > other parts in this FS image, then how can we expect fsck to fix it by reading > > other parts? > > For rw fsck, that may cause huge problem and I agree with you on error out. > But for ro fsck, it's a little overkilled for me. > > Currently, if we found error in extent tree, we still continue checking > fstree, to shows what is really wrong. > > And for case like btrfs-image restored images, its dev extents doesn't > even match with its chunk (may be it's already fixed?), but that's not a > big problem for ro btrfsck, and we can go on without problem. > > So the same case is here for ro btrfsck. > As long as that's ro btrfsck, we could just continue as we don't really > need the total_bytes in superblock. AFAICS super_block->total_bytes is used in btrfs_alloc_chunk and in several places in mkfs, but 'check' does not look at the value at all, so warning for the read-only should be ok. -- 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 4e47849..988d03f 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -4120,6 +4120,20 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, * Hint to catch really bogus numbers, bitflips or so, more exact checks are * done later */ + if (btrfs_super_total_bytes(sb) == 0) { + printk(KERN_ERR "BTRFS: total bytes is zero\n"); + ret = -EINVAL; + } + if (btrfs_super_bytes_used(sb) < 6 * btrfs_super_nodesize(sb)) { + printk(KERN_ERR "BTRFS: bytes_used is too small %llu\n", + btrfs_super_bytes_used(sb)); + ret = -EINVAL; + } + if (btrfs_super_stripesize(sb) != 4096) { + printk(KERN_ERR "BTRFS: invalid stripesize %u\n", + btrfs_super_stripesize(sb)); + ret = -EINVAL; + } if (btrfs_super_num_devices(sb) > (1UL << 31)) printk(KERN_WARNING "BTRFS: suspicious number of devices: %llu\n", btrfs_super_num_devices(sb));
This adds valid checks for super_total_bytes, super_bytes_used and super_stripesize. Reported-by: Vegard Nossum <vegard.nossum@oracle.com> Reported-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/disk-io.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)