FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[gregkh]

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())

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Linus Torvalds]

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

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Greg Kroah-Hartman]

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

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Linus Torvalds]

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

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1448 bytes --]

 fs/pipe.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 6534470a6c19..37a003b645ef 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -27,6 +27,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
@@ -621,7 +636,7 @@ struct pipe_inode_info *alloc_pipe_info(void)
 
 		if (!too_many_pipe_buffers_hard(user)) {
 			if (too_many_pipe_buffers_soft(user))
-				pipe_bufs = 1;
+				pipe_bufs = PIPE_MIN_DEF_BUFFERS;
 			pipe->bufs = kzalloc(sizeof(struct pipe_buffer) * pipe_bufs, GFP_KERNEL);
 		}
 

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Alex Xu (Hello71)]

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.

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Willy Tarreau]

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

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Greg Kroah-Hartman]

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

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Willy Tarreau]

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

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Greg Kroah-Hartman]

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

Re: FAILED: patch "[PATCH] pipe: increase minimum default pipe size to 2 pages" failed to apply to 4.4-stable tree

[Willy Tarreau]

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