Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1732269 > unrolled thread

Re: [PATCH RFC 0/3] A few round_pipe_size() and pipe-max-size fixups

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 0/3] A few round_pipe_size() and pipe-max-size fixups "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2017-09-14 15:30 +0200
    Re: [PATCH RFC 0/3] A few round_pipe_size() and pipe-max-size fixups Randy Dunlap <rdunlap@infradead.org> - 2017-09-14 19:00 +0200
      Re: [PATCH RFC 0/3] A few round_pipe_size() and pipe-max-size fixups Joe Lawrence <joe.lawrence@redhat.com> - 2017-09-14 21:20 +0200
        Re: [PATCH RFC 0/3] A few round_pipe_size() and pipe-max-size fixups Randy Dunlap <rdunlap@infradead.org> - 2017-09-14 22:30 +0200

#1732269 — Re: [PATCH RFC 0/3] A few round_pipe_size() and pipe-max-size fixups

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.
>
>   2 - visible non-rounded pipe-max-size value: there is no mutual
>       exclusion or protection between the time pipe_max_size is assigned
>       a raw value from proc_dointvec_minmax() and when it is rounded.
>
>   3 - procfs signed wrap: echo'ing a large number into
>       /proc/sys/fs/pipe-max-size and then cat'ing it back out shows a
>       negative value.
>
>
> This RFC serves as a bug report and a contains a few possible fixes.
> There may be better / more consistent ways to fix the overflows and
> procfs bugs, but I figured I'd throw an RFC w/code out there for initial
> conversation.  Suggestions welcome!

Thank for working on this. I have no improvements to suggest. The
patches all look sane to me. For the whole series:

Reviewed-by: MIchael Kerrisk <mtk.manpages@gmail.com>

Cheers,

Michael


> Testing
> =======
>
> Patch 1 - 32bit overflow
> ------------------------
> From userspace:
>
>   fcntl(fd, F_SETPIPE_SZ, 0xffffffff);
>
> - Before the fix, return value was 4096 as pipe size overflowed and
>   was set to 4096
>
> - After the fix, returns -1 and sets errno EINVAL, pipe size remains
>   untouched
>
>
> Patch 2 - non-rounded pipe-max-size value
> -----------------------------------------
> Keep plugging in values that need to be rounded:
>
>   while (true); do echo 1048570 > /proc/sys/fs/pipe-max-size; done
>
> and in another terminal, loop around reading the value:
>
>   time (while (true); do SIZE=$(cat /proc/sys/fs/pipe-max-size); [[ $(( $SIZE % 4096 )) -ne 0 ]] && break; done; echo "$SIZE")
>   1048570
>
>   real    0m46.213s
>   user    0m29.688s
>   sys     0m20.042s
>
> after the fix, the test loop never encountered a non-page-rounded value.
>
>
> Patch 3 - procfs signed wrap
> ----------------------------
> Before:
>
>   % echo 2147483647 >/proc/sys/fs/pipe-max-size
>   % cat /proc/sys/fs/pipe-max-size
>   -2147483648
>
> After:
>
>   % echo 2147483647 >/proc/sys/fs/pipe-max-size
>   % cat /proc/sys/fs/pipe-max-size
>   2147483648
>
>
> Joe Lawrence (3):
>   pipe: avoid round_pipe_size() nr_pages overflow on 32-bit
>   pipe: protect pipe_max_size access with a mutex
>   pipe: match pipe_max_size data type with procfs
>
>  fs/pipe.c       | 48 +++++++++++++++++++++++++++++++++++++++++-------
>  kernel/sysctl.c |  2 +-
>  2 files changed, 42 insertions(+), 8 deletions(-)
>
> --
> 1.8.3.1
>



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

[toc] | [next] | [standalone]

#1732460

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?


Otherwise all 3 patches look good to me.

thanks,
-- 
~Randy

[toc] | [prev] | [next] | [standalone]

#1732525

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

[toc] | [prev] | [next] | [standalone]

#1732555

On 09/14/17 12:19, Joe Lawrence wrote:
> 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.

Yes, looks good to me.  Thanks.


-- 
~Randy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web