* [PATCH v2 0/4] pipe: Trivial cleanups
@ 2025-03-07 5:29 K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring() K Prateek Nayak
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: K Prateek Nayak @ 2025-03-07 5:29 UTC (permalink / raw)
To: Linus Torvalds, Oleg Nesterov, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-kernel
Cc: Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal, K Prateek Nayak
Hello folks,
Based on the suggestion on the RFC, the treewide conversion of
references to pipe->{head,tail} from unsigned int to pipe_index_t has
been dropped for now. The series contains trivial cleanup suggested to
limit the nr_slots in pipe_resize_ring() to be covered between
pipe_index_t limits of pipe->{head,tail} and using pipe_buf() to remove
the open-coded usage of masks to access pipe buffer building on Linus'
cleanup of fs/fuse/dev.c in commit ebb0f38bb47f ("fs/pipe: fix pipe
buffer index use in FUSE")
If folks are interested in converting a selective subset of references
to pipe->{head,tail} from unsigned int to pipe_index_t, please comment
on
https://lore.kernel.org/all/20250306113924.20004-4-kprateek.nayak@amd.com/
and I can send out a patch or you can go ahead and send one out yourself
too :)
Changes are based on Linus' tree at commit 00a7d39898c8 ("fs/pipe: add
simpler helpers for common cases")
---
Changelog:
RFC v1..v2:
o Dropped the RFC tag.
o Use (piep_index_t)-1u as the upper limit for nr_slots replacing the
BITS_PER_TYPE() hackery to get the limits (Oleg; Patch 1)
o Patch 2 from the RFC v1 was dropped as it became redundand after the
introduction of pipe_is_full() helper.
o Patch 2-4 are additional cleanups introduced in this version to use
pipe_buf() replacing all the open-coded logic in various subsystems.
(Oleg; Patch 2-4)
RFC: https://lore.kernel.org/all/20250306113924.20004-1-kprateek.nayak@amd.com/
---
K Prateek Nayak (4):
fs/pipe: Limit the slots in pipe_resize_ring()
kernel/watch_queue: Use pipe_buf() to retrieve the pipe buffer
fs/pipe: Use pipe_buf() helper to retrieve pipe buffer
fs/splice: Use pipe_buf() helper to retrieve pipe buffer
fs/pipe.c | 13 +++++++------
fs/splice.c | 40 ++++++++++++++--------------------------
kernel/watch_queue.c | 7 +++----
3 files changed, 24 insertions(+), 36 deletions(-)
base-commit: 00a7d39898c8010bfd5ff62af31ca5db34421b38
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring()
2025-03-07 5:29 [PATCH v2 0/4] pipe: Trivial cleanups K Prateek Nayak
@ 2025-03-07 5:29 ` K Prateek Nayak
2025-03-07 14:51 ` Oleg Nesterov
2025-03-07 5:29 ` [PATCH v2 2/4] kernel/watch_queue: Use pipe_buf() to retrieve the pipe buffer K Prateek Nayak
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: K Prateek Nayak @ 2025-03-07 5:29 UTC (permalink / raw)
To: Linus Torvalds, Oleg Nesterov, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-kernel
Cc: Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal, K Prateek Nayak
Limit the number of slots in pipe_resize_ring() to the maximum value
representable by pipe->{head,tail}. Values beyond the max limit can
lead to incorrect pipe occupancy related calculations where the pipe
will never appear full.
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
Changelog:
RFC v1..v2:
o Use (pipe_index_t)-1u as the limit instead of BITS_PER_TYPE()
hackery. (Oleg)
o Added the "Suggested-by:" tag.
---
fs/pipe.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/pipe.c b/fs/pipe.c
index 4d0799e4e719..88e81f84e3ea 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1271,6 +1271,10 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots)
struct pipe_buffer *bufs;
unsigned int head, tail, mask, n;
+ /* nr_slots larger than limits of pipe->{head,tail} */
+ if (unlikely(nr_slots > (pipe_index_t)-1u))
+ return -EINVAL;
+
bufs = kcalloc(nr_slots, sizeof(*bufs),
GFP_KERNEL_ACCOUNT | __GFP_NOWARN);
if (unlikely(!bufs))
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] kernel/watch_queue: Use pipe_buf() to retrieve the pipe buffer
2025-03-07 5:29 [PATCH v2 0/4] pipe: Trivial cleanups K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring() K Prateek Nayak
@ 2025-03-07 5:29 ` K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 3/4] fs/pipe: Use pipe_buf() helper to retrieve " K Prateek Nayak
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: K Prateek Nayak @ 2025-03-07 5:29 UTC (permalink / raw)
To: Linus Torvalds, Oleg Nesterov, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-kernel
Cc: Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal, K Prateek Nayak
Use pipe_buf() helper to retrieve the pipe buffer in
post_one_notification() replacing the open-coded the logic.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
Changelog:
RFC v1..v2:
o New patch.
---
kernel/watch_queue.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index 5267adeaa403..605129eb61a1 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -101,12 +101,11 @@ static bool post_one_notification(struct watch_queue *wqueue,
struct pipe_inode_info *pipe = wqueue->pipe;
struct pipe_buffer *buf;
struct page *page;
- unsigned int head, tail, mask, note, offset, len;
+ unsigned int head, tail, note, offset, len;
bool done = false;
spin_lock_irq(&pipe->rd_wait.lock);
- mask = pipe->ring_size - 1;
head = pipe->head;
tail = pipe->tail;
if (pipe_full(head, tail, pipe->ring_size))
@@ -124,7 +123,7 @@ static bool post_one_notification(struct watch_queue *wqueue,
memcpy(p + offset, n, len);
kunmap_atomic(p);
- buf = &pipe->bufs[head & mask];
+ buf = pipe_buf(pipe, head);
buf->page = page;
buf->private = (unsigned long)wqueue;
buf->ops = &watch_queue_pipe_buf_ops;
@@ -147,7 +146,7 @@ static bool post_one_notification(struct watch_queue *wqueue,
return done;
lost:
- buf = &pipe->bufs[(head - 1) & mask];
+ buf = pipe_buf(pipe, head - 1);
buf->flags |= PIPE_BUF_FLAG_LOSS;
goto out;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] fs/pipe: Use pipe_buf() helper to retrieve pipe buffer
2025-03-07 5:29 [PATCH v2 0/4] pipe: Trivial cleanups K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring() K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 2/4] kernel/watch_queue: Use pipe_buf() to retrieve the pipe buffer K Prateek Nayak
@ 2025-03-07 5:29 ` K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 4/4] fs/splice: " K Prateek Nayak
2025-03-10 7:59 ` [PATCH v2 0/4] pipe: Trivial cleanups Christian Brauner
4 siblings, 0 replies; 9+ messages in thread
From: K Prateek Nayak @ 2025-03-07 5:29 UTC (permalink / raw)
To: Linus Torvalds, Oleg Nesterov, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-kernel
Cc: Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal, K Prateek Nayak
Use pipe_buf() helper to retrieve the pipe buffer throughout the file
replacing the open-coded the logic.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
Changelog:
RFC v1..v2:
o New patch.
---
fs/pipe.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index 88e81f84e3ea..4d6ca0f892b1 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -274,7 +274,6 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
/* Read ->head with a barrier vs post_one_notification() */
unsigned int head = smp_load_acquire(&pipe->head);
unsigned int tail = pipe->tail;
- unsigned int mask = pipe->ring_size - 1;
#ifdef CONFIG_WATCH_QUEUE
if (pipe->note_loss) {
@@ -301,7 +300,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
#endif
if (!pipe_empty(head, tail)) {
- struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, tail);
size_t chars = buf->len;
size_t written;
int error;
@@ -471,8 +470,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
was_empty = pipe_empty(head, pipe->tail);
chars = total_len & (PAGE_SIZE-1);
if (chars && !was_empty) {
- unsigned int mask = pipe->ring_size - 1;
- struct pipe_buffer *buf = &pipe->bufs[(head - 1) & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, head - 1);
int offset = buf->offset + buf->len;
if ((buf->flags & PIPE_BUF_FLAG_CAN_MERGE) &&
@@ -503,7 +501,6 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
head = pipe->head;
if (!pipe_full(head, pipe->tail, pipe->max_usage)) {
- unsigned int mask = pipe->ring_size - 1;
struct pipe_buffer *buf;
struct page *page = pipe->tmp_page;
int copied;
@@ -525,7 +522,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
pipe->head = head + 1;
/* Insert it into the buffer array */
- buf = &pipe->bufs[head & mask];
+ buf = pipe_buf(pipe, head);
buf->page = page;
buf->ops = &anon_pipe_buf_ops;
buf->offset = 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] fs/splice: Use pipe_buf() helper to retrieve pipe buffer
2025-03-07 5:29 [PATCH v2 0/4] pipe: Trivial cleanups K Prateek Nayak
` (2 preceding siblings ...)
2025-03-07 5:29 ` [PATCH v2 3/4] fs/pipe: Use pipe_buf() helper to retrieve " K Prateek Nayak
@ 2025-03-07 5:29 ` K Prateek Nayak
2025-03-10 7:59 ` [PATCH v2 0/4] pipe: Trivial cleanups Christian Brauner
4 siblings, 0 replies; 9+ messages in thread
From: K Prateek Nayak @ 2025-03-07 5:29 UTC (permalink / raw)
To: Linus Torvalds, Oleg Nesterov, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-kernel
Cc: Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal, K Prateek Nayak
Use pipe_buf() helper to retrieve the pipe buffer throughout the file
replacing the open-coded the logic.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
Changelog:
RFC v1..v2:
o New patch.
---
fs/splice.c | 40 ++++++++++++++--------------------------
1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/fs/splice.c b/fs/splice.c
index 23fa5561b944..90d464241f15 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -200,7 +200,6 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
unsigned int spd_pages = spd->nr_pages;
unsigned int tail = pipe->tail;
unsigned int head = pipe->head;
- unsigned int mask = pipe->ring_size - 1;
ssize_t ret = 0;
int page_nr = 0;
@@ -214,7 +213,7 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
}
while (!pipe_full(head, tail, pipe->max_usage)) {
- struct pipe_buffer *buf = &pipe->bufs[head & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, head);
buf->page = spd->pages[page_nr];
buf->offset = spd->partial[page_nr].offset;
@@ -247,7 +246,6 @@ ssize_t add_to_pipe(struct pipe_inode_info *pipe, struct pipe_buffer *buf)
{
unsigned int head = pipe->head;
unsigned int tail = pipe->tail;
- unsigned int mask = pipe->ring_size - 1;
int ret;
if (unlikely(!pipe->readers)) {
@@ -256,7 +254,7 @@ ssize_t add_to_pipe(struct pipe_inode_info *pipe, struct pipe_buffer *buf)
} else if (pipe_full(head, tail, pipe->max_usage)) {
ret = -EAGAIN;
} else {
- pipe->bufs[head & mask] = *buf;
+ *pipe_buf(pipe, head) = *buf;
pipe->head = head + 1;
return buf->len;
}
@@ -447,11 +445,10 @@ static int splice_from_pipe_feed(struct pipe_inode_info *pipe, struct splice_des
{
unsigned int head = pipe->head;
unsigned int tail = pipe->tail;
- unsigned int mask = pipe->ring_size - 1;
int ret;
while (!pipe_empty(head, tail)) {
- struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, tail);
sd->len = buf->len;
if (sd->len > sd->total_len)
@@ -495,8 +492,7 @@ static int splice_from_pipe_feed(struct pipe_inode_info *pipe, struct splice_des
static inline bool eat_empty_buffer(struct pipe_inode_info *pipe)
{
unsigned int tail = pipe->tail;
- unsigned int mask = pipe->ring_size - 1;
- struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, tail);
if (unlikely(!buf->len)) {
pipe_buf_release(pipe, buf);
@@ -690,7 +686,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
while (sd.total_len) {
struct kiocb kiocb;
struct iov_iter from;
- unsigned int head, tail, mask;
+ unsigned int head, tail;
size_t left;
int n;
@@ -711,12 +707,11 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
head = pipe->head;
tail = pipe->tail;
- mask = pipe->ring_size - 1;
/* build the vector */
left = sd.total_len;
for (n = 0; !pipe_empty(head, tail) && left && n < nbufs; tail++) {
- struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, tail);
size_t this_len = buf->len;
/* zero-length bvecs are not supported, skip them */
@@ -752,7 +747,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
/* dismiss the fully eaten buffers, adjust the partial one */
tail = pipe->tail;
while (ret) {
- struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, tail);
if (ret >= buf->len) {
ret -= buf->len;
buf->len = 0;
@@ -809,7 +804,7 @@ ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
pipe_lock(pipe);
while (len > 0) {
- unsigned int head, tail, mask, bc = 0;
+ unsigned int head, tail, bc = 0;
size_t remain = len;
/*
@@ -846,10 +841,9 @@ ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
head = pipe->head;
tail = pipe->tail;
- mask = pipe->ring_size - 1;
while (!pipe_empty(head, tail)) {
- struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, tail);
size_t seg;
if (!buf->len) {
@@ -894,7 +888,7 @@ ssize_t splice_to_socket(struct pipe_inode_info *pipe, struct file *out,
len -= ret;
tail = pipe->tail;
while (ret > 0) {
- struct pipe_buffer *buf = &pipe->bufs[tail & mask];
+ struct pipe_buffer *buf = pipe_buf(pipe, tail);
size_t seg = min_t(size_t, ret, buf->len);
buf->offset += seg;
@@ -1725,7 +1719,6 @@ static int splice_pipe_to_pipe(struct pipe_inode_info *ipipe,
struct pipe_buffer *ibuf, *obuf;
unsigned int i_head, o_head;
unsigned int i_tail, o_tail;
- unsigned int i_mask, o_mask;
int ret = 0;
bool input_wakeup = false;
@@ -1747,9 +1740,7 @@ static int splice_pipe_to_pipe(struct pipe_inode_info *ipipe,
pipe_double_lock(ipipe, opipe);
i_tail = ipipe->tail;
- i_mask = ipipe->ring_size - 1;
o_head = opipe->head;
- o_mask = opipe->ring_size - 1;
do {
size_t o_len;
@@ -1792,8 +1783,8 @@ static int splice_pipe_to_pipe(struct pipe_inode_info *ipipe,
goto retry;
}
- ibuf = &ipipe->bufs[i_tail & i_mask];
- obuf = &opipe->bufs[o_head & o_mask];
+ ibuf = pipe_buf(ipipe, i_tail);
+ obuf = pipe_buf(opipe, o_head);
if (len >= ibuf->len) {
/*
@@ -1862,7 +1853,6 @@ static ssize_t link_pipe(struct pipe_inode_info *ipipe,
struct pipe_buffer *ibuf, *obuf;
unsigned int i_head, o_head;
unsigned int i_tail, o_tail;
- unsigned int i_mask, o_mask;
ssize_t ret = 0;
/*
@@ -1873,9 +1863,7 @@ static ssize_t link_pipe(struct pipe_inode_info *ipipe,
pipe_double_lock(ipipe, opipe);
i_tail = ipipe->tail;
- i_mask = ipipe->ring_size - 1;
o_head = opipe->head;
- o_mask = opipe->ring_size - 1;
do {
if (!opipe->readers) {
@@ -1896,8 +1884,8 @@ static ssize_t link_pipe(struct pipe_inode_info *ipipe,
pipe_full(o_head, o_tail, opipe->max_usage))
break;
- ibuf = &ipipe->bufs[i_tail & i_mask];
- obuf = &opipe->bufs[o_head & o_mask];
+ ibuf = pipe_buf(ipipe, i_tail);
+ obuf = pipe_buf(opipe, o_head);
/*
* Get a reference to this pipe buffer,
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring()
2025-03-07 5:29 ` [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring() K Prateek Nayak
@ 2025-03-07 14:51 ` Oleg Nesterov
2025-03-07 16:16 ` K Prateek Nayak
0 siblings, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2025-03-07 14:51 UTC (permalink / raw)
To: K Prateek Nayak
Cc: Linus Torvalds, Alexander Viro, Christian Brauner, linux-fsdevel,
linux-kernel, Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal
On 03/07, K Prateek Nayak wrote:
>
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1271,6 +1271,10 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots)
> struct pipe_buffer *bufs;
> unsigned int head, tail, mask, n;
>
> + /* nr_slots larger than limits of pipe->{head,tail} */
> + if (unlikely(nr_slots > (pipe_index_t)-1u))
> + return -EINVAL;
The whole series look "obviously" good to me,
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
-------------------------------------------------------------------------------
But damn ;) lets look at round_pipe_size(),
unsigned int round_pipe_size(unsigned int size)
{
if (size > (1U << 31))
return 0;
/* Minimum pipe size, as required by POSIX */
if (size < PAGE_SIZE)
return PAGE_SIZE;
return roundup_pow_of_two(size);
}
it is a bit silly to allow the maximum size == 1U << 31 in pipe_set_size()
or (more importantly) in /proc/sys/fs/pipe-max-size, and then nack nr_slots
in pipe_resize_ring().
So perhaps this check should go into round_pipe_size() ? Although I can't
suggest a simple/clear check without unnecesary restrictions for the case
when pipe_index_t is u16.
pipe_resize_ring() has another caller, watch_queue_set_size(), but it has
its own hard limits...
Oleg.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring()
2025-03-07 14:51 ` Oleg Nesterov
@ 2025-03-07 16:16 ` K Prateek Nayak
2025-03-07 22:30 ` Oleg Nesterov
0 siblings, 1 reply; 9+ messages in thread
From: K Prateek Nayak @ 2025-03-07 16:16 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Linus Torvalds, Alexander Viro, Christian Brauner, linux-fsdevel,
linux-kernel, Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal
Hello Oleg,
Thank you for the review.
On 3/7/2025 8:21 PM, Oleg Nesterov wrote:
> On 03/07, K Prateek Nayak wrote:
>>
>> --- a/fs/pipe.c
>> +++ b/fs/pipe.c
>> @@ -1271,6 +1271,10 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots)
>> struct pipe_buffer *bufs;
>> unsigned int head, tail, mask, n;
>>
>> + /* nr_slots larger than limits of pipe->{head,tail} */
>> + if (unlikely(nr_slots > (pipe_index_t)-1u))
>> + return -EINVAL;
>
> The whole series look "obviously" good to me,
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>
> -------------------------------------------------------------------------------
> But damn ;) lets look at round_pipe_size(),
>
> unsigned int round_pipe_size(unsigned int size)
> {
> if (size > (1U << 31))
> return 0;
>
> /* Minimum pipe size, as required by POSIX */
> if (size < PAGE_SIZE)
> return PAGE_SIZE;
>
> return roundup_pow_of_two(size);
> }
>
> it is a bit silly to allow the maximum size == 1U << 31 in pipe_set_size()
> or (more importantly) in /proc/sys/fs/pipe-max-size, and then nack nr_slots
> in pipe_resize_ring().
>
> So perhaps this check should go into round_pipe_size() ? Although I can't
> suggest a simple/clear check without unnecesary restrictions for the case
> when pipe_index_t is u16.
>
> pipe_resize_ring() has another caller, watch_queue_set_size(), but it has
> its own hard limits...
"nr_notes" for watch queues cannot cross 512 so we should be covered there.
As for round_pipe_size(), we can do:
diff --git a/fs/pipe.c b/fs/pipe.c
index ce1af7592780..f82098aaa510 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1253,6 +1253,8 @@ const struct file_operations pipefifo_fops = {
*/
unsigned int round_pipe_size(unsigned int size)
{
+ unsigned int max_slots;
+
if (size > (1U << 31))
return 0;
@@ -1260,7 +1262,14 @@ unsigned int round_pipe_size(unsigned int size)
if (size < PAGE_SIZE)
return PAGE_SIZE;
- return roundup_pow_of_two(size);
+ size = roundup_pow_of_two(size);
+ max_slots = size >> PAGE_SHIFT;
+
+ /* Max slots cannot be covered pipe->{head,tail} limits */
+ if (max_slots > (pipe_index_t)-1U)
+ return 0;
+
+ return size;
}
/*
--
Since pipe_resize_ring() can be called without actually looking at
"pipe_max_size" as is the case with watch queues, we can either keep the
check in pipe_resize_ring() as well out of paranoia or get rid of it
since the current users are within the bounds.
Thoughts?
>
> Oleg.
>
--
Thanks and Regards,
Prateek
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring()
2025-03-07 16:16 ` K Prateek Nayak
@ 2025-03-07 22:30 ` Oleg Nesterov
0 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2025-03-07 22:30 UTC (permalink / raw)
To: K Prateek Nayak
Cc: Linus Torvalds, Alexander Viro, Christian Brauner, linux-fsdevel,
linux-kernel, Jan Kara, Matthew Wilcox (Oracle), Mateusz Guzik,
Rasmus Villemoes, Gautham R. Shenoy, Neeraj.Upadhyay,
Ananth.narayan, Swapnil Sapkal
On 03/07, K Prateek Nayak wrote:
>
> On 3/7/2025 8:21 PM, Oleg Nesterov wrote:
> >On 03/07, K Prateek Nayak wrote:
> >>
> >>--- a/fs/pipe.c
> >>+++ b/fs/pipe.c
> >>@@ -1271,6 +1271,10 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots)
> >> struct pipe_buffer *bufs;
> >> unsigned int head, tail, mask, n;
> >>
> >>+ /* nr_slots larger than limits of pipe->{head,tail} */
> >>+ if (unlikely(nr_slots > (pipe_index_t)-1u))
> >>+ return -EINVAL;
> >
> >The whole series look "obviously" good to me,
> >
> >Reviewed-by: Oleg Nesterov <oleg@redhat.com>
So, in case it wasn't clear, you could safely ignore everything else below ;)
> >pipe_resize_ring() has another caller, watch_queue_set_size(), but it has
> >its own hard limits...
>
> "nr_notes" for watch queues cannot cross 512 so we should be covered there.
Yes, yes, this is what I meant,
> As for round_pipe_size(), we can do:
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index ce1af7592780..f82098aaa510 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1253,6 +1253,8 @@ const struct file_operations pipefifo_fops = {
> */
> unsigned int round_pipe_size(unsigned int size)
> {
> + unsigned int max_slots;
> +
> if (size > (1U << 31))
> return 0;
> @@ -1260,7 +1262,14 @@ unsigned int round_pipe_size(unsigned int size)
> if (size < PAGE_SIZE)
> return PAGE_SIZE;
> - return roundup_pow_of_two(size);
> + size = roundup_pow_of_two(size);
> + max_slots = size >> PAGE_SHIFT;
> +
> + /* Max slots cannot be covered pipe->{head,tail} limits */
> + if (max_slots > (pipe_index_t)-1U)
> + return 0;
Sure, this will work, but still it doesn't look clear/clean to me.
But no, no, I don't blame your suggestion.
To me, round_pipe_size() looks confusing with or without the changes we
discuss. Why does it use "1U << 31" as a maximum size? OK, this is because
that "1 << 31" is the maximum power-of-2 which can fit into u32.
But, even if this code assumes that pipe->head/tail are u32, why this
restriction? Most probably I missed something, but I don't understand.
> Since pipe_resize_ring() can be called without actually looking at
> "pipe_max_size"
Again, only if the caller is watch_queue_set_size(), but it has its own
hard limit.
So. I won't argue either way. Whatever looks better to you. My ack
still stands.
Sorry for (yet another) confusing and almost off-topic email from me.
Oleg.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/4] pipe: Trivial cleanups
2025-03-07 5:29 [PATCH v2 0/4] pipe: Trivial cleanups K Prateek Nayak
` (3 preceding siblings ...)
2025-03-07 5:29 ` [PATCH v2 4/4] fs/splice: " K Prateek Nayak
@ 2025-03-10 7:59 ` Christian Brauner
4 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2025-03-10 7:59 UTC (permalink / raw)
To: K Prateek Nayak
Cc: Christian Brauner, Jan Kara, Matthew Wilcox (Oracle),
Mateusz Guzik, Rasmus Villemoes, Gautham R. Shenoy,
Neeraj.Upadhyay, Ananth.narayan, Swapnil Sapkal, Linus Torvalds,
Oleg Nesterov, Alexander Viro, linux-fsdevel, linux-kernel
On Fri, 07 Mar 2025 05:29:15 +0000, K Prateek Nayak wrote:
> Based on the suggestion on the RFC, the treewide conversion of
> references to pipe->{head,tail} from unsigned int to pipe_index_t has
> been dropped for now. The series contains trivial cleanup suggested to
> limit the nr_slots in pipe_resize_ring() to be covered between
> pipe_index_t limits of pipe->{head,tail} and using pipe_buf() to remove
> the open-coded usage of masks to access pipe buffer building on Linus'
> cleanup of fs/fuse/dev.c in commit ebb0f38bb47f ("fs/pipe: fix pipe
> buffer index use in FUSE")
>
> [...]
Applied to the vfs-6.15.pipe branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.pipe branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.15.pipe
[1/4] fs/pipe: Limit the slots in pipe_resize_ring()
https://git.kernel.org/vfs/vfs/c/cf3d0c54b21c
[2/4] kernel/watch_queue: Use pipe_buf() to retrieve the pipe buffer
https://git.kernel.org/vfs/vfs/c/547476063e12
[3/4] fs/pipe: Use pipe_buf() helper to retrieve pipe buffer
https://git.kernel.org/vfs/vfs/c/ba0822021c3c
[4/4] fs/splice: Use pipe_buf() helper to retrieve pipe buffer
https://git.kernel.org/vfs/vfs/c/d5c6cb01b69c
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-10 7:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-07 5:29 [PATCH v2 0/4] pipe: Trivial cleanups K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 1/4] fs/pipe: Limit the slots in pipe_resize_ring() K Prateek Nayak
2025-03-07 14:51 ` Oleg Nesterov
2025-03-07 16:16 ` K Prateek Nayak
2025-03-07 22:30 ` Oleg Nesterov
2025-03-07 5:29 ` [PATCH v2 2/4] kernel/watch_queue: Use pipe_buf() to retrieve the pipe buffer K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 3/4] fs/pipe: Use pipe_buf() helper to retrieve " K Prateek Nayak
2025-03-07 5:29 ` [PATCH v2 4/4] fs/splice: " K Prateek Nayak
2025-03-10 7:59 ` [PATCH v2 0/4] pipe: Trivial cleanups Christian Brauner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox