All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fs/btrfs: Make extent item iteration to handle gaps
@ 2021-10-16  1:40 Qu Wenruo
  2021-10-28  7:36 ` Qu Wenruo
  0 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2021-10-16  1:40 UTC (permalink / raw)
  To: grub-devel; +Cc: linux-btrfs

[BUG]
Grub btrfs implementation can't handle two very basic btrfs file
layouts:

1. Mixed inline/regualr extents
   # mkfs.btrfs -f test.img
   # mount test.img /mnt/btrfs
   # xfs_io -f -c "pwrite 0 1k" -c "sync" -c "falloc 0 4k" \
	       -c "pwrite 4k 4k" /mnt/btrfs/file
   # umount /mnt/btrfs
   # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"

   Such mixed inline/regular extents case is not recommended layout,
   but all existing tools and kernel can handle it without problem

2. NO_HOLES feature
   # mkfs.btrfs -f test.img -O no_holes
   # mount test.img /mnt/btrfs
   # xfs_io -f -c "pwrite 0 4k" -c "pwrite 8k 4k" /mnt/btrfs/file
   # umount /mnt/btrfs
   # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"

   NO_HOLES feature is going to be the default mkfs feature in the incoming
   v5.15 release, and kernel has support for it since v4.0.

[CAUSE]
The way GRUB btrfs code iterates through file extents relies on no gap
between extents.

If any gap is hit, then grub btrfs will error out, without any proper
reason to help debug the bug.

This is a bad assumption, since a long long time ago btrfs has a new
feature called NO_HOLES to allow btrfs to skip the padding hole extent
to reduce metadata usage.

The NO_HOLES feature is already stable since kernel v4.0 and is going to
be the default mkfs feature in the incoming v5.15 btrfs-progs release.

[FIX]
When there is a extent gap, instead of error out, just try next item.

This is still not ideal, as kernel/progs/U-boot all do the iteration
item by item, not relying on the file offset continuity.

But it will be way more time consuming to correct the whole behavior
than starting from scratch to build a proper designed btrfs module for GRUB.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 grub-core/fs/btrfs.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 63203034dfc6..4fbcbec7524a 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -1443,6 +1443,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
       grub_size_t csize;
       grub_err_t err;
       grub_off_t extoff;
+      struct grub_btrfs_leaf_descriptor desc;
       if (!data->extent || data->extstart > pos || data->extino != ino
 	  || data->exttree != tree || data->extend <= pos)
 	{
@@ -1455,7 +1456,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
 	  key_in.type = GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM;
 	  key_in.offset = grub_cpu_to_le64 (pos);
 	  err = lower_bound (data, &key_in, &key_out, tree,
-			     &elemaddr, &elemsize, NULL, 0);
+			     &elemaddr, &elemsize, &desc, 0);
 	  if (err)
 	    return -1;
 	  if (key_out.object_id != ino
@@ -1494,10 +1495,36 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
 			PRIxGRUB_UINT64_T "\n",
 			grub_le_to_cpu64 (key_out.offset),
 			grub_le_to_cpu64 (data->extent->size));
+	  /*
+	   * The way of extent item iteration is pretty bad, it completely
+	   * requires all extents are contiguous, which is not ensured.
+	   *
+	   * Features like NO_HOLE and mixed inline/regular extents can cause
+	   * gaps between file extent items.
+	   *
+	   * The correct way is to follow kernel/U-boot to iterate item by
+	   * item, without any assumption on the file offset continuity.
+	   *
+	   * Here we just manually skip to next item and re-do the verification.
+	   *
+	   * TODO: Rework the whole extent item iteration code, if not the
+	   * whole btrfs implementation.
+	   */
 	  if (data->extend <= pos)
 	    {
-	      grub_error (GRUB_ERR_BAD_FS, "extent not found");
-	      return -1;
+	      err = next(data, &desc, &elemaddr, &elemsize, &key_out);
+	      if (err < 0)
+		return -1;
+	      /* No next item for the inode, we hit the end */
+	      if (err == 0 || key_out.object_id != ino ||
+		  key_out.type != GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM)
+		      return pos - pos0;
+
+	      csize = grub_le_to_cpu64(key_out.offset) - pos;
+	      buf += csize;
+	      pos += csize;
+	      len -= csize;
+	      continue;
 	    }
 	}
       csize = data->extend - pos;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/btrfs: Make extent item iteration to handle gaps
  2021-10-16  1:40 [PATCH] fs/btrfs: Make extent item iteration to handle gaps Qu Wenruo
@ 2021-10-28  7:36 ` Qu Wenruo
  2021-10-28  9:27   ` Michael Chang
  2021-10-29 12:52   ` David Sterba
  0 siblings, 2 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-10-28  7:36 UTC (permalink / raw)
  To: grub-devel; +Cc: linux-btrfs

Gentle ping?

Without this patch, the new mkfs.btrfs NO_HOLES feature would break any 
kernel/initramfs with hole in it.

And considering the modification is already small, I believe this patch 
is definitely worthy as a bug fix.

Thanks,
Qu

On 2021/10/16 09:40, Qu Wenruo wrote:
> [BUG]
> Grub btrfs implementation can't handle two very basic btrfs file
> layouts:
> 
> 1. Mixed inline/regualr extents
>     # mkfs.btrfs -f test.img
>     # mount test.img /mnt/btrfs
>     # xfs_io -f -c "pwrite 0 1k" -c "sync" -c "falloc 0 4k" \
> 	       -c "pwrite 4k 4k" /mnt/btrfs/file
>     # umount /mnt/btrfs
>     # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"
> 
>     Such mixed inline/regular extents case is not recommended layout,
>     but all existing tools and kernel can handle it without problem
> 
> 2. NO_HOLES feature
>     # mkfs.btrfs -f test.img -O no_holes
>     # mount test.img /mnt/btrfs
>     # xfs_io -f -c "pwrite 0 4k" -c "pwrite 8k 4k" /mnt/btrfs/file
>     # umount /mnt/btrfs
>     # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"
> 
>     NO_HOLES feature is going to be the default mkfs feature in the incoming
>     v5.15 release, and kernel has support for it since v4.0.
> 
> [CAUSE]
> The way GRUB btrfs code iterates through file extents relies on no gap
> between extents.
> 
> If any gap is hit, then grub btrfs will error out, without any proper
> reason to help debug the bug.
> 
> This is a bad assumption, since a long long time ago btrfs has a new
> feature called NO_HOLES to allow btrfs to skip the padding hole extent
> to reduce metadata usage.
> 
> The NO_HOLES feature is already stable since kernel v4.0 and is going to
> be the default mkfs feature in the incoming v5.15 btrfs-progs release.
> 
> [FIX]
> When there is a extent gap, instead of error out, just try next item.
> 
> This is still not ideal, as kernel/progs/U-boot all do the iteration
> item by item, not relying on the file offset continuity.
> 
> But it will be way more time consuming to correct the whole behavior
> than starting from scratch to build a proper designed btrfs module for GRUB.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   grub-core/fs/btrfs.c | 33 ++++++++++++++++++++++++++++++---
>   1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 63203034dfc6..4fbcbec7524a 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -1443,6 +1443,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
>         grub_size_t csize;
>         grub_err_t err;
>         grub_off_t extoff;
> +      struct grub_btrfs_leaf_descriptor desc;
>         if (!data->extent || data->extstart > pos || data->extino != ino
>   	  || data->exttree != tree || data->extend <= pos)
>   	{
> @@ -1455,7 +1456,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
>   	  key_in.type = GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM;
>   	  key_in.offset = grub_cpu_to_le64 (pos);
>   	  err = lower_bound (data, &key_in, &key_out, tree,
> -			     &elemaddr, &elemsize, NULL, 0);
> +			     &elemaddr, &elemsize, &desc, 0);
>   	  if (err)
>   	    return -1;
>   	  if (key_out.object_id != ino
> @@ -1494,10 +1495,36 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
>   			PRIxGRUB_UINT64_T "\n",
>   			grub_le_to_cpu64 (key_out.offset),
>   			grub_le_to_cpu64 (data->extent->size));
> +	  /*
> +	   * The way of extent item iteration is pretty bad, it completely
> +	   * requires all extents are contiguous, which is not ensured.
> +	   *
> +	   * Features like NO_HOLE and mixed inline/regular extents can cause
> +	   * gaps between file extent items.
> +	   *
> +	   * The correct way is to follow kernel/U-boot to iterate item by
> +	   * item, without any assumption on the file offset continuity.
> +	   *
> +	   * Here we just manually skip to next item and re-do the verification.
> +	   *
> +	   * TODO: Rework the whole extent item iteration code, if not the
> +	   * whole btrfs implementation.
> +	   */
>   	  if (data->extend <= pos)
>   	    {
> -	      grub_error (GRUB_ERR_BAD_FS, "extent not found");
> -	      return -1;
> +	      err = next(data, &desc, &elemaddr, &elemsize, &key_out);
> +	      if (err < 0)
> +		return -1;
> +	      /* No next item for the inode, we hit the end */
> +	      if (err == 0 || key_out.object_id != ino ||
> +		  key_out.type != GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM)
> +		      return pos - pos0;
> +
> +	      csize = grub_le_to_cpu64(key_out.offset) - pos;
> +	      buf += csize;
> +	      pos += csize;
> +	      len -= csize;
> +	      continue;
>   	    }
>   	}
>         csize = data->extend - pos;
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/btrfs: Make extent item iteration to handle gaps
  2021-10-28  7:36 ` Qu Wenruo
@ 2021-10-28  9:27   ` Michael Chang
  2021-10-28  9:40     ` Qu Wenruo
  2021-10-29 12:52   ` David Sterba
  1 sibling, 1 reply; 8+ messages in thread
From: Michael Chang @ 2021-10-28  9:27 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: Qu Wenruo, linux-btrfs

On Thu, Oct 28, 2021 at 03:36:10PM +0800, The development of GNU GRUB wrote:
> Gentle ping?
> 
> Without this patch, the new mkfs.btrfs NO_HOLES feature would break any
> kernel/initramfs with hole in it.
> 
> And considering the modification is already small, I believe this patch is
> definitely worthy as a bug fix.

+1

This does worth more attention as booting would have been blocked.

> 
> Thanks,
> Qu
> 
> On 2021/10/16 09:40, Qu Wenruo wrote:
> > [BUG]
> > Grub btrfs implementation can't handle two very basic btrfs file
> > layouts:
> > 
> > 1. Mixed inline/regualr extents
> >     # mkfs.btrfs -f test.img
> >     # mount test.img /mnt/btrfs
> >     # xfs_io -f -c "pwrite 0 1k" -c "sync" -c "falloc 0 4k" \
> > 	       -c "pwrite 4k 4k" /mnt/btrfs/file
> >     # umount /mnt/btrfs
> >     # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"
> > 
> >     Such mixed inline/regular extents case is not recommended layout,
> >     but all existing tools and kernel can handle it without problem
> > 
> > 2. NO_HOLES feature
> >     # mkfs.btrfs -f test.img -O no_holes
> >     # mount test.img /mnt/btrfs
> >     # xfs_io -f -c "pwrite 0 4k" -c "pwrite 8k 4k" /mnt/btrfs/file
> >     # umount /mnt/btrfs
> >     # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"
> > 
> >     NO_HOLES feature is going to be the default mkfs feature in the incoming
> >     v5.15 release, and kernel has support for it since v4.0.
> > 
> > [CAUSE]
> > The way GRUB btrfs code iterates through file extents relies on no gap
> > between extents.
> > 
> > If any gap is hit, then grub btrfs will error out, without any proper
> > reason to help debug the bug.
> > 
> > This is a bad assumption, since a long long time ago btrfs has a new
> > feature called NO_HOLES to allow btrfs to skip the padding hole extent
> > to reduce metadata usage.
> > 
> > The NO_HOLES feature is already stable since kernel v4.0 and is going to
> > be the default mkfs feature in the incoming v5.15 btrfs-progs release.
> > 
> > [FIX]
> > When there is a extent gap, instead of error out, just try next item.
> > 
> > This is still not ideal, as kernel/progs/U-boot all do the iteration
> > item by item, not relying on the file offset continuity.
> > 
> > But it will be way more time consuming to correct the whole behavior
> > than starting from scratch to build a proper designed btrfs module for GRUB.
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >   grub-core/fs/btrfs.c | 33 ++++++++++++++++++++++++++++++---
> >   1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> > index 63203034dfc6..4fbcbec7524a 100644
> > --- a/grub-core/fs/btrfs.c
> > +++ b/grub-core/fs/btrfs.c
> > @@ -1443,6 +1443,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
> >         grub_size_t csize;
> >         grub_err_t err;
> >         grub_off_t extoff;
> > +      struct grub_btrfs_leaf_descriptor desc;
> >         if (!data->extent || data->extstart > pos || data->extino != ino
> >   	  || data->exttree != tree || data->extend <= pos)
> >   	{
> > @@ -1455,7 +1456,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
> >   	  key_in.type = GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM;
> >   	  key_in.offset = grub_cpu_to_le64 (pos);
> >   	  err = lower_bound (data, &key_in, &key_out, tree,
> > -			     &elemaddr, &elemsize, NULL, 0);
> > +			     &elemaddr, &elemsize, &desc, 0);
> >   	  if (err)
> >   	    return -1;
> >   	  if (key_out.object_id != ino
> > @@ -1494,10 +1495,36 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
> >   			PRIxGRUB_UINT64_T "\n",
> >   			grub_le_to_cpu64 (key_out.offset),
> >   			grub_le_to_cpu64 (data->extent->size));
> > +	  /*
> > +	   * The way of extent item iteration is pretty bad, it completely
> > +	   * requires all extents are contiguous, which is not ensured.
> > +	   *
> > +	   * Features like NO_HOLE and mixed inline/regular extents can cause
> > +	   * gaps between file extent items.
> > +	   *
> > +	   * The correct way is to follow kernel/U-boot to iterate item by
> > +	   * item, without any assumption on the file offset continuity.
> > +	   *
> > +	   * Here we just manually skip to next item and re-do the verification.
> > +	   *
> > +	   * TODO: Rework the whole extent item iteration code, if not the
> > +	   * whole btrfs implementation.
> > +	   */
> >   	  if (data->extend <= pos)
> >   	    {
> > -	      grub_error (GRUB_ERR_BAD_FS, "extent not found");
> > -	      return -1;
> > +	      err = next(data, &desc, &elemaddr, &elemsize, &key_out);
> > +	      if (err < 0)
> > +		return -1;
> > +	      /* No next item for the inode, we hit the end */
> > +	      if (err == 0 || key_out.object_id != ino ||
> > +		  key_out.type != GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM)
> > +		      return pos - pos0;
> > +
> > +	      csize = grub_le_to_cpu64(key_out.offset) - pos;
> > +	      buf += csize;
> > +	      pos += csize;
> > +	      len -= csize;

Does it make sense to add some sort of range check here to safegard len
from being underflow ? My justfication is that csize value is read out
from disk so can be anything wild presumably.

Thanks,
Michael


> > +	      continue;
> >   	    }
> >   	}
> >         csize = data->extend - pos;
> > 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/btrfs: Make extent item iteration to handle gaps
  2021-10-28  9:27   ` Michael Chang
@ 2021-10-28  9:40     ` Qu Wenruo
  0 siblings, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-10-28  9:40 UTC (permalink / raw)
  To: Michael Chang, The development of GNU GRUB; +Cc: Qu Wenruo, linux-btrfs



On 2021/10/28 17:27, Michael Chang wrote:
> On Thu, Oct 28, 2021 at 03:36:10PM +0800, The development of GNU GRUB wrote:
>> Gentle ping?
>>
>> Without this patch, the new mkfs.btrfs NO_HOLES feature would break any
>> kernel/initramfs with hole in it.
>>
>> And considering the modification is already small, I believe this patch is
>> definitely worthy as a bug fix.
>
> +1
>
> This does worth more attention as booting would have been blocked.
>
>>
>> Thanks,
>> Qu
>>
>> On 2021/10/16 09:40, Qu Wenruo wrote:
>>> [BUG]
>>> Grub btrfs implementation can't handle two very basic btrfs file
>>> layouts:
>>>
>>> 1. Mixed inline/regualr extents
>>>      # mkfs.btrfs -f test.img
>>>      # mount test.img /mnt/btrfs
>>>      # xfs_io -f -c "pwrite 0 1k" -c "sync" -c "falloc 0 4k" \
>>> 	       -c "pwrite 4k 4k" /mnt/btrfs/file
>>>      # umount /mnt/btrfs
>>>      # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"
>>>
>>>      Such mixed inline/regular extents case is not recommended layout,
>>>      but all existing tools and kernel can handle it without problem
>>>
>>> 2. NO_HOLES feature
>>>      # mkfs.btrfs -f test.img -O no_holes
>>>      # mount test.img /mnt/btrfs
>>>      # xfs_io -f -c "pwrite 0 4k" -c "pwrite 8k 4k" /mnt/btrfs/file
>>>      # umount /mnt/btrfs
>>>      # ./grub-fstest ./grub-fstest --debug=btrfs ~/test.img hex "/file"
>>>
>>>      NO_HOLES feature is going to be the default mkfs feature in the incoming
>>>      v5.15 release, and kernel has support for it since v4.0.
>>>
>>> [CAUSE]
>>> The way GRUB btrfs code iterates through file extents relies on no gap
>>> between extents.
>>>
>>> If any gap is hit, then grub btrfs will error out, without any proper
>>> reason to help debug the bug.
>>>
>>> This is a bad assumption, since a long long time ago btrfs has a new
>>> feature called NO_HOLES to allow btrfs to skip the padding hole extent
>>> to reduce metadata usage.
>>>
>>> The NO_HOLES feature is already stable since kernel v4.0 and is going to
>>> be the default mkfs feature in the incoming v5.15 btrfs-progs release.
>>>
>>> [FIX]
>>> When there is a extent gap, instead of error out, just try next item.
>>>
>>> This is still not ideal, as kernel/progs/U-boot all do the iteration
>>> item by item, not relying on the file offset continuity.
>>>
>>> But it will be way more time consuming to correct the whole behavior
>>> than starting from scratch to build a proper designed btrfs module for GRUB.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>    grub-core/fs/btrfs.c | 33 ++++++++++++++++++++++++++++++---
>>>    1 file changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>>> index 63203034dfc6..4fbcbec7524a 100644
>>> --- a/grub-core/fs/btrfs.c
>>> +++ b/grub-core/fs/btrfs.c
>>> @@ -1443,6 +1443,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
>>>          grub_size_t csize;
>>>          grub_err_t err;
>>>          grub_off_t extoff;
>>> +      struct grub_btrfs_leaf_descriptor desc;
>>>          if (!data->extent || data->extstart > pos || data->extino != ino
>>>    	  || data->exttree != tree || data->extend <= pos)
>>>    	{
>>> @@ -1455,7 +1456,7 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
>>>    	  key_in.type = GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM;
>>>    	  key_in.offset = grub_cpu_to_le64 (pos);
>>>    	  err = lower_bound (data, &key_in, &key_out, tree,
>>> -			     &elemaddr, &elemsize, NULL, 0);
>>> +			     &elemaddr, &elemsize, &desc, 0);
>>>    	  if (err)
>>>    	    return -1;
>>>    	  if (key_out.object_id != ino
>>> @@ -1494,10 +1495,36 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
>>>    			PRIxGRUB_UINT64_T "\n",
>>>    			grub_le_to_cpu64 (key_out.offset),
>>>    			grub_le_to_cpu64 (data->extent->size));
>>> +	  /*
>>> +	   * The way of extent item iteration is pretty bad, it completely
>>> +	   * requires all extents are contiguous, which is not ensured.
>>> +	   *
>>> +	   * Features like NO_HOLE and mixed inline/regular extents can cause
>>> +	   * gaps between file extent items.
>>> +	   *
>>> +	   * The correct way is to follow kernel/U-boot to iterate item by
>>> +	   * item, without any assumption on the file offset continuity.
>>> +	   *
>>> +	   * Here we just manually skip to next item and re-do the verification.
>>> +	   *
>>> +	   * TODO: Rework the whole extent item iteration code, if not the
>>> +	   * whole btrfs implementation.
>>> +	   */
>>>    	  if (data->extend <= pos)
>>>    	    {
>>> -	      grub_error (GRUB_ERR_BAD_FS, "extent not found");
>>> -	      return -1;
>>> +	      err = next(data, &desc, &elemaddr, &elemsize, &key_out);
>>> +	      if (err < 0)
>>> +		return -1;
>>> +	      /* No next item for the inode, we hit the end */
>>> +	      if (err == 0 || key_out.object_id != ino ||
>>> +		  key_out.type != GRUB_BTRFS_ITEM_TYPE_EXTENT_ITEM)
>>> +		      return pos - pos0;
>>> +
>>> +	      csize = grub_le_to_cpu64(key_out.offset) - pos;
>>> +	      buf += csize;
>>> +	      pos += csize;
>>> +	      len -= csize;
>
> Does it make sense to add some sort of range check here to safegard len
> from being underflow ?

Yes, thanks for catching this.

> My justfication is that csize value is read out
> from disk so can be anything wild presumably.

It can be even more easier to trigger the underflow.

If the read range ends inside a hole, and NO_HOLES feature is enabled,
then csize will be larger than len, and underflow.

Thankfully for GRUB it always read the whole file, not part of it.
So it's fine for GRUB, but definitely not for a generic fs implementation.

So I'll update the patch to do a clamp on the size.

Thanks,
Qu

>
> Thanks,
> Michael
>
>
>>> +	      continue;
>>>    	    }
>>>    	}
>>>          csize = data->extend - pos;
>>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/btrfs: Make extent item iteration to handle gaps
  2021-10-28  7:36 ` Qu Wenruo
  2021-10-28  9:27   ` Michael Chang
@ 2021-10-29 12:52   ` David Sterba
  2021-11-02 14:53     ` Daniel Kiper
  1 sibling, 1 reply; 8+ messages in thread
From: David Sterba @ 2021-10-29 12:52 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: grub-devel, linux-btrfs

On Thu, Oct 28, 2021 at 03:36:10PM +0800, Qu Wenruo wrote:
> Gentle ping?
> 
> Without this patch, the new mkfs.btrfs NO_HOLES feature would break any 
> kernel/initramfs with hole in it.

Just to clarify, it's not a new feature, it's been out for a long time
(since 3.14).  The new thing is that it's going to be enabled by default
so the potential impact would be higher.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/btrfs: Make extent item iteration to handle gaps
  2021-10-29 12:52   ` David Sterba
@ 2021-11-02 14:53     ` Daniel Kiper
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Kiper @ 2021-11-02 14:53 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, grub-devel, linux-btrfs

On Fri, Oct 29, 2021 at 02:52:29PM +0200, David Sterba wrote:
> On Thu, Oct 28, 2021 at 03:36:10PM +0800, Qu Wenruo wrote:
> > Gentle ping?
> >
> > Without this patch, the new mkfs.btrfs NO_HOLES feature would break any
> > kernel/initramfs with hole in it.
>
> Just to clarify, it's not a new feature, it's been out for a long time
> (since 3.14).  The new thing is that it's going to be enabled by default
> so the potential impact would be higher.

I will take a look at this patch probably next week.

Daniel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] fs/btrfs: Make extent item iteration to handle gaps
@ 2021-10-18 10:51 David Disseldorp
  0 siblings, 0 replies; 8+ messages in thread
From: David Disseldorp @ 2021-10-18 10:51 UTC (permalink / raw)
  To: wqu; +Cc: grub-devel

Tested-by: David Disseldorp <ddiss@suse.de>

This fixes the failures seen with the Dracut reflinked initramfs
patchset at https://github.com/dracutdevs/dracut/pull/1531 .

Thanks, David


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/btrfs: Make extent item iteration to handle gaps
       [not found] <20211018124903.661dca99@suse.de>
@ 2021-10-18 10:51 ` Qu Wenruo
  0 siblings, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-10-18 10:51 UTC (permalink / raw)
  To: David Disseldorp, The development of GNU GRUB; +Cc: linux-btrfs



On 2021/10/18 18:49, David Disseldorp wrote:
> Tested-by: David Disseldorp <ddiss@suse.de>
> 
> This fixes the failures seen with the Dracut reflinked initramfs
> patchset at https://github.com/dracutdevs/dracut/pull/1531 .
> 
> Thanks, David
> 
Missing CC to grub mail list I guess?

Thanks,
Qu


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-11-02 15:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-16  1:40 [PATCH] fs/btrfs: Make extent item iteration to handle gaps Qu Wenruo
2021-10-28  7:36 ` Qu Wenruo
2021-10-28  9:27   ` Michael Chang
2021-10-28  9:40     ` Qu Wenruo
2021-10-29 12:52   ` David Sterba
2021-11-02 14:53     ` Daniel Kiper
     [not found] <20211018124903.661dca99@suse.de>
2021-10-18 10:51 ` Qu Wenruo
2021-10-18 10:51 David Disseldorp

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.