The patch below does not apply to the 4.4-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 46c4c9d1beb7f5b4cec4dd90e7728720583ee348 Mon Sep 17 00:00:00 2001
From: "Alex Xu (Hello71)" alex_y_xu@yahoo.ca Date: Thu, 5 Aug 2021 10:40:47 -0400 Subject: [PATCH] pipe: increase minimum default pipe size to 2 pages
This program always prints 4096 and hangs before the patch, and always prints 8192 and exits successfully after:
int main() { int pipefd[2]; for (int i = 0; i < 1025; i++) if (pipe(pipefd) == -1) return 1; size_t bufsz = fcntl(pipefd[1], F_GETPIPE_SZ); printf("%zd\n", bufsz); char *buf = calloc(bufsz, 1); write(pipefd[1], buf, bufsz); read(pipefd[0], buf, bufsz-1); write(pipefd[1], buf, 1); }
Note that you may need to increase your RLIMIT_NOFILE before running the program.
Fixes: 759c01142a ("pipe: limit the per-user amount of pages allocated in pipes") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/lkml/1628086770.5rn8p04n6j.none@localhost/ Link: https://lore.kernel.org/lkml/1628127094.lxxn016tj7.none@localhost/ Signed-off-by: Alex Xu (Hello71) alex_y_xu@yahoo.ca Signed-off-by: Linus Torvalds torvalds@linux-foundation.org
diff --git a/fs/pipe.c b/fs/pipe.c index 9ef4231cce61..8e6ef62aeb1c 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -31,6 +31,21 @@
#include "internal.h"
+/* + * New pipe buffers will be restricted to this size while the user is exceeding + * their pipe buffer quota. The general pipe use case needs at least two + * buffers: one for data yet to be read, and one for new data. If this is less + * than two, then a write to a non-empty pipe may block even if the pipe is not + * full. This can occur with GNU make jobserver or similar uses of pipes as + * semaphores: multiple processes may be waiting to write tokens back to the + * pipe before reading tokens: https://lore.kernel.org/lkml/1628086770.5rn8p04n6j.none@localhost/. + * + * Users can reduce their pipe buffers with F_SETPIPE_SZ below this at their + * own risk, namely: pipe writes to non-full pipes may block until the pipe is + * emptied. + */ +#define PIPE_MIN_DEF_BUFFERS 2 + /* * The max size that a non-root user is allowed to grow the pipe. Can * be set by root in /proc/sys/fs/pipe-max-size @@ -781,8 +796,8 @@ struct pipe_inode_info *alloc_pipe_info(void) user_bufs = account_pipe_buffers(user, 0, pipe_bufs);
if (too_many_pipe_buffers_soft(user_bufs) && pipe_is_unprivileged_user()) { - user_bufs = account_pipe_buffers(user, pipe_bufs, 1); - pipe_bufs = 1; + user_bufs = account_pipe_buffers(user, pipe_bufs, PIPE_MIN_DEF_BUFFERS); + pipe_bufs = PIPE_MIN_DEF_BUFFERS; }
if (too_many_pipe_buffers_hard(user_bufs) && pipe_is_unprivileged_user())
On Mon, Aug 9, 2021 at 2:52 AM gregkh@linuxfoundation.org wrote:
The patch below does not apply to the 4.4-stable tree.
It shouldn't.
The pipe buffer accounting and soft limits that introduced the whole "limp along with limited pipe buffers" behavior that this fixes was introduced by
Fixes: 759c01142a ("pipe: limit the per-user amount of pages allocated in pipes")
..which made it into 4.5.
So 4.4 is unaffected and doesn't want this patch.
Linus
On Mon, Aug 09, 2021 at 09:23:00AM -0700, Linus Torvalds wrote:
On Mon, Aug 9, 2021 at 2:52 AM gregkh@linuxfoundation.org wrote:
The patch below does not apply to the 4.4-stable tree.
It shouldn't.
The pipe buffer accounting and soft limits that introduced the whole "limp along with limited pipe buffers" behavior that this fixes was introduced by
Fixes: 759c01142a ("pipe: limit the per-user amount of pages allocated in pipes")
..which made it into 4.5.
So 4.4 is unaffected and doesn't want this patch.
But that commit showed up in 4.4.13 as fa6d0ba12a8e ("pipe: limit the per-user amount of pages allocated in pipes") which is why I asked about this. The code didn't look similar at all, so I couldn't easily figure out the backport myself :(
Willy, any ideas?
thanks,
greg k-h
On Mon, Aug 9, 2021 at 9:27 AM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
But that commit showed up in 4.4.13 as fa6d0ba12a8e ("pipe: limit the per-user amount of pages allocated in pipes") which is why I asked about this. The code didn't look similar at all, so I couldn't easily figure out the backport myself :(
Oh, I guess I have to actually download the stable tree. Normally I only keep the main tree git around..
Linus
On Mon, Aug 9, 2021 at 9:36 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Oh, I guess I have to actually download the stable tree. Normally I only keep the main tree git around..
Ok, so it does the accounting a bit differently - after allocating them rather than before - but it actually ends up being a simpler patch because of that.
I think it ends up like the attached.
UNTESTED.
I'm not 100% convinced we need to backport this that far anyway, but whatever. Nobody sane runs something like a build server on 4.4.
Nobody sane should run 4.4 at all, of course.
Linus
On Mon, Aug 09, 2021 at 09:46:42AM -0700, Linus Torvalds wrote:
On Mon, Aug 9, 2021 at 9:36 AM Linus Torvalds torvalds@linux-foundation.org wrote:
Oh, I guess I have to actually download the stable tree. Normally I only keep the main tree git around..
Ok, so it does the accounting a bit differently - after allocating them rather than before - but it actually ends up being a simpler patch because of that.
I think it ends up like the attached.
UNTESTED.
I'm not 100% convinced we need to backport this that far anyway, but whatever. Nobody sane runs something like a build server on 4.4.
{sigh}
I wish.
Nobody sane should run 4.4 at all, of course.
Oh I really wish. 4.4 is sticking around in some situations for a very long time for odd reasons, and build servers look to be one of them (a very large cloud company seems enamored with that kernel for some reason...)
fs/pipe.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
This looks good to me, I'll queue it up in a bit as it's more descriptive than Alex's backport.
Thanks for this!
greg k-h
On Mon, Aug 09, 2021 at 07:40:41PM +0200, Greg Kroah-Hartman wrote:
fs/pipe.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
This looks good to me, I'll queue it up in a bit as it's more descriptive than Alex's backport.
Greg, do you *really* want to backport it to 4.4 ? I mean, if nobody faces this issue in 4.4 I can see more risks with the fix that without for systems with low memory (or manually tuned memory usage).
Willy
On Mon, Aug 09, 2021 at 09:04:06PM +0200, Willy Tarreau wrote:
On Mon, Aug 09, 2021 at 07:40:41PM +0200, Greg Kroah-Hartman wrote:
fs/pipe.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
This looks good to me, I'll queue it up in a bit as it's more descriptive than Alex's backport.
Greg, do you *really* want to backport it to 4.4 ? I mean, if nobody faces this issue in 4.4 I can see more risks with the fix that without for systems with low memory (or manually tuned memory usage).
I always prefer to merge "known fixes" to the stable kernel trees as somehow once one person hits a bug, everyone hits it, no matter how long it was in hiding :)
The additional memory usage here seems low to me, and we backported the needed accounting changes there a long time ago, and we know that 4.4.y is being used for build servers as well as other huge hosting providers. The "tiny" systems that might be on 4.4 are not really updating themselves to newer kernels from what I can tell.
So I would prefer to take this patch, but am always willing to revert it if someone reports problems with it, as it keeps us in sync with the other stable branches, and with upstream, as close as possible.
thanks,
greg k-h
On Tue, Aug 10, 2021 at 08:54:13AM +0200, Greg Kroah-Hartman wrote:
Greg, do you *really* want to backport it to 4.4 ? I mean, if nobody faces this issue in 4.4 I can see more risks with the fix that without for systems with low memory (or manually tuned memory usage).
I always prefer to merge "known fixes" to the stable kernel trees as somehow once one person hits a bug, everyone hits it, no matter how long it was in hiding :)
The additional memory usage here seems low to me, and we backported the needed accounting changes there a long time ago, and we know that 4.4.y is being used for build servers as well as other huge hosting providers. The "tiny" systems that might be on 4.4 are not really updating themselves to newer kernels from what I can tell.
I agree that someone sticking to 4.4 surely doesn't want to update.
So I would prefer to take this patch, but am always willing to revert it if someone reports problems with it,
In this case I agree, given that such a problem, if any, would be quick to notice.
Thanks, Willy
Excerpts from Greg Kroah-Hartman's message of August 9, 2021 12:27 pm:
On Mon, Aug 09, 2021 at 09:23:00AM -0700, Linus Torvalds wrote:
On Mon, Aug 9, 2021 at 2:52 AM gregkh@linuxfoundation.org wrote:
The patch below does not apply to the 4.4-stable tree.
It shouldn't.
The pipe buffer accounting and soft limits that introduced the whole "limp along with limited pipe buffers" behavior that this fixes was introduced by
Fixes: 759c01142a ("pipe: limit the per-user amount of pages allocated in pipes")
..which made it into 4.5.
So 4.4 is unaffected and doesn't want this patch.
But that commit showed up in 4.4.13 as fa6d0ba12a8e ("pipe: limit the per-user amount of pages allocated in pipes") which is why I asked about this. The code didn't look similar at all, so I couldn't easily figure out the backport myself :(
Willy, any ideas?
thanks,
greg k-h
alloc_pipe_info was heavily modified in 09b4d19900 ("pipe: simplify logic in alloc_pipe_info()") and a005ca0e68 ("pipe: fix limit checking in alloc_pipe_info()"), which I think landed in 4.9 and weren't backported. The backported patch should look similar to this:
@@ -621,7 +621,7 @@
if (!too_many_pipe_buffers_hard(user)) { if (too_many_pipe_buffers_soft(user)) - pipe_bufs = 1; + pipe_bufs = 2; pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * pipe_bufs, GFP_KERNEL); }
I can send a rebased patch, but I think we can also leave it the way it is. It's a bit of an edge case; if nobody's hit it so far on 4.4, maybe it can just stay this way until February. There's SLTS, but I don't think they're interested in this kind of patch. Thoughts?
Cheers, Alex.
On Mon, Aug 09, 2021 at 12:51:48PM -0400, Alex Xu (Hello71) wrote:
Excerpts from Greg Kroah-Hartman's message of August 9, 2021 12:27 pm:
On Mon, Aug 09, 2021 at 09:23:00AM -0700, Linus Torvalds wrote:
On Mon, Aug 9, 2021 at 2:52 AM gregkh@linuxfoundation.org wrote:
The patch below does not apply to the 4.4-stable tree.
It shouldn't.
The pipe buffer accounting and soft limits that introduced the whole "limp along with limited pipe buffers" behavior that this fixes was introduced by
Fixes: 759c01142a ("pipe: limit the per-user amount of pages allocated in pipes")
..which made it into 4.5.
So 4.4 is unaffected and doesn't want this patch.
But that commit showed up in 4.4.13 as fa6d0ba12a8e ("pipe: limit the per-user amount of pages allocated in pipes") which is why I asked about this. The code didn't look similar at all, so I couldn't easily figure out the backport myself :(
Willy, any ideas?
thanks,
greg k-h
alloc_pipe_info was heavily modified in 09b4d19900 ("pipe: simplify logic in alloc_pipe_info()") and a005ca0e68 ("pipe: fix limit checking in alloc_pipe_info()"), which I think landed in 4.9 and weren't backported. The backported patch should look similar to this:
@@ -621,7 +621,7 @@
if (!too_many_pipe_buffers_hard(user)) { if (too_many_pipe_buffers_soft(user))
pipe_bufs = 1;
pipe_bufs = 2; pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * pipe_bufs, GFP_KERNEL); }
I can send a rebased patch, but I think we can also leave it the way it is. It's a bit of an edge case; if nobody's hit it so far on 4.4, maybe it can just stay this way until February. There's SLTS, but I don't think they're interested in this kind of patch. Thoughts?
I tend to think that if the patch spent 5 years in 4.4 without anyone hitting the problem it's likely that most of the applications found on these distros are not affected either. Doubling the number of pages could however increase the amount of memory used on some small machines heavily using pipes (e.g. for splicing), reducing their stability, which is probably not desirable if nobody complained about the current behavior on that version.
Thus while I think that this fix should do the job I think it's better to leave it out of 4.4 until someone *really* wants it.
Just my two cents, Willy
linux-stable-mirror@lists.linaro.org