[PATCH 1/8] pipe: relocate round_pipe_size() above pipe_set_size()

[PATCH 1/8] pipe: relocate round_pipe_size() above pipe_set_size()

[Michael Kerrisk (man-pages)]

This is a minor preparatory patch. After subsequent patches,
round_pipe_size() will be called from pipe_set_size(), so place
round_pipe_size() above pipe_set_size().

Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

---
 fs/pipe.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 4ebe6b2..7d7c21e 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1011,6 +1011,18 @@ const struct file_operations pipefifo_fops = {
 };
 
 /*
+ * Currently we rely on the pipe array holding a power-of-2 number
+ * of pages.
+ */
+static inline unsigned int round_pipe_size(unsigned int size)
+{
+	unsigned long nr_pages;
+
+	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
+}
+
+/*
  * Allocate a new array of pipe buffers and copy the info over. Returns the
  * pipe size if successful, or return -ERROR on error.
  */
@@ -1062,18 +1074,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long nr_pages)
 }
 
 /*
- * Currently we rely on the pipe array holding a power-of-2 number
- * of pages.
- */
-static inline unsigned int round_pipe_size(unsigned int size)
-{
-	unsigned long nr_pages;
-
-	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
-}
-
-/*
  * This should work even if CONFIG_PROC_FS isn't set, as proc_dointvec_minmax
  * will return an error.
  */
-- 
2.5.5

[PATCH 2/8] pipe: move limit checking logic into pipe_set_size()

[Michael Kerrisk (man-pages)]

This is a preparatory patch for following work. Move the F_SETPIPE_SZ
limit-checking logic from pipe_fcntl() into pipe_set_size().
This simplifies the code a little, and allows for reworking
required in later patches.

Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

---
 fs/pipe.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 7d7c21e..4b98fd0 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1026,9 +1026,24 @@ static inline unsigned int round_pipe_size(unsigned int size)
  * Allocate a new array of pipe buffers and copy the info over. Returns the
  * pipe size if successful, or return -ERROR on error.
  */
-static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long nr_pages)
+static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 {
 	struct pipe_buffer *bufs;
+	unsigned int size, nr_pages;
+
+	size = round_pipe_size(arg);
+	nr_pages = size >> PAGE_SHIFT;
+
+	if (!nr_pages)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size)
+		return -EPERM;
+
+	if ((too_many_pipe_buffers_hard(pipe->user) ||
+			too_many_pipe_buffers_soft(pipe->user)) &&
+			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
 
 	/*
 	 * We can shrink the pipe, if arg >= pipe->nrbufs. Since we don't
@@ -1112,28 +1127,9 @@ long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 	__pipe_lock(pipe);
 
 	switch (cmd) {
-	case F_SETPIPE_SZ: {
-		unsigned int size, nr_pages;
-
-		size = round_pipe_size(arg);
-		nr_pages = size >> PAGE_SHIFT;
-
-		ret = -EINVAL;
-		if (!nr_pages)
-			goto out;
-
-		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
-			ret = -EPERM;
-			goto out;
-		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
-			    too_many_pipe_buffers_soft(pipe->user)) &&
-		           !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN)) {
-			ret = -EPERM;
-			goto out;
-		}
-		ret = pipe_set_size(pipe, nr_pages);
+	case F_SETPIPE_SZ:
+		ret = pipe_set_size(pipe, arg);
 		break;
-		}
 	case F_GETPIPE_SZ:
 		ret = pipe->buffers * PAGE_SIZE;
 		break;
@@ -1142,7 +1138,6 @@ long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 		break;
 	}
 
-out:
 	__pipe_unlock(pipe);
 	return ret;
 }
-- 
2.5.5

[PATCH 3/8] pipe: refactor argument for account_pipe_buffers()

[Michael Kerrisk (man-pages)]

This is a preparatory patch for following work. account_pipe_buffers()
performs accounting in the 'user_struct'. There is no need to pass
a pointer to a 'pipe_inode_info' struct (which is then dereferenced
to obtain a pointer to the 'user' field). Instead, pass a pointer
directly to the 'user_struct'. This change is needed in preparation
for subsequent patches (and the resulting code is a little more logical).

Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

---
 fs/pipe.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 4b98fd0..37b7f5e 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -604,10 +604,10 @@ pipe_fasync(int fd, struct file *filp, int on)
 	return retval;
 }
 
-static void account_pipe_buffers(struct pipe_inode_info *pipe,
+static void account_pipe_buffers(struct user_struct *user,
                                  unsigned long old, unsigned long new)
 {
-	atomic_long_add(new - old, &pipe->user->pipe_bufs);
+	atomic_long_add(new - old, &user->pipe_bufs);
 }
 
 static bool too_many_pipe_buffers_soft(struct user_struct *user)
@@ -644,7 +644,7 @@ struct pipe_inode_info *alloc_pipe_info(void)
 			pipe->r_counter = pipe->w_counter = 1;
 			pipe->buffers = pipe_bufs;
 			pipe->user = user;
-			account_pipe_buffers(pipe, 0, pipe_bufs);
+			account_pipe_buffers(user, 0, pipe_bufs);
 			mutex_init(&pipe->mutex);
 			return pipe;
 		}
@@ -659,7 +659,7 @@ void free_pipe_info(struct pipe_inode_info *pipe)
 {
 	int i;
 
-	account_pipe_buffers(pipe, pipe->buffers, 0);
+	account_pipe_buffers(pipe->user, pipe->buffers, 0);
 	free_uid(pipe->user);
 	for (i = 0; i < pipe->buffers; i++) {
 		struct pipe_buffer *buf = pipe->bufs + i;
@@ -1080,7 +1080,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 			memcpy(bufs + head, pipe->bufs, tail * sizeof(struct pipe_buffer));
 	}
 
-	account_pipe_buffers(pipe, pipe->buffers, nr_pages);
+	account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
 	pipe->curbuf = 0;
 	kfree(pipe->bufs);
 	pipe->bufs = bufs;
-- 
2.5.5

[PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Michael Kerrisk (man-pages)]

The limit checking in pipe_set_size() (used by fcntl(F_SETPIPE_SZ))
has the following problems:

(1) When increasing the pipe capacity, the checks against the limits
    in /proc/sys/fs/pipe-user-pages-{soft,hard} are made against
    existing consumption, and exclude the memory required for the
    increased pipe capacity. The new increase in pipe capacity
    can then push the total memory used by the user for pipes
    (possibly far) over a limit. This can also trigger the problem
    described next.

(2) The limit checks are performed even when the new pipe capacity
    is less than the existing pipe capacity. This can lead to
    problems if a user sets a large pipe capacity, and then the
    limits are lowered, with the result that the user will no
    longer be able to decrease the pipe capacity.

(3) As currently implemented, accounting and checking against the limits
    is done as follows:

    (a) Test whether the user has exceeded the limit.
    (b) Make new pipe buffer allocation.
    (c) Account new allocation against the limits.

    This is racey. Multiple processes may pass point (a) simultaneously,
    and then allocate pipe buffers that are accounted for only in step (c).
    The race means that the user's pipe buffer allocation could be pushed
    over the limit (by an arbitrary amount, depending on how unlucky we
    were in the race). [Thanks to Vegard Nossum for spotting this point,
    which I had missed.]

This patch addresses the above problems as follows:

* Perform checks against the limits only when increasing a pipe's
  capacity; an unprivileged user can always decrease a pipe's capacity.
* Alter the checks against limits to include the memory required for the
  new pipe capacity.
* Re-order the accounting step so that it precedes the buffer
  allocation. If the accounting step determines that a limit has
  been reached, revert the accounting and cause the operation to fail.

The program below can be used to demonstrate problems 1 and 2, and the
effect of the fix. The program takes one or more command-line
arguments. The first argument specifies the number of pipes that the
program should create. The remaining arguments are, alternately, pipe
capacities that should be set using fcntl(F_SETPIPE_SZ), and sleep
intervals (in seconds) between the fcntl() operations. (The sleep
intervals allow the possibility to change the limits between fcntl()
operations.)

Problem 1
=========

Using the test program on an unpatched kernel, we first set some limits:

    # echo 0 > /proc/sys/fs/pipe-user-pages-soft
    # echo 1000000000 > /proc/sys/fs/pipe-max-size
    # echo 10000 > /proc/sys/fs/pipe-user-pages-hard    # 40.96 MB

Then show that we can set a pipe with capacity (100MB) that is
over the hard limit

    # sudo -u mtk ./test_F_SETPIPE_SZ 1 100000000
    Initial pipe capacity: 65536
        Loop 1: set pipe capacity to 100000000 bytes
            F_SETPIPE_SZ returned 134217728

Now set the capacity to 100MB twice. The second call fails (which is
probably surprising to most users, since it seems like a no-op):

    # sudo -u mtk ./test_F_SETPIPE_SZ 1 100000000 0 100000000
    Initial pipe capacity: 65536
        Loop 1: set pipe capacity to 100000000 bytes
            F_SETPIPE_SZ returned 134217728
        Loop 2: set pipe capacity to 100000000 bytes
            Loop 2, pipe 0: F_SETPIPE_SZ failed: fcntl: Operation not permitted

With a patched kernel, setting a capacity over the limit fails at the
first attempt:

    # echo 0 > /proc/sys/fs/pipe-user-pages-soft
    # echo 1000000000 > /proc/sys/fs/pipe-max-size
    # echo 10000 > /proc/sys/fs/pipe-user-pages-hard
    # sudo -u mtk ./test_F_SETPIPE_SZ 1 100000000
    Initial pipe capacity: 65536
        Loop 1: set pipe capacity to 100000000 bytes
            Loop 1, pipe 0: F_SETPIPE_SZ failed: fcntl: Operation not permitted

There is a small chance that the change to fix this problem could
break user-space, since there are cases where fcntl(F_SETPIPE_SZ)
calls that previously succeeded might fail. However, the chances
are small, since (a) the pipe-user-pages-{soft,hard} limits are
new (in 4.5), and the default soft/hard limits are high/unlimited.
Therefore, it seems warranted to make these limits operate more
precisely (and behave more like what users probably expect).

Problem 2
=========

Running the test program on an unpatched kernel, we first set some limits:

    # getconf PAGESIZE
    4096
    # echo 0 > /proc/sys/fs/pipe-user-pages-soft
    # echo 1000000000 > /proc/sys/fs/pipe-max-size
    # echo 10000 > /proc/sys/fs/pipe-user-pages-hard    # 40.96 MB

Now perform two fcntl(F_SETPIPE_SZ) operations on a single pipe,
first setting a pipe capacity (10MB), sleeping for a few seconds,
during which time the hard limit is lowered, and then set pipe
capacity to a smaller amount (5MB):

    # sudo -u mtk ./test_F_SETPIPE_SZ 1 10000000 15 5000000 &
    [1] 748
    # Initial pipe capacity: 65536
        Loop 1: set pipe capacity to 10000000 bytes
            F_SETPIPE_SZ returned 16777216
            Sleeping 15 seconds

    # echo 1000 > /proc/sys/fs/pipe-user-pages-hard      # 4.096 MB
    #     Loop 2: set pipe capacity to 5000000 bytes
            Loop 2, pipe 0: F_SETPIPE_SZ failed: fcntl: Operation not permitted

In this case, the user should be able to lower the limit.

With a kernel that has the patch below, the second fcntl()
succeeds:

    # echo 0 > /proc/sys/fs/pipe-user-pages-soft
    # echo 1000000000 > /proc/sys/fs/pipe-max-size
    # echo 10000 > /proc/sys/fs/pipe-user-pages-hard
    # sudo -u mtk ./test_F_SETPIPE_SZ 1 10000000 15 5000000 &
    [1] 3215
    # Initial pipe capacity: 65536
    #     Loop 1: set pipe capacity to 10000000 bytes
            F_SETPIPE_SZ returned 16777216
            Sleeping 15 seconds

    # echo 1000 > /proc/sys/fs/pipe-user-pages-hard

    #     Loop 2: set pipe capacity to 5000000 bytes
            F_SETPIPE_SZ returned 8388608

8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---

/* test_F_SETPIPE_SZ.c 

   (C) 2016, Michael Kerrisk; licensed under GNU GPL version 2 or later

   Test operation of fcntl(F_SETPIPE_SZ) for setting pipe capacity
   and interactions with limits defined by /proc/sys/fs/pipe-* files.
*/

#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>

int
main(int argc, char *argv[])
{
    int (*pfd)[2];
    int npipes;
    int pcap, rcap;
    int j, p, s, stime, loop;

    if (argc < 2) {
        fprintf(stderr, "Usage: %s num-pipes "
                "[pipe-capacity sleep-time]...\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    npipes = atoi(argv[1]);

    pfd = calloc(npipes, sizeof (int [2]));
    if (pfd == NULL) {
        perror("calloc");
        exit(EXIT_FAILURE);
    }

    for (j = 0; j < npipes; j++) {
        if (pipe(pfd[j]) == -1) {
            fprintf(stderr, "Loop %d: pipe() failed: ", j);
            perror("pipe");
            exit(EXIT_FAILURE);
        }
    }

    printf("Initial pipe capacity: %d\n", fcntl(pfd[0][0], F_GETPIPE_SZ));

    for (j = 2; j < argc; j += 2 ) {
        loop = j / 2;
        pcap = atoi(argv[j]);
        printf("    Loop %d: set pipe capacity to %d bytes\n", loop, pcap);

        for (p = 0; p < npipes; p++) {
            s = fcntl(pfd[p][0], F_SETPIPE_SZ, pcap);
            if (s == -1) {
                fprintf(stderr, "        Loop %d, pipe %d: F_SETPIPE_SZ "
                        "failed: ", loop, p);
                perror("fcntl");
                exit(EXIT_FAILURE);
            }

            if (p == 0) {
                printf("        F_SETPIPE_SZ returned %d\n", s);
                rcap = s;
            } else {
                if (s != rcap) {
                    fprintf(stderr, "        Loop %d, pipe %d: F_SETPIPE_SZ "
                            "unexpected return: %d\n", loop, p, s);
                    exit(EXIT_FAILURE);
                }
            }

            stime = (j + 1 < argc) ? atoi(argv[j + 1]) : 0;
            if (stime > 0) {
                printf("        Sleeping %d seconds\n", stime);
                sleep(stime);
            }
        }
    }

    exit(EXIT_SUCCESS);
}
8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---8x---


Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

---
 fs/pipe.c | 43 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 37b7f5e..a7470a9 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1030,6 +1030,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 {
 	struct pipe_buffer *bufs;
 	unsigned int size, nr_pages;
+	long ret = 0;
 
 	size = round_pipe_size(arg);
 	nr_pages = size >> PAGE_SHIFT;
@@ -1037,13 +1038,26 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	if (!nr_pages)
 		return -EINVAL;
 
-	if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size)
-		return -EPERM;
+	account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
 
-	if ((too_many_pipe_buffers_hard(pipe->user) ||
-			too_many_pipe_buffers_soft(pipe->user)) &&
-			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	/*
+	 * If trying to increase the pipe capacity, check that an
+	 * unprivileged user is not trying to exceed various limits.
+	 * (Decreasing the pipe capacity is always permitted, even
+	 * if the user is currently over a limit.)
+	 */
+	if (nr_pages > pipe->buffers) {
+		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
+			ret = -EPERM;
+			goto out_revert_acct;
+		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
+				too_many_pipe_buffers_soft(pipe->user)) &&
+				!capable(CAP_SYS_RESOURCE) &&
+				!capable(CAP_SYS_ADMIN)) {
+			ret = -EPERM;
+			goto out_revert_acct;
+		}
+	}
 
 	/*
 	 * We can shrink the pipe, if arg >= pipe->nrbufs. Since we don't
@@ -1051,13 +1065,17 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	 * again like we would do for growing. If the pipe currently
 	 * contains more buffers than arg, then return busy.
 	 */
-	if (nr_pages < pipe->nrbufs)
-		return -EBUSY;
+	if (nr_pages < pipe->nrbufs) {
+		ret = -EBUSY;
+		goto out_revert_acct;
+	}
 
 	bufs = kcalloc(nr_pages, sizeof(*bufs),
 		       GFP_KERNEL_ACCOUNT | __GFP_NOWARN);
-	if (unlikely(!bufs))
-		return -ENOMEM;
+	if (unlikely(!bufs)) {
+		ret = -ENOMEM;
+		goto out_revert_acct;
+	}
 
 	/*
 	 * The pipe array wraps around, so just start the new one at zero
@@ -1080,12 +1098,15 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 			memcpy(bufs + head, pipe->bufs, tail * sizeof(struct pipe_buffer));
 	}
 
-	account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
 	pipe->curbuf = 0;
 	kfree(pipe->bufs);
 	pipe->bufs = bufs;
 	pipe->buffers = nr_pages;
 	return nr_pages * PAGE_SIZE;
+
+out_revert_acct:
+	account_pipe_buffers(pipe->user, nr_pages, pipe->buffers);
+	return ret;
 }
 
 /*
-- 
2.5.5

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Willy Tarreau]

Hi Michael,

Since you're changing this code, it's probably worth swapping the size
check and capable() below to save a function call in the normal path :

On Fri, Aug 19, 2016 at 05:25:35PM +1200, Michael Kerrisk (man-pages) wrote:
> +	if (nr_pages > pipe->buffers) {
> +		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {

=>		if (size > pipe_max_size && !capable(CAP_SYS_RESOURCE)) {

> +			ret = -EPERM;
> +			goto out_revert_acct;
> +		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
> +				too_many_pipe_buffers_soft(pipe->user)) &&
> +				!capable(CAP_SYS_RESOURCE) &&
> +				!capable(CAP_SYS_ADMIN)) {
> +			ret = -EPERM;
> +			goto out_revert_acct;
> +		}
> +	}
(...)

Cheers,
Willy

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Michael Kerrisk (man-pages)]

On 08/19/2016 05:48 PM, Willy Tarreau wrote:
> Hi Michael,
> 
> Since you're changing this code, it's probably worth swapping the size
> check and capable() below to save a function call in the normal path :
> 
> On Fri, Aug 19, 2016 at 05:25:35PM +1200, Michael Kerrisk (man-pages) wrote:
>> +	if (nr_pages > pipe->buffers) {
>> +		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
> 
> =>		if (size > pipe_max_size && !capable(CAP_SYS_RESOURCE)) {
> 
>> +			ret = -EPERM;
>> +			goto out_revert_acct;
>> +		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
>> +				too_many_pipe_buffers_soft(pipe->user)) &&
>> +				!capable(CAP_SYS_RESOURCE) &&
>> +				!capable(CAP_SYS_ADMIN)) {
>> +			ret = -EPERM;
>> +			goto out_revert_acct;
>> +		}
>> +	}
> (...)

Thanks, Willy. Fixed for the next iteration. (And I made the same fix made 
also in the 8/8 patch).

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Michael Kerrisk (man-pages)]

Hi Willy,

Might you have a chance to further review of this patch series?
It would be great if you could, since much of it touches changes
made by you earlier.

Thanks,

Michael

On 08/19/2016 05:48 PM, Willy Tarreau wrote:
> Hi Michael,
> 
> Since you're changing this code, it's probably worth swapping the size
> check and capable() below to save a function call in the normal path :
> 
> On Fri, Aug 19, 2016 at 05:25:35PM +1200, Michael Kerrisk (man-pages) wrote:
>> +	if (nr_pages > pipe->buffers) {
>> +		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
> 
> =>		if (size > pipe_max_size && !capable(CAP_SYS_RESOURCE)) {
> 
>> +			ret = -EPERM;
>> +			goto out_revert_acct;
>> +		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
>> +				too_many_pipe_buffers_soft(pipe->user)) &&
>> +				!capable(CAP_SYS_RESOURCE) &&
>> +				!capable(CAP_SYS_ADMIN)) {
>> +			ret = -EPERM;
>> +			goto out_revert_acct;
>> +		}
>> +	}
> (...)
> 
> Cheers,
> Willy
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Willy Tarreau]

Hi Michael,

On Mon, Aug 22, 2016 at 09:15:35AM +1200, Michael Kerrisk (man-pages) wrote:
> Hi Willy,
> 
> Might you have a chance to further review of this patch series?
> It would be great if you could, since much of it touches changes
> made by you earlier.

Well, all I did there was implementing a suggestion from Linus, but I'm
not a specialist at all there. However I've read all your series and at
least with my limited knowledge, all I've read seems to make sense at
the code matches the descriptions. I don't remember any particular trap
in this place so I'm not worried.

I remember that I noticed this inaccuracy in the accounting but I
estimated it was not important since the goal was to *limit* resource
usage with a simple patch that we could easily backport. Your approach
looks clean and possibly backportable if needed. That's all I can say
I'm afraid :-/

Cheers,
Willy

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Michael Kerrisk (man-pages)]

Hi Willy,

On 08/22/2016 09:35 AM, Willy Tarreau wrote:
> Hi Michael,
> 
> On Mon, Aug 22, 2016 at 09:15:35AM +1200, Michael Kerrisk (man-pages) wrote:
>> Hi Willy,
>>
>> Might you have a chance to further review of this patch series?
>> It would be great if you could, since much of it touches changes
>> made by you earlier.
> 
> Well, all I did there was implementing a suggestion from Linus, but I'm
> not a specialist at all there. However I've read all your series and at
> least with my limited knowledge, all I've read seems to make sense at
> the code matches the descriptions. I don't remember any particular trap
> in this place so I'm not worried.

Okay.

> I remember that I noticed this inaccuracy in the accounting but I
> estimated it was not important since the goal was to *limit* resource
> usage with a simple patch that we could easily backport. Your approach
> looks clean and possibly backportable if needed. That's all I can say
> I'm afraid :-/

No problem. Thanks for the reply!

Cheers,

Michael



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Vegard Nossum]

On 08/19/2016 07:25 AM, Michael Kerrisk (man-pages) wrote:
> The limit checking in pipe_set_size() (used by fcntl(F_SETPIPE_SZ))
> has the following problems:
[...]
> @@ -1030,6 +1030,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>   {
>   	struct pipe_buffer *bufs;
>   	unsigned int size, nr_pages;
> +	long ret = 0;
>
>   	size = round_pipe_size(arg);
>   	nr_pages = size >> PAGE_SHIFT;
> @@ -1037,13 +1038,26 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>   	if (!nr_pages)
>   		return -EINVAL;
>
> -	if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size)
> -		return -EPERM;
> +	account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
>
> -	if ((too_many_pipe_buffers_hard(pipe->user) ||
> -			too_many_pipe_buffers_soft(pipe->user)) &&
> -			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> +	/*
> +	 * If trying to increase the pipe capacity, check that an
> +	 * unprivileged user is not trying to exceed various limits.
> +	 * (Decreasing the pipe capacity is always permitted, even
> +	 * if the user is currently over a limit.)
> +	 */
> +	if (nr_pages > pipe->buffers) {
> +		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
> +			ret = -EPERM;
> +			goto out_revert_acct;
> +		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
> +				too_many_pipe_buffers_soft(pipe->user)) &&
> +				!capable(CAP_SYS_RESOURCE) &&
> +				!capable(CAP_SYS_ADMIN)) {
> +			ret = -EPERM;
> +			goto out_revert_acct;
> +		}
> +	}

I'm slightly worried about not checking arg/nr_pages before we pass it
on to account_pipe_buffers().

The potential problem happens if the user passes a very large number
which will overflow pipe->user->pipe_bufs.

On 32-bit, sizeof(int) == sizeof(long), so if they pass arg = INT_MAX
then round_pipe_size() returns INT_MAX. Although it's true that the
accounting is done in terms of pages and not bytes, so you'd need on the
order of (1 << 13) = 8192 processes hitting the limit at the same time
in order to make it overflow, which seems a bit unlikely.

(See https://lkml.org/lkml/2016/8/12/215 for another discussion on the
limit checking)

Is there any reason why we couldn't do the (size > pipe_max_size) check
before calling account_pipe_buffers()?


Vegard

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Michael Kerrisk (man-pages)]

Hi Vegard,

On 08/19/2016 08:30 PM, Vegard Nossum wrote:
> On 08/19/2016 07:25 AM, Michael Kerrisk (man-pages) wrote:
>> The limit checking in pipe_set_size() (used by fcntl(F_SETPIPE_SZ))
>> has the following problems:
> [...]
>> @@ -1030,6 +1030,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>>   {
>>   	struct pipe_buffer *bufs;
>>   	unsigned int size, nr_pages;
>> +	long ret = 0;
>>
>>   	size = round_pipe_size(arg);
>>   	nr_pages = size >> PAGE_SHIFT;
>> @@ -1037,13 +1038,26 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>>   	if (!nr_pages)
>>   		return -EINVAL;
>>
>> -	if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size)
>> -		return -EPERM;
>> +	account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
>>
>> -	if ((too_many_pipe_buffers_hard(pipe->user) ||
>> -			too_many_pipe_buffers_soft(pipe->user)) &&
>> -			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
>> -		return -EPERM;
>> +	/*
>> +	 * If trying to increase the pipe capacity, check that an
>> +	 * unprivileged user is not trying to exceed various limits.
>> +	 * (Decreasing the pipe capacity is always permitted, even
>> +	 * if the user is currently over a limit.)
>> +	 */
>> +	if (nr_pages > pipe->buffers) {
>> +		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
>> +			ret = -EPERM;
>> +			goto out_revert_acct;
>> +		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
>> +				too_many_pipe_buffers_soft(pipe->user)) &&
>> +				!capable(CAP_SYS_RESOURCE) &&
>> +				!capable(CAP_SYS_ADMIN)) {
>> +			ret = -EPERM;
>> +			goto out_revert_acct;
>> +		}
>> +	}
> 
> I'm slightly worried about not checking arg/nr_pages before we pass it
> on to account_pipe_buffers().
> 
> The potential problem happens if the user passes a very large number
> which will overflow pipe->user->pipe_bufs.
> 
> On 32-bit, sizeof(int) == sizeof(long), so if they pass arg = INT_MAX
> then round_pipe_size() returns INT_MAX. Although it's true that the
> accounting is done in terms of pages and not bytes, so you'd need on the
> order of (1 << 13) = 8192 processes hitting the limit at the same time
> in order to make it overflow, which seems a bit unlikely.
> 
> (See https://lkml.org/lkml/2016/8/12/215 for another discussion on the
> limit checking)
> 
> Is there any reason why we couldn't do the (size > pipe_max_size) check
> before calling account_pipe_buffers()?

No reason that I can see. Just a little more work to be done in the
code, I think.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Michael Kerrisk (man-pages)]

On 08/20/2016 08:56 AM, Michael Kerrisk (man-pages) wrote:
> Hi Vegard,
> 
> On 08/19/2016 08:30 PM, Vegard Nossum wrote:
>> On 08/19/2016 07:25 AM, Michael Kerrisk (man-pages) wrote:
>>> The limit checking in pipe_set_size() (used by fcntl(F_SETPIPE_SZ))
>>> has the following problems:
>> [...]
>>> @@ -1030,6 +1030,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>>>   {
>>>   	struct pipe_buffer *bufs;
>>>   	unsigned int size, nr_pages;
>>> +	long ret = 0;
>>>
>>>   	size = round_pipe_size(arg);
>>>   	nr_pages = size >> PAGE_SHIFT;
>>> @@ -1037,13 +1038,26 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>>>   	if (!nr_pages)
>>>   		return -EINVAL;
>>>
>>> -	if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size)
>>> -		return -EPERM;
>>> +	account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
>>>
>>> -	if ((too_many_pipe_buffers_hard(pipe->user) ||
>>> -			too_many_pipe_buffers_soft(pipe->user)) &&
>>> -			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
>>> -		return -EPERM;
>>> +	/*
>>> +	 * If trying to increase the pipe capacity, check that an
>>> +	 * unprivileged user is not trying to exceed various limits.
>>> +	 * (Decreasing the pipe capacity is always permitted, even
>>> +	 * if the user is currently over a limit.)
>>> +	 */
>>> +	if (nr_pages > pipe->buffers) {
>>> +		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
>>> +			ret = -EPERM;
>>> +			goto out_revert_acct;
>>> +		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
>>> +				too_many_pipe_buffers_soft(pipe->user)) &&
>>> +				!capable(CAP_SYS_RESOURCE) &&
>>> +				!capable(CAP_SYS_ADMIN)) {
>>> +			ret = -EPERM;
>>> +			goto out_revert_acct;
>>> +		}
>>> +	}
>>
>> I'm slightly worried about not checking arg/nr_pages before we pass it
>> on to account_pipe_buffers().
>>
>> The potential problem happens if the user passes a very large number
>> which will overflow pipe->user->pipe_bufs.
>>
>> On 32-bit, sizeof(int) == sizeof(long), so if they pass arg = INT_MAX
>> then round_pipe_size() returns INT_MAX. Although it's true that the
>> accounting is done in terms of pages and not bytes, so you'd need on the
>> order of (1 << 13) = 8192 processes hitting the limit at the same time
>> in order to make it overflow, which seems a bit unlikely.
>>
>> (See https://lkml.org/lkml/2016/8/12/215 for another discussion on the
>> limit checking)
>>
>> Is there any reason why we couldn't do the (size > pipe_max_size) check
>> before calling account_pipe_buffers()?
> 
> No reason that I can see. Just a little more work to be done in the
> code, I think.

And, just so I make sure we're understanding each other... I assume you
mean changing the code here to something like:

static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
{
        struct pipe_buffer *bufs;
        unsigned int size, nr_pages;
        unsigned long user_bufs;
        long ret = 0;

        size = round_pipe_size(arg);
        nr_pages = size >> PAGE_SHIFT;

        if (!nr_pages)
                return -EINVAL;

        /*
         * If trying to increase the pipe capacity, check that an
         * unprivileged user is not trying to exceed various limits
         * (soft limit check here, hard limit check just below).
         * Decreasing the pipe capacity is always permitted, even
         * if the user is currently over a limit.
         */
        if (nr_pages > pipe->buffers &&
                        size > pipe_max_size && !capable(CAP_SYS_RESOURCE))
                return -EPERM;

        user_bufs = account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);

        if (nr_pages > pipe->buffers &&
                        too_many_pipe_buffers_hard(user_bufs ||
                        too_many_pipe_buffers_soft(user_bufs)) &&
                        !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN)) {
                ret = -EPERM;
                goto out_revert_acct;
        }

Right?

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Vegard Nossum]

On 08/20/2016 01:17 AM, Michael Kerrisk (man-pages) wrote:
> On 08/20/2016 08:56 AM, Michael Kerrisk (man-pages) wrote:
>> On 08/19/2016 08:30 PM, Vegard Nossum wrote:
>>> Is there any reason why we couldn't do the (size > pipe_max_size) check
>>> before calling account_pipe_buffers()?
>>
>> No reason that I can see. Just a little more work to be done in the
>> code, I think.
>
> And, just so I make sure we're understanding each other... I assume you
> mean changing the code here to something like:
[...]
>         if (nr_pages > pipe->buffers &&
>                         size > pipe_max_size && !capable(CAP_SYS_RESOURCE))
>                 return -EPERM;
>
>         user_bufs = account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
>
>         if (nr_pages > pipe->buffers &&
>                         too_many_pipe_buffers_hard(user_bufs ||
>                         too_many_pipe_buffers_soft(user_bufs)) &&
>                         !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN)) {
>                 ret = -EPERM;
>                 goto out_revert_acct;
>         }
>
> Right?

Yup, that's what I had in mind. (The parantheses are messed up though.)


Vegard

Re: [PATCH 4/8] pipe: fix limit checking in pipe_set_size()

[Michael Kerrisk (man-pages)]

On 08/21/2016 10:33 PM, Vegard Nossum wrote:
> On 08/20/2016 01:17 AM, Michael Kerrisk (man-pages) wrote:
>> On 08/20/2016 08:56 AM, Michael Kerrisk (man-pages) wrote:
>>> On 08/19/2016 08:30 PM, Vegard Nossum wrote:
>>>> Is there any reason why we couldn't do the (size > pipe_max_size) check
>>>> before calling account_pipe_buffers()?
>>>
>>> No reason that I can see. Just a little more work to be done in the
>>> code, I think.
>>
>> And, just so I make sure we're understanding each other... I assume you
>> mean changing the code here to something like:
> [...]
>>         if (nr_pages > pipe->buffers &&
>>                         size > pipe_max_size && !capable(CAP_SYS_RESOURCE))
>>                 return -EPERM;
>>
>>         user_bufs = account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
>>
>>         if (nr_pages > pipe->buffers &&
>>                         too_many_pipe_buffers_hard(user_bufs ||
>>                         too_many_pipe_buffers_soft(user_bufs)) &&
>>                         !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN)) {
>>                 ret = -EPERM;
>>                 goto out_revert_acct;
>>         }
>>
>> Right?
> 
> Yup, that's what I had in mind.

Okay -- changed.

> (The parantheses are messed up though.)

Yup, was just a quick untested edit to make sure we meant the same thing.

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[PATCH 5/8] pipe: simplify logic in alloc_pipe_info()

[Michael Kerrisk (man-pages)]

Replace an 'if' block that covers most of the code in this
function with a 'goto'. This makes the code a little simpler
to read, and also simplifies the next patch.

Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

---
 fs/pipe.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index a7470a9..613c6b9 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -625,33 +625,34 @@ static bool too_many_pipe_buffers_hard(struct user_struct *user)
 struct pipe_inode_info *alloc_pipe_info(void)
 {
 	struct pipe_inode_info *pipe;
+	unsigned long pipe_bufs = PIPE_DEF_BUFFERS;
+	struct user_struct *user = get_current_user();
 
 	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL_ACCOUNT);
-	if (pipe) {
-		unsigned long pipe_bufs = PIPE_DEF_BUFFERS;
-		struct user_struct *user = get_current_user();
-
-		if (!too_many_pipe_buffers_hard(user)) {
-			if (too_many_pipe_buffers_soft(user))
-				pipe_bufs = 1;
-			pipe->bufs = kcalloc(pipe_bufs,
-					     sizeof(struct pipe_buffer),
-					     GFP_KERNEL_ACCOUNT);
-		}
+	if (pipe == NULL)
+		goto out_free_uid;
+
+	if (!too_many_pipe_buffers_hard(user)) {
+		if (too_many_pipe_buffers_soft(user))
+			pipe_bufs = 1;
+		pipe->bufs = kcalloc(pipe_bufs,
+				     sizeof(struct pipe_buffer),
+				     GFP_KERNEL_ACCOUNT);
+	}
 
-		if (pipe->bufs) {
-			init_waitqueue_head(&pipe->wait);
-			pipe->r_counter = pipe->w_counter = 1;
-			pipe->buffers = pipe_bufs;
-			pipe->user = user;
-			account_pipe_buffers(user, 0, pipe_bufs);
-			mutex_init(&pipe->mutex);
-			return pipe;
-		}
-		free_uid(user);
-		kfree(pipe);
+	if (pipe->bufs) {
+		init_waitqueue_head(&pipe->wait);
+		pipe->r_counter = pipe->w_counter = 1;
+		pipe->buffers = pipe_bufs;
+		pipe->user = user;
+		account_pipe_buffers(user, 0, pipe_bufs);
+		mutex_init(&pipe->mutex);
+		return pipe;
 	}
 
+	kfree(pipe);
+out_free_uid:
+	free_uid(user);
 	return NULL;
 }
 
-- 
2.5.5

[PATCH 6/8] pipe: fix limit checking in alloc_pipe_info()

[Michael Kerrisk (man-pages)]

The limit checking in alloc_pipe_info() (used by pipe(2) and when
opening a FIFO) has the following problems:

(1) When checking capacity required for the new pipe, the checks
    against the limit in /proc/sys/fs/pipe-user-pages-{soft,hard}
    are made against existing consumption, and exclude the memory
    required for the new pipe capacity. As a consequence: (1) the
    memory allocation throttling provided by the soft limit does
    not kick in quite as early as it should, and (2) the user can
    overrun the hard limit.

(2) As currently implemented, accounting and checking against the limits
    is done as follows:

    (a) Test whether the user has exceeded the limit.
    (b) Make new pipe buffer allocation.
    (c) Account new allocation against the limits.

    This is racey. Multiple processes may pass point (a)
    simultaneously, and then allocate pipe buffers that are
    accounted for only in step (c).  The race means that the
    user's pipe buffer allocation could be pushed over the limit
    (by an arbitrary amount, depending on how unlucky we were in
    the race). [Thanks to Vegard Nossum for spotting this point,
    which I had missed.]

This patch addresses the above problems as follows:

* Alter the checks against limits to include the memory required for the
  new pipe.
* Re-order the accounting step so that it precedes the buffer allocation.
  If the accounting step determines that a limit has been reached, revert
  the accounting and cause the operation to fail.

Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

---
 fs/pipe.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 613c6b9..705d79f 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -632,24 +632,28 @@ struct pipe_inode_info *alloc_pipe_info(void)
 	if (pipe == NULL)
 		goto out_free_uid;
 
-	if (!too_many_pipe_buffers_hard(user)) {
-		if (too_many_pipe_buffers_soft(user))
-			pipe_bufs = 1;
-		pipe->bufs = kcalloc(pipe_bufs,
-				     sizeof(struct pipe_buffer),
-				     GFP_KERNEL_ACCOUNT);
-	}
+	if (too_many_pipe_buffers_soft(user))
+		pipe_bufs = 1;
+
+	account_pipe_buffers(user, 0, pipe_bufs);
+
+	if (too_many_pipe_buffers_hard(user))
+		goto out_revert_acct;
+
+	pipe->bufs = kcalloc(pipe_bufs, sizeof(struct pipe_buffer),
+			     GFP_KERNEL_ACCOUNT);
 
 	if (pipe->bufs) {
 		init_waitqueue_head(&pipe->wait);
 		pipe->r_counter = pipe->w_counter = 1;
 		pipe->buffers = pipe_bufs;
 		pipe->user = user;
-		account_pipe_buffers(user, 0, pipe_bufs);
 		mutex_init(&pipe->mutex);
 		return pipe;
 	}
 
+out_revert_acct:
+	account_pipe_buffers(user, pipe_bufs, 0);
 	kfree(pipe);
 out_free_uid:
 	free_uid(user);
-- 
2.5.5

[PATCH 7/8] pipe: make account_pipe_buffers() return a value, and use it

[Michael Kerrisk (man-pages)]

This is an optional patch, to provide a small performance improvement.
Alter account_pipe_buffers() so that it returns the new value in
user->pipe_bufs. This means that we can refactor too_many_pipe_buffers_soft()
and too_many_pipe_buffers_hard() to avoid the costs of repeated use of
atomic_long_read() to get the value user->pipe_bufs.

Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

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

diff --git a/fs/pipe.c b/fs/pipe.c
index 705d79f..ada1777 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -604,22 +604,20 @@ pipe_fasync(int fd, struct file *filp, int on)
 	return retval;
 }
 
-static void account_pipe_buffers(struct user_struct *user,
+static unsigned long account_pipe_buffers(struct user_struct *user,
                                  unsigned long old, unsigned long new)
 {
-	atomic_long_add(new - old, &user->pipe_bufs);
+	return atomic_long_add_return(new - old, &user->pipe_bufs);
 }
 
-static bool too_many_pipe_buffers_soft(struct user_struct *user)
+static bool too_many_pipe_buffers_soft(unsigned long num_bufs)
 {
-	return pipe_user_pages_soft &&
-	       atomic_long_read(&user->pipe_bufs) >= pipe_user_pages_soft;
+	return pipe_user_pages_soft && num_bufs >= pipe_user_pages_soft;
 }
 
-static bool too_many_pipe_buffers_hard(struct user_struct *user)
+static bool too_many_pipe_buffers_hard(unsigned long num_bufs)
 {
-	return pipe_user_pages_hard &&
-	       atomic_long_read(&user->pipe_bufs) >= pipe_user_pages_hard;
+	return pipe_user_pages_hard && num_bufs >= pipe_user_pages_hard;
 }
 
 struct pipe_inode_info *alloc_pipe_info(void)
@@ -627,17 +625,18 @@ struct pipe_inode_info *alloc_pipe_info(void)
 	struct pipe_inode_info *pipe;
 	unsigned long pipe_bufs = PIPE_DEF_BUFFERS;
 	struct user_struct *user = get_current_user();
+	unsigned long num_bufs;
 
 	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL_ACCOUNT);
 	if (pipe == NULL)
 		goto out_free_uid;
 
-	if (too_many_pipe_buffers_soft(user))
+	if (too_many_pipe_buffers_soft(atomic_long_read(&user->pipe_bufs)))
 		pipe_bufs = 1;
 
-	account_pipe_buffers(user, 0, pipe_bufs);
+	num_bufs = account_pipe_buffers(user, 0, pipe_bufs);
 
-	if (too_many_pipe_buffers_hard(user))
+	if (too_many_pipe_buffers_hard(num_bufs))
 		goto out_revert_acct;
 
 	pipe->bufs = kcalloc(pipe_bufs, sizeof(struct pipe_buffer),
@@ -653,7 +652,7 @@ struct pipe_inode_info *alloc_pipe_info(void)
 	}
 
 out_revert_acct:
-	account_pipe_buffers(user, pipe_bufs, 0);
+	(void) account_pipe_buffers(user, pipe_bufs, 0);
 	kfree(pipe);
 out_free_uid:
 	free_uid(user);
@@ -664,7 +663,7 @@ void free_pipe_info(struct pipe_inode_info *pipe)
 {
 	int i;
 
-	account_pipe_buffers(pipe->user, pipe->buffers, 0);
+	(void) account_pipe_buffers(pipe->user, pipe->buffers, 0);
 	free_uid(pipe->user);
 	for (i = 0; i < pipe->buffers; i++) {
 		struct pipe_buffer *buf = pipe->bufs + i;
@@ -1035,6 +1034,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 {
 	struct pipe_buffer *bufs;
 	unsigned int size, nr_pages;
+	unsigned long num_bufs;
 	long ret = 0;
 
 	size = round_pipe_size(arg);
@@ -1043,7 +1043,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	if (!nr_pages)
 		return -EINVAL;
 
-	account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
+	num_bufs = account_pipe_buffers(pipe->user, pipe->buffers, nr_pages);
 
 	/*
 	 * If trying to increase the pipe capacity, check that an
@@ -1055,8 +1055,8 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
 			ret = -EPERM;
 			goto out_revert_acct;
-		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
-				too_many_pipe_buffers_soft(pipe->user)) &&
+		} else if ((too_many_pipe_buffers_hard(num_bufs) ||
+				too_many_pipe_buffers_soft(num_bufs)) &&
 				!capable(CAP_SYS_RESOURCE) &&
 				!capable(CAP_SYS_ADMIN)) {
 			ret = -EPERM;
@@ -1110,7 +1110,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	return nr_pages * PAGE_SIZE;
 
 out_revert_acct:
-	account_pipe_buffers(pipe->user, nr_pages, pipe->buffers);
+	(void) account_pipe_buffers(pipe->user, nr_pages, pipe->buffers);
 	return ret;
 }
 
-- 
2.5.5

Re: [PATCH 7/8] pipe: make account_pipe_buffers() return a value, and use it

[Vegard Nossum]

On 08/19/2016 07:25 AM, Michael Kerrisk (man-pages) wrote:
> This is an optional patch, to provide a small performance improvement.
> Alter account_pipe_buffers() so that it returns the new value in
> user->pipe_bufs. This means that we can refactor too_many_pipe_buffers_soft()
> and too_many_pipe_buffers_hard() to avoid the costs of repeated use of
> atomic_long_read() to get the value user->pipe_bufs.
[...]
> @@ -627,17 +625,18 @@ struct pipe_inode_info *alloc_pipe_info(void)
>   	struct pipe_inode_info *pipe;
>   	unsigned long pipe_bufs = PIPE_DEF_BUFFERS;
>   	struct user_struct *user = get_current_user();
> +	unsigned long num_bufs;

Maybe user_bufs would be more descriptive since num_bufs is a bit
ambiguous without the context.

>
>   	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL_ACCOUNT);
>   	if (pipe == NULL)
>   		goto out_free_uid;
>
> -	if (too_many_pipe_buffers_soft(user))
> +	if (too_many_pipe_buffers_soft(atomic_long_read(&user->pipe_bufs)))
>   		pipe_bufs = 1;
>
> -	account_pipe_buffers(user, 0, pipe_bufs);
> +	num_bufs = account_pipe_buffers(user, 0, pipe_bufs);
>
> -	if (too_many_pipe_buffers_hard(user))
> +	if (too_many_pipe_buffers_hard(num_bufs))
>   		goto out_revert_acct;
>
>   	pipe->bufs = kcalloc(pipe_bufs, sizeof(struct pipe_buffer),

Why not structure it like this?

num_bufs = account_pipe_buffers(user, 0, pipe_bufs);
if (too_many_pipe_buffers_soft(num_bufs)) {
	num_bufs = account_pipe_buffers(user, pipe_bufs, 1);
	pipe_bufs = 1;
}
if (too_many_pipe_buffers_hard(num_bufs))
	goto out_revert_acct;

Otherwise you still have the case that somebody makes it past
too_many_pipe_buffers_soft() before the accounting is done.


Vegard

Re: [PATCH 7/8] pipe: make account_pipe_buffers() return a value, and use it

[Michael Kerrisk (man-pages)]

On 08/19/2016 09:36 PM, Vegard Nossum wrote:
> On 08/19/2016 07:25 AM, Michael Kerrisk (man-pages) wrote:
>> This is an optional patch, to provide a small performance improvement.
>> Alter account_pipe_buffers() so that it returns the new value in
>> user->pipe_bufs. This means that we can refactor too_many_pipe_buffers_soft()
>> and too_many_pipe_buffers_hard() to avoid the costs of repeated use of
>> atomic_long_read() to get the value user->pipe_bufs.
> [...]
>> @@ -627,17 +625,18 @@ struct pipe_inode_info *alloc_pipe_info(void)
>>   	struct pipe_inode_info *pipe;
>>   	unsigned long pipe_bufs = PIPE_DEF_BUFFERS;
>>   	struct user_struct *user = get_current_user();
>> +	unsigned long num_bufs;
> 
> Maybe user_bufs would be more descriptive since num_bufs is a bit
> ambiguous without the context.

Okay -- changed.

>>   	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL_ACCOUNT);
>>   	if (pipe == NULL)
>>   		goto out_free_uid;
>>
>> -	if (too_many_pipe_buffers_soft(user))
>> +	if (too_many_pipe_buffers_soft(atomic_long_read(&user->pipe_bufs)))
>>   		pipe_bufs = 1;
>>
>> -	account_pipe_buffers(user, 0, pipe_bufs);
>> +	num_bufs = account_pipe_buffers(user, 0, pipe_bufs);
>>
>> -	if (too_many_pipe_buffers_hard(user))
>> +	if (too_many_pipe_buffers_hard(num_bufs))
>>   		goto out_revert_acct;
>>
>>   	pipe->bufs = kcalloc(pipe_bufs, sizeof(struct pipe_buffer),
> 
> Why not structure it like this?
> 
> num_bufs = account_pipe_buffers(user, 0, pipe_bufs);
> if (too_many_pipe_buffers_soft(num_bufs)) {
> 	num_bufs = account_pipe_buffers(user, pipe_bufs, 1);
> 	pipe_bufs = 1;
> }
> if (too_many_pipe_buffers_hard(num_bufs))
> 	goto out_revert_acct;
> 
> Otherwise you still have the case that somebody makes it past
> too_many_pipe_buffers_soft() before the accounting is done.

Ahh -- thanks! I knew there was a small glitch there, but decided to 
ignore it because didn't see the obvious solution. Fixed in 6/8 patch.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[PATCH 8/8] pipe: cap initial pipe capacity according to pipe-max-size limit

[Michael Kerrisk (man-pages)]

This is an patch that provides behavior that is more consistent,
and probably less surprising to users. I consider the change
optional, and welcome opinions about whether it should be applied.

By default, pipes are created with a capacity of 64 kiB.  However,
/proc/sys/fs/pipe-max-size may be set smaller than this value.  In
this scenario, an unprivileged user could thus create a pipe whose
initial capacity exceeds the limit. Therefore, it seems logical to
cap the initial pipe capacity according to the value of
pipe-max-size.

The test program shown earlier in this patch series can be used to
demonstrate the effect of the change brought about with this
patch:

    # cat /proc/sys/fs/pipe-max-size
    1048576
    # sudo -u mtk ./test_F_SETPIPE_SZ 1
    Initial pipe capacity: 65536
    # echo 10000 > /proc/sys/fs/pipe-max-size
    # cat /proc/sys/fs/pipe-max-size
    16384
    # sudo -u mtk ./test_F_SETPIPE_SZ 1
    Initial pipe capacity: 16384
    # ./test_F_SETPIPE_SZ 1
    Initial pipe capacity: 65536

The last two executions of 'test_F_SETPIPE_SZ' show that pipe-max-size
caps the initial allocation for a new pipe for unprivileged users, but
not for privileged users.

Cc: Willy Tarreau <w@1wt.eu>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: socketpair@gmail.com
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Jens Axboe <axboe@fb.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-api@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>

---
 fs/pipe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/pipe.c b/fs/pipe.c
index ada1777..caced8b 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -631,6 +631,9 @@ struct pipe_inode_info *alloc_pipe_info(void)
 	if (pipe == NULL)
 		goto out_free_uid;
 
+	if (!capable(CAP_SYS_RESOURCE) && pipe_bufs * PAGE_SIZE > pipe_max_size)
+		pipe_bufs = pipe_max_size >> PAGE_SHIFT;
+
 	if (too_many_pipe_buffers_soft(atomic_long_read(&user->pipe_bufs)))
 		pipe_bufs = 1;
 
-- 
2.5.5