[PATCH 1/2] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs.

[PATCH 1/2] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs.

[Andrey Ryabinin]

The amount of pipe->buffers is basically controlled by userspace by
fcntl(... F_SETPIPE_SZ ...) so it could be large. High order allocations
could be slow (if memory is heavily fragmented) or may fail if the order
is larger than PAGE_ALLOC_COSTLY_ORDER.

Since the 'bufs' doesn't need to be physically contiguous, use
the kvmalloc_array() to allocate memory. If high order
page isn't available, the kvamalloc*() will fallback to 0-order.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/fuse/dev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index c6b88fa85e2e..74900571546d 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1362,8 +1362,8 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	if (!fud)
 		return -EPERM;
 
-	bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
-			     GFP_KERNEL);
+	bufs = kvmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
+			      GFP_KERNEL);
 	if (!bufs)
 		return -ENOMEM;
 
@@ -1396,7 +1396,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	for (; page_nr < cs.nr_segs; page_nr++)
 		put_page(bufs[page_nr].page);
 
-	kfree(bufs);
+	kvfree(bufs);
 	return ret;
 }
 
@@ -1944,8 +1944,8 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 	if (!fud)
 		return -EPERM;
 
-	bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
-			     GFP_KERNEL);
+	bufs = kvmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
+			      GFP_KERNEL);
 	if (!bufs)
 		return -ENOMEM;
 
@@ -2003,7 +2003,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 		pipe_buf_release(pipe, &bufs[idx]);
 
 out:
-	kfree(bufs);
+	kvfree(bufs);
 	return ret;
 }
 
-- 
2.16.4

[PATCH 2/2] fs/fuse, splice_write: reduce allocation size.

[Andrey Ryabinin]

The 'bufs' array contains 'pipe->buffers' elements, but the
fuse_dev_splice_write() uses only 'pipe->nrbufs' elements.

So reduce the allocation size to 'pipe->nrbufs' elements.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/fuse/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 74900571546d..39789f070cde 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1944,7 +1944,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 	if (!fud)
 		return -EPERM;
 
-	bufs = kvmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
+	bufs = kvmalloc_array(pipe->nrbufs, sizeof(struct pipe_buffer),
 			      GFP_KERNEL);
 	if (!bufs)
 		return -ENOMEM;
-- 
2.16.4

Re: [PATCH 2/2] fs/fuse, splice_write: reduce allocation size.

[Miklos Szeredi]

On Mon, Jul 16, 2018 at 6:03 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> The 'bufs' array contains 'pipe->buffers' elements, but the
> fuse_dev_splice_write() uses only 'pipe->nrbufs' elements.

Hmm, only valid with pipe lock held, AFAICS.

True for using ->buffers as well...

Would you mind resending this series with an additional starting patch
that moves the bufs allocations inside pipe_lock()/pipe_unlock() to
fix races with fcntl(F_SETPIPE_SZ).

Thanks,
Miklos

Re: [PATCH 2/2] fs/fuse, splice_write: reduce allocation size.

[Andrey Ryabinin]

On 07/17/2018 05:47 PM, Miklos Szeredi wrote:
> On Mon, Jul 16, 2018 at 6:03 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>> The 'bufs' array contains 'pipe->buffers' elements, but the
>> fuse_dev_splice_write() uses only 'pipe->nrbufs' elements.
> 
> Hmm, only valid with pipe lock held, AFAICS.
> 
> True for using ->buffers as well...
> 
> Would you mind resending this series with an additional starting patch
> that moves the bufs allocations inside pipe_lock()/pipe_unlock() to
> fix races with fcntl(F_SETPIPE_SZ).
> 

Sure, will do shortly.
I suppose the patch should go with a stable tag, right?


> Thanks,
> Miklos
> 

[PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock()

[Andrey Ryabinin]

fuse_dev_splice_write() reads pipe->buffers to determine the size of
'bufs' array before taking the pipe_lock(). This is not safe as
another thread might change the 'pipe->buffers' between the allocation
and taking the pipe_lock(). So we end up with too small 'bufs' array.

Move the bufs allocations inside pipe_lock()/pipe_unlock() to fix this.

Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device")
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: <stable@vger.kernel.org>
---
 fs/fuse/dev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index c6b88fa85e2e..702592cce546 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1944,12 +1944,15 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 	if (!fud)
 		return -EPERM;
 
+	pipe_lock(pipe);
+
 	bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
 			     GFP_KERNEL);
-	if (!bufs)
+	if (!bufs) {
+		pipe_unlock(pipe);
 		return -ENOMEM;
+	}
 
-	pipe_lock(pipe);
 	nbuf = 0;
 	rem = 0;
 	for (idx = 0; idx < pipe->nrbufs && rem < len; idx++)
-- 
2.16.4

[PATCH v2 2/3] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs.

[Andrey Ryabinin]

The amount of pipe->buffers is basically controlled by userspace by
fcntl(... F_SETPIPE_SZ ...) so it could be large. High order allocations
could be slow (if memory is heavily fragmented) or may fail if the order
is larger than PAGE_ALLOC_COSTLY_ORDER.

Since the 'bufs' doesn't need to be physically contiguous, use
the kvmalloc_array() to allocate memory. If high order
page isn't available, the kvamalloc*() will fallback to 0-order.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/fuse/dev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 702592cce546..fd4a838c1673 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1362,8 +1362,8 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	if (!fud)
 		return -EPERM;
 
-	bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
-			     GFP_KERNEL);
+	bufs = kvmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
+			      GFP_KERNEL);
 	if (!bufs)
 		return -ENOMEM;
 
@@ -1396,7 +1396,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	for (; page_nr < cs.nr_segs; page_nr++)
 		put_page(bufs[page_nr].page);
 
-	kfree(bufs);
+	kvfree(bufs);
 	return ret;
 }
 
@@ -1946,8 +1946,8 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 
 	pipe_lock(pipe);
 
-	bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
-			     GFP_KERNEL);
+	bufs = kvmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
+			      GFP_KERNEL);
 	if (!bufs) {
 		pipe_unlock(pipe);
 		return -ENOMEM;
@@ -2006,7 +2006,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 		pipe_buf_release(pipe, &bufs[idx]);
 
 out:
-	kfree(bufs);
+	kvfree(bufs);
 	return ret;
 }
 
-- 
2.16.4

[PATCH v2 3/3] fs/fuse, splice_write: reduce allocation size.

[Andrey Ryabinin]

The 'bufs' array contains 'pipe->buffers' elements, but the
fuse_dev_splice_write() uses only 'pipe->nrbufs' elements.

So reduce the allocation size to 'pipe->nrbufs' elements.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/fuse/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index fd4a838c1673..d78af3c146f9 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1946,7 +1946,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 
 	pipe_lock(pipe);
 
-	bufs = kvmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
+	bufs = kvmalloc_array(pipe->nrbufs, sizeof(struct pipe_buffer),
 			      GFP_KERNEL);
 	if (!bufs) {
 		pipe_unlock(pipe);
-- 
2.16.4

Re: [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock()

[Vlastimil Babka]

On 7/17/18 6:00 PM, Andrey Ryabinin wrote:
> fuse_dev_splice_write() reads pipe->buffers to determine the size of
> 'bufs' array before taking the pipe_lock(). This is not safe as
> another thread might change the 'pipe->buffers' between the allocation
> and taking the pipe_lock(). So we end up with too small 'bufs' array.
> 
> Move the bufs allocations inside pipe_lock()/pipe_unlock() to fix this.
> 
> Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device")
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: <stable@vger.kernel.org>

BTW, why don't we need to do the same in fuse_dev_splice_read()?

Thanks,
Vlastimil

> ---
>  fs/fuse/dev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index c6b88fa85e2e..702592cce546 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -1944,12 +1944,15 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
>  	if (!fud)
>  		return -EPERM;
>  
> +	pipe_lock(pipe);
> +
>  	bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
>  			     GFP_KERNEL);
> -	if (!bufs)
> +	if (!bufs) {
> +		pipe_unlock(pipe);
>  		return -ENOMEM;
> +	}
>  
> -	pipe_lock(pipe);
>  	nbuf = 0;
>  	rem = 0;
>  	for (idx = 0; idx < pipe->nrbufs && rem < len; idx++)
> 

Re: [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock()

[Andrey Ryabinin]

On 6/12/19 11:57 AM, Vlastimil Babka wrote:
> On 7/17/18 6:00 PM, Andrey Ryabinin wrote:
>> fuse_dev_splice_write() reads pipe->buffers to determine the size of
>> 'bufs' array before taking the pipe_lock(). This is not safe as
>> another thread might change the 'pipe->buffers' between the allocation
>> and taking the pipe_lock(). So we end up with too small 'bufs' array.
>>
>> Move the bufs allocations inside pipe_lock()/pipe_unlock() to fix this.
>>
>> Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device")
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Cc: <stable@vger.kernel.org>
> 
> BTW, why don't we need to do the same in fuse_dev_splice_read()?
> 

do_splice() already takes the pipe_lock() before calling ->splice_read()

> Thanks,
> Vlastimil
> 
>> ---
>>  fs/fuse/dev.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index c6b88fa85e2e..702592cce546 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -1944,12 +1944,15 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
>>  	if (!fud)
>>  		return -EPERM;
>>  
>> +	pipe_lock(pipe);
>> +
>>  	bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer),
>>  			     GFP_KERNEL);
>> -	if (!bufs)
>> +	if (!bufs) {
>> +		pipe_unlock(pipe);
>>  		return -ENOMEM;
>> +	}
>>  
>> -	pipe_lock(pipe);
>>  	nbuf = 0;
>>  	rem = 0;
>>  	for (idx = 0; idx < pipe->nrbufs && rem < len; idx++)
>>
>