* [PATCH 1/2] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs.
@ 2018-07-16 16:03 Andrey Ryabinin
2018-07-16 16:03 ` [PATCH 2/2] fs/fuse, splice_write: reduce allocation size Andrey Ryabinin
0 siblings, 1 reply; 9+ messages in thread
From: Andrey Ryabinin @ 2018-07-16 16:03 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] fs/fuse, splice_write: reduce allocation size.
2018-07-16 16:03 [PATCH 1/2] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs Andrey Ryabinin
@ 2018-07-16 16:03 ` Andrey Ryabinin
2018-07-17 14:47 ` Miklos Szeredi
0 siblings, 1 reply; 9+ messages in thread
From: Andrey Ryabinin @ 2018-07-16 16:03 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] fs/fuse, splice_write: reduce allocation size.
2018-07-16 16:03 ` [PATCH 2/2] fs/fuse, splice_write: reduce allocation size Andrey Ryabinin
@ 2018-07-17 14:47 ` Miklos Szeredi
2018-07-17 15:45 ` Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Andrey Ryabinin
0 siblings, 2 replies; 9+ messages in thread
From: Miklos Szeredi @ 2018-07-17 14:47 UTC (permalink / raw)
To: Andrey Ryabinin; +Cc: linux-fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] fs/fuse, splice_write: reduce allocation size.
2018-07-17 14:47 ` Miklos Szeredi
@ 2018-07-17 15:45 ` Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Andrey Ryabinin
1 sibling, 0 replies; 9+ messages in thread
From: Andrey Ryabinin @ 2018-07-17 15:45 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock()
2018-07-17 14:47 ` Miklos Szeredi
2018-07-17 15:45 ` Andrey Ryabinin
@ 2018-07-17 16:00 ` Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 2/3] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs Andrey Ryabinin
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: Andrey Ryabinin @ 2018-07-17 16:00 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel, Andrey Ryabinin, stable
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs.
2018-07-17 16:00 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Andrey Ryabinin
@ 2018-07-17 16:00 ` Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 3/3] fs/fuse, splice_write: reduce allocation size Andrey Ryabinin
2019-06-12 8:57 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Vlastimil Babka
2 siblings, 0 replies; 9+ messages in thread
From: Andrey Ryabinin @ 2018-07-17 16:00 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] fs/fuse, splice_write: reduce allocation size.
2018-07-17 16:00 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 2/3] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs Andrey Ryabinin
@ 2018-07-17 16:00 ` Andrey Ryabinin
2019-06-12 8:57 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Vlastimil Babka
2 siblings, 0 replies; 9+ messages in thread
From: Andrey Ryabinin @ 2018-07-17 16:00 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock()
2018-07-17 16:00 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 2/3] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 3/3] fs/fuse, splice_write: reduce allocation size Andrey Ryabinin
@ 2019-06-12 8:57 ` Vlastimil Babka
2019-06-13 9:10 ` Andrey Ryabinin
2 siblings, 1 reply; 9+ messages in thread
From: Vlastimil Babka @ 2019-06-12 8:57 UTC (permalink / raw)
To: Andrey Ryabinin, Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel, stable
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++)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock()
2019-06-12 8:57 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Vlastimil Babka
@ 2019-06-13 9:10 ` Andrey Ryabinin
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Ryabinin @ 2019-06-13 9:10 UTC (permalink / raw)
To: Vlastimil Babka, Miklos Szeredi; +Cc: linux-fsdevel, linux-kernel, stable
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++)
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-13 15:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-16 16:03 [PATCH 1/2] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs Andrey Ryabinin
2018-07-16 16:03 ` [PATCH 2/2] fs/fuse, splice_write: reduce allocation size Andrey Ryabinin
2018-07-17 14:47 ` Miklos Szeredi
2018-07-17 15:45 ` Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 2/3] fs/fuse, splice: use kvmalloc to allocate array of pipe_buffer structs Andrey Ryabinin
2018-07-17 16:00 ` [PATCH v2 3/3] fs/fuse, splice_write: reduce allocation size Andrey Ryabinin
2019-06-12 8:57 ` [PATCH v2 1/3] fs/fuse, splice_write: Don't access pipe->buffers without pipe_lock() Vlastimil Babka
2019-06-13 9:10 ` Andrey Ryabinin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).