2019-07-23 16:12:14

by Vasily Averin

Subject: [PATCH] fuse: BUG_ON's correction in fuse_dev_splice_write()

commit 963545357202 ("fuse: reduce allocation size for splice_write")
changed size of bufs array, so first BUG_ON should be corrected too.
Second BUG_ON become useless, first one also includes the second check:
any unsigned nbuf value cannot be less than 0.

Fixes: 963545357202 ("fuse: reduce allocation size for splice_write")
Signed-off-by: Vasily Averin <[email protected]>
---
fs/fuse/dev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index ea8237513dfa..311c7911271c 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2064,8 +2064,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
struct pipe_buffer *ibuf;
struct pipe_buffer *obuf;

- BUG_ON(nbuf >= pipe->buffers);
- BUG_ON(!pipe->nrbufs);
+ BUG_ON(nbuf >= pipe->nrbufs);
ibuf = &pipe->bufs[pipe->curbuf];
obuf = &bufs[nbuf];

--
2.17.1

2019-08-01 11:09:31

by Miklos Szeredi

Subject: Re: [PATCH] fuse: BUG_ON's correction in fuse_dev_splice_write()

On Tue, Jul 23, 2019 at 8:33 AM Vasily Averin <[email protected]> wrote:
>
> commit 963545357202 ("fuse: reduce allocation size for splice_write")
> changed size of bufs array, so first BUG_ON should be corrected too.
> Second BUG_ON become useless, first one also includes the second check:
> any unsigned nbuf value cannot be less than 0.

This patch seems broken: it assumes that pipe->nrbufs doesn't change.
Have you actually tested it?

Thanks,
Miklos

2019-08-19 06:53:53

by Vasily Averin

Subject: Re: [PATCH] fuse: BUG_ON's correction in fuse_dev_splice_write()

On 8/1/19 2:01 PM, Miklos Szeredi wrote:
> On Tue, Jul 23, 2019 at 8:33 AM Vasily Averin <[email protected]> wrote:
>>
>> commit 963545357202 ("fuse: reduce allocation size for splice_write")
>> changed size of bufs array, so first BUG_ON should be corrected too.
>> Second BUG_ON become useless, first one also includes the second check:
>> any unsigned nbuf value cannot be less than 0.
>
> This patch seems broken: it assumes that pipe->nrbufs doesn't change.
> Have you actually tested it?

You're right, I've missed it.
I've prepared second patch version which fixes first BUG_ON only.
checkpatch.pl also advises to replace BUG_ONs to WARN_ONs and 'unsigned' to 'unsigned int'
however I'm don't understand what it's better here:
- keep all as is,
- or merge all changes together,
- or do it in separate patches,
- or do something else?

I believe it makes sense to remove BUG_ONs in separate patch, or may be merge it with current one,
but I do not like an idea to fight against bare 'unsigned' in fuse.

Could you please comment it?

Thank you,
Vasily Averin

2019-08-19 06:55:22

by Vasily Averin

Subject: [PATCH] fuse: BUG_ON correction in fuse_dev_splice_write()

commit 963545357202 ("fuse: reduce allocation size for splice_write")
changed size of bufs array, so BUG_ON which checks the index of the array
shold also be fixed.

Fixes: 963545357202 ("fuse: reduce allocation size for splice_write")
Signed-off-by: Vasily Averin <[email protected]>
---
fs/fuse/dev.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index ea8237513dfa..f4ef6e01642c 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2029,7 +2029,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
struct file *out, loff_t *ppos,
size_t len, unsigned int flags)
{
- unsigned nbuf;
+ unsigned nbuf, bsize;
unsigned idx;
struct pipe_buffer *bufs;
struct fuse_copy_state cs;
@@ -2043,7 +2043,8 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,

pipe_lock(pipe);

- bufs = kvmalloc_array(pipe->nrbufs, sizeof(struct pipe_buffer),
+ bsize = pipe->nrbufs;
+ bufs = kvmalloc_array(bsize, sizeof(struct pipe_buffer),
GFP_KERNEL);
if (!bufs) {
pipe_unlock(pipe);
@@ -2064,7 +2065,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
struct pipe_buffer *ibuf;
struct pipe_buffer *obuf;

- BUG_ON(nbuf >= pipe->buffers);
+ BUG_ON(nbuf >= bsize);
BUG_ON(!pipe->nrbufs);
ibuf = &pipe->bufs[pipe->curbuf];
obuf = &bufs[nbuf];
--
2.17.1

2019-10-03 08:05:28

by Miklos Szeredi

Subject: Re: [PATCH] fuse: BUG_ON correction in fuse_dev_splice_write()

On Mon, Aug 19, 2019 at 8:53 AM Vasily Averin <[email protected]> wrote:
>
> commit 963545357202 ("fuse: reduce allocation size for splice_write")
> changed size of bufs array, so BUG_ON which checks the index of the array
> shold also be fixed.
>
> Fixes: 963545357202 ("fuse: reduce allocation size for splice_write")
> Signed-off-by: Vasily Averin <[email protected]>
> ---
> fs/fuse/dev.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index ea8237513dfa..f4ef6e01642c 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2029,7 +2029,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> struct file *out, loff_t *ppos,
> size_t len, unsigned int flags)
> {
> - unsigned nbuf;
> + unsigned nbuf, bsize;
> unsigned idx;
> struct pipe_buffer *bufs;
> struct fuse_copy_state cs;
> @@ -2043,7 +2043,8 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
>
> pipe_lock(pipe);
>
> - bufs = kvmalloc_array(pipe->nrbufs, sizeof(struct pipe_buffer),
> + bsize = pipe->nrbufs;
> + bufs = kvmalloc_array(bsize, sizeof(struct pipe_buffer),
> GFP_KERNEL);
> if (!bufs) {
> pipe_unlock(pipe);
> @@ -2064,7 +2065,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> struct pipe_buffer *ibuf;
> struct pipe_buffer *obuf;
>
> - BUG_ON(nbuf >= pipe->buffers);
> + BUG_ON(nbuf >= bsize);
> BUG_ON(!pipe->nrbufs);

Better turn these into WARN_ON's.. Fixed and applied.

Thanks,
Miklos