DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 491C17E426
Subject: Re: [PATCH RFC 0/3] A few round_pipe_size() and pipe-max-size fixups
To: Randy Dunlap <rdunlap@infradead.org>, mtk.manpages@gmail.com
Cc: lkml <linux-kernel@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        "Luis R. Rodriguez" <mcgrof@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Mikulas Patocka <mpatocka@redhat.com>
References: <1504622676-2992-1-git-send-email-joe.lawrence@redhat.com>
 <CAKgNAkjrv_y9XMu947YxjfugAFS=539gJQhAZE9yASuooXFqxw@mail.gmail.com>
 <201487e9-235c-8604-50f2-02bd1170d8e8@infradead.org>
From: Joe Lawrence <joe.lawrence@redhat.com>
Organization: Red Hat
Message-ID: <70559d91-c90b-0060-2b16-3a392ade62a5@redhat.com>
Date: Thu, 14 Sep 2017 15:19:24 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <201487e9-235c-8604-50f2-02bd1170d8e8@infradead.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1465
Lines: 51

On 09/14/2017 12:57 PM, Randy Dunlap wrote:
> On 09/14/17 06:26, Michael Kerrisk (man-pages) wrote:
>> Hello Joe,
>>
>> On 5 September 2017 at 16:44, Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>> While backporting Michael's "pipe: fix limit handling" [1] patchset to a
>>> distro-kernel, Mikulas noticed that current upstream pipe limit handling
>>> contains a few problems:
>>>
>>>   1 - round_pipe_size() nr_pages overflow on 32bit:  this would
>>>       subsequently try roundup_pow_of_two(0), which is undefined.
> 
> Hi,
> Sorry I missed the initial posting of this.
> 
> The man page for F_SETPIPE_SZ (http://man7.org/linux/man-pages/man2/fcntl.2.html)
> says:
> "Attempts to set the pipe capacity below the page size are
> silently rounded up to the page size."
> 
> That implies to me that setting pipe size to 0 would round up to PAGE_SIZE.
> Doesn't patch 1/3 change that to return -EINVAL?

Good catch.  How about something like this:

/*
 * Minimum pipe size, as required by POSIX
 */
unsigned int pipe_min_size = PAGE_SIZE;

...

static inline unsigned int round_pipe_size(unsigned int size)
 {
        unsigned long nr_pages;

+       if (size < pipe_min_size)
+               size = pipe_min_size;
+
        nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
        if (nr_pages == 0)
                return 0;

> 
> Otherwise all 3 patches look good to me.

If the above is good, I can fold this into patch 1 and respin the set.

Thanks,

-- Joe