* [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()
@ 2021-08-18 16:08 Sidong Yang
2021-08-18 22:24 ` David Sterba
2021-08-19 8:04 ` Nikolay Borisov
0 siblings, 2 replies; 7+ messages in thread
From: Sidong Yang @ 2021-08-18 16:08 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()
2021-08-18 16:08 [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same() Sidong Yang
@ 2021-08-18 22:24 ` David Sterba
2021-08-18 23:22 ` Sidong Yang
2021-08-19 8:04 ` Nikolay Borisov
1 sibling, 1 reply; 7+ messages in thread
From: David Sterba @ 2021-08-18 22:24 UTC (permalink / raw)
To: Sidong Yang; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()
2021-08-18 22:24 ` David Sterba
@ 2021-08-18 23:22 ` Sidong Yang
0 siblings, 0 replies; 7+ messages in thread
From: Sidong Yang @ 2021-08-18 23:22 UTC (permalink / raw)
To: dsterba, linux-btrfs
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()
2021-08-18 16:08 [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same() Sidong Yang
2021-08-18 22:24 ` David Sterba
@ 2021-08-19 8:04 ` Nikolay Borisov
2021-08-19 15:32 ` Sidong Yang
1 sibling, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2021-08-19 8:04 UTC (permalink / raw)
To: Sidong Yang, linux-btrfs
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,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()
2021-08-19 8:04 ` Nikolay Borisov
@ 2021-08-19 15:32 ` Sidong Yang
2021-08-19 18:12 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Sidong Yang @ 2021-08-19 15:32 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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,
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()
2021-08-19 15:32 ` Sidong Yang
@ 2021-08-19 18:12 ` David Sterba
2021-08-20 0:32 ` Sidong Yang
0 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2021-08-19 18:12 UTC (permalink / raw)
To: Sidong Yang; +Cc: Nikolay Borisov, linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same()
2021-08-19 18:12 ` David Sterba
@ 2021-08-20 0:32 ` Sidong Yang
0 siblings, 0 replies; 7+ messages in thread
From: Sidong Yang @ 2021-08-20 0:32 UTC (permalink / raw)
To: dsterba, Nikolay Borisov, linux-btrfs
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-20 0:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 16:08 [PATCH] btrfs: reflink: Assure length != 0 in btrfs_extent_same() Sidong Yang
2021-08-18 22:24 ` David Sterba
2021-08-18 23:22 ` Sidong Yang
2021-08-19 8:04 ` Nikolay Borisov
2021-08-19 15:32 ` Sidong Yang
2021-08-19 18:12 ` David Sterba
2021-08-20 0:32 ` Sidong Yang
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.