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:

quoted

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 <redacted>
---
 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

quoted

 	eb = find_extent_buffer(fs_info, start);
 	if (eb)
 		return eb;