| Message ID | 1463184422-13584-3-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Liu, Thanks for your patch first. On 05/14/2016 08:06 AM, Liu Bo wrote: > Thanks to fuzz testing, we can pass an invalid bytenr to extent buffer > via alloc_extent_buffer(). An unaligned eb can have more pages than it > should have, which ends up extent buffer's leak or some corrupted content > in extent buffer. > > This adds a warning to let us quickly know what was happening. > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/extent_io.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index d247fc0..e601e0f 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -4868,6 +4868,10 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > int uptodate = 1; > int ret; > > + WARN_ONCE(!IS_ALIGNED(start, fs_info->tree_root->sectorsize), > + KERN_WARNING "eb->start(%llu) is not aligned to root->sectorsize(%u)\n", > + start, fs_info->tree_root->sectorsize); > + IMHO this is a quite big problem. As almost all other things rely on the assumption that extent buffer are at least sectorsize aligned. What about warning and returning NULL? WARN_ONCE() only won't info user quick enough. BTW, after a quick glance into __alloc_extent_buffer(), it seems that we didn't check the return pointer of kmem_cache_zalloc(), since you're fixing things around that code, would you mind to fix it too? Thanks, Qu > eb = find_extent_buffer(fs_info, start); > if (eb) > return eb; > -- 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 06:30:52PM +0800, Qu Wenruo wrote: > Hi Liu, > > Thanks for your patch first. > > On 05/14/2016 08:06 AM, Liu Bo wrote: > > Thanks to fuzz testing, we can pass an invalid bytenr to extent buffer > > via alloc_extent_buffer(). An unaligned eb can have more pages than it > > should have, which ends up extent buffer's leak or some corrupted content > > in extent buffer. > > > > This adds a warning to let us quickly know what was happening. > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > fs/btrfs/extent_io.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > index d247fc0..e601e0f 100644 > > --- a/fs/btrfs/extent_io.c > > +++ b/fs/btrfs/extent_io.c > > @@ -4868,6 +4868,10 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > > int uptodate = 1; > > int ret; > > > > + WARN_ONCE(!IS_ALIGNED(start, fs_info->tree_root->sectorsize), > > + KERN_WARNING "eb->start(%llu) is not aligned to root->sectorsize(%u)\n", > > + start, fs_info->tree_root->sectorsize); > > + > > IMHO this is a quite big problem. As almost all other things rely on the > assumption that extent buffer are at least sectorsize aligned. > It won't cause too much trouble as reading eb's page can prevent btrfs using this eb. > What about warning and returning NULL? WARN_ONCE() only won't info user > quick enough. I'm OK with warning, but I just realized that warning doesn't show which filesystem has problems, so btrfs_crit and -EINVAL is preferable. > > BTW, after a quick glance into __alloc_extent_buffer(), it seems that we > didn't check the return pointer of kmem_cache_zalloc(), since you're fixing > things around that code, would you mind to fix it too? It's not necessary to do that since it's using __GFP_NOFAIL. Thanks, -liubo > > Thanks, > Qu > > > eb = find_extent_buffer(fs_info, start); > > if (eb) > > return eb; > > -- 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 16, 2016 at 11:01:41AM -0700, Liu Bo wrote: > On Sat, May 14, 2016 at 06:30:52PM +0800, Qu Wenruo wrote: > > Hi Liu, > > > > Thanks for your patch first. > > > > On 05/14/2016 08:06 AM, Liu Bo wrote: > > > Thanks to fuzz testing, we can pass an invalid bytenr to extent buffer > > > via alloc_extent_buffer(). An unaligned eb can have more pages than it > > > should have, which ends up extent buffer's leak or some corrupted content > > > in extent buffer. > > > > > > This adds a warning to let us quickly know what was happening. > > > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > > --- > > > fs/btrfs/extent_io.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > > index d247fc0..e601e0f 100644 > > > --- a/fs/btrfs/extent_io.c > > > +++ b/fs/btrfs/extent_io.c > > > @@ -4868,6 +4868,10 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > > > int uptodate = 1; > > > int ret; > > > > > > + WARN_ONCE(!IS_ALIGNED(start, fs_info->tree_root->sectorsize), > > > + KERN_WARNING "eb->start(%llu) is not aligned to root->sectorsize(%u)\n", > > > + start, fs_info->tree_root->sectorsize); > > > + > > > > IMHO this is a quite big problem. As almost all other things rely on the > > assumption that extent buffer are at least sectorsize aligned. > > It won't cause too much trouble as reading eb's page can prevent btrfs > using this eb. > > > What about warning and returning NULL? WARN_ONCE() only won't info user > > quick enough. > > I'm OK with warning, but I just realized that warning doesn't show which > filesystem has problems, so btrfs_crit and -EINVAL is preferable. NULL means it's allocation error, so please convert it to IS_ERR and return more fine grained errors so we can distinguish the problems. An unaligned 'start' almost always means a corruption or other problem in the callers of alloc_extent_buffer(). -- 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 17, 2016 at 11:39:52AM +0200, David Sterba wrote: > On Mon, May 16, 2016 at 11:01:41AM -0700, Liu Bo wrote: > > On Sat, May 14, 2016 at 06:30:52PM +0800, Qu Wenruo wrote: > > > Hi Liu, > > > > > > Thanks for your patch first. > > > > > > On 05/14/2016 08:06 AM, Liu Bo wrote: > > > > Thanks to fuzz testing, we can pass an invalid bytenr to extent buffer > > > > via alloc_extent_buffer(). An unaligned eb can have more pages than it > > > > should have, which ends up extent buffer's leak or some corrupted content > > > > in extent buffer. > > > > > > > > This adds a warning to let us quickly know what was happening. > > > > > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > > > --- > > > > fs/btrfs/extent_io.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > > > index d247fc0..e601e0f 100644 > > > > --- a/fs/btrfs/extent_io.c > > > > +++ b/fs/btrfs/extent_io.c > > > > @@ -4868,6 +4868,10 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > > > > int uptodate = 1; > > > > int ret; > > > > > > > > + WARN_ONCE(!IS_ALIGNED(start, fs_info->tree_root->sectorsize), > > > > + KERN_WARNING "eb->start(%llu) is not aligned to root->sectorsize(%u)\n", > > > > + start, fs_info->tree_root->sectorsize); > > > > + > > > > > > IMHO this is a quite big problem. As almost all other things rely on the > > > assumption that extent buffer are at least sectorsize aligned. > > > > It won't cause too much trouble as reading eb's page can prevent btrfs > > using this eb. > > > > > What about warning and returning NULL? WARN_ONCE() only won't info user > > > quick enough. > > > > I'm OK with warning, but I just realized that warning doesn't show which > > filesystem has problems, so btrfs_crit and -EINVAL is preferable. > > NULL means it's allocation error, so please convert it to IS_ERR and > return more fine grained errors so we can distinguish the problems. An > unaligned 'start' almost always means a corruption or other problem in > the callers of alloc_extent_buffer(). OK, sounds good. 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
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index d247fc0..e601e0f 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -4868,6 +4868,10 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, int uptodate = 1; int ret; + WARN_ONCE(!IS_ALIGNED(start, fs_info->tree_root->sectorsize), + KERN_WARNING "eb->start(%llu) is not aligned to root->sectorsize(%u)\n", + start, fs_info->tree_root->sectorsize); + eb = find_extent_buffer(fs_info, start); if (eb) return eb;
Thanks to fuzz testing, we can pass an invalid bytenr to extent buffer via alloc_extent_buffer(). An unaligned eb can have more pages than it should have, which ends up extent buffer's leak or some corrupted content in extent buffer. This adds a warning to let us quickly know what was happening. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/extent_io.c | 4 ++++ 1 file changed, 4 insertions(+)