[PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[Sidong Yang]

btrfs_extent_same() cannot be called with zero length. Because when
length is zero, it would be filtered by condition in
btrfs_remap_file_range(). But if this function is used in other case in
future, it can make ret as uninitialized.

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
 fs/btrfs/reflink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index 9b0814318e72..69eb50f2f0b4 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -653,6 +653,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	u64 i, tail_len, chunk_count;
 	struct btrfs_root *root_dst = BTRFS_I(dst)->root;
 
+	ASSERT(olen);
 	spin_lock(&root_dst->root_item_lock);
 	if (root_dst->send_in_progress) {
 		btrfs_warn_rl(root_dst->fs_info,
-- 
2.25.1

Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[David Sterba]

On Wed, Aug 18, 2021 at 04:08:15PM +0000, Sidong Yang wrote:
> btrfs_extent_same() cannot be called with zero length. Because when
> length is zero, it would be filtered by condition in
> btrfs_remap_file_range(). But if this function is used in other case in
> future, it can make ret as uninitialized.

Do you have a specific future in mind? Adding the assert won't hurt so
ok.

Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[Sidong Yang]

On Thu, Aug 19, 2021 at 12:24:47AM +0200, David Sterba wrote:
> On Wed, Aug 18, 2021 at 04:08:15PM +0000, Sidong Yang wrote:
> > btrfs_extent_same() cannot be called with zero length. Because when
> > length is zero, it would be filtered by condition in
> > btrfs_remap_file_range(). But if this function is used in other case in
> > future, it can make ret as uninitialized.
> 
> Do you have a specific future in mind? Adding the assert won't hurt so
> ok.

No, sorry, I just want to make it safe. is there any way to better than
adding assert? Would it better to initialize ret?

Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[Nikolay Borisov]

On 18.08.21 г. 19:08, Sidong Yang wrote:
> btrfs_extent_same() cannot be called with zero length. Because when
> length is zero, it would be filtered by condition in
> btrfs_remap_file_range(). But if this function is used in other case in
> future, it can make ret as uninitialized.
> 
> Signed-off-by: Sidong Yang <realwakka@gmail.com>

This is not sufficient, with the assert compiled out the error would
still be in place. It seem that it is sufficient to initialize ret to
some non-arbitrary value i.e -EINVAL ?

> ---
>  fs/btrfs/reflink.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> index 9b0814318e72..69eb50f2f0b4 100644
> --- a/fs/btrfs/reflink.c
> +++ b/fs/btrfs/reflink.c
> @@ -653,6 +653,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
>  	u64 i, tail_len, chunk_count;
>  	struct btrfs_root *root_dst = BTRFS_I(dst)->root;
>  
> +	ASSERT(olen);
>  	spin_lock(&root_dst->root_item_lock);
>  	if (root_dst->send_in_progress) {
>  		btrfs_warn_rl(root_dst->fs_info,
> 

Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[Sidong Yang]

On Thu, Aug 19, 2021 at 11:04:58AM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.08.21 г. 19:08, Sidong Yang wrote:
> > btrfs_extent_same() cannot be called with zero length. Because when
> > length is zero, it would be filtered by condition in
> > btrfs_remap_file_range(). But if this function is used in other case in
> > future, it can make ret as uninitialized.
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> 
> This is not sufficient, with the assert compiled out the error would
> still be in place. It seem that it is sufficient to initialize ret to
> some non-arbitrary value i.e -EINVAL ?

I agree. It's better way to assign intial value than adding assert. If
there is code that initialize ret, It seems that assert is no need for
this.
> 
> > ---
> >  fs/btrfs/reflink.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> > index 9b0814318e72..69eb50f2f0b4 100644
> > --- a/fs/btrfs/reflink.c
> > +++ b/fs/btrfs/reflink.c
> > @@ -653,6 +653,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
> >  	u64 i, tail_len, chunk_count;
> >  	struct btrfs_root *root_dst = BTRFS_I(dst)->root;
> >  
> > +	ASSERT(olen);
> >  	spin_lock(&root_dst->root_item_lock);
> >  	if (root_dst->send_in_progress) {
> >  		btrfs_warn_rl(root_dst->fs_info,
> > 

Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[David Sterba]

On Thu, Aug 19, 2021 at 03:32:16PM +0000, Sidong Yang wrote:
> On Thu, Aug 19, 2021 at 11:04:58AM +0300, Nikolay Borisov wrote:
> > 
> > 
> > On 18.08.21 г. 19:08, Sidong Yang wrote:
> > > btrfs_extent_same() cannot be called with zero length. Because when
> > > length is zero, it would be filtered by condition in
> > > btrfs_remap_file_range(). But if this function is used in other case in
> > > future, it can make ret as uninitialized.
> > > 
> > > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > 
> > This is not sufficient, with the assert compiled out the error would
> > still be in place. It seem that it is sufficient to initialize ret to
> > some non-arbitrary value i.e -EINVAL ?
> 
> I agree. It's better way to assign intial value than adding assert. If
> there is code that initialize ret, It seems that assert is no need for
> this.

Patch with assert removed, please send the one initializing the return
value, thanks.

Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()

[Sidong Yang]

On Thu, Aug 19, 2021 at 08:12:49PM +0200, David Sterba wrote:
> On Thu, Aug 19, 2021 at 03:32:16PM +0000, Sidong Yang wrote:
> > On Thu, Aug 19, 2021 at 11:04:58AM +0300, Nikolay Borisov wrote:
> > > 
> > > 
> > > On 18.08.21 г. 19:08, Sidong Yang wrote:
> > > > btrfs_extent_same() cannot be called with zero length. Because when
> > > > length is zero, it would be filtered by condition in
> > > > btrfs_remap_file_range(). But if this function is used in other case in
> > > > future, it can make ret as uninitialized.
> > > > 
> > > > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > > 
> > > This is not sufficient, with the assert compiled out the error would
> > > still be in place. It seem that it is sufficient to initialize ret to
> > > some non-arbitrary value i.e -EINVAL ?
> > 
> > I agree. It's better way to assign intial value than adding assert. If
> > there is code that initialize ret, It seems that assert is no need for
> > this.
> 
> Patch with assert removed, please send the one initializing the return
> value, thanks.

Sure, I'll write it in v2.

Thanks,
Sidong