Re: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ

[Vegard Nossum]

This is a multi-part message in MIME format.
On 08/15/2016 10:06 AM, Willy Tarreau wrote:
> On Fri, Aug 12, 2016 at 02:35:40PM +0200, Vegard Nossum wrote:
> > The problem is that if the argument (an unsigned long) passed to
> > F_SETPIPE_SZ is either 0 or greater than UINT_MAX, then
> > roundup_pow_of_two() will hit undefined behavior because the shift
> > width will be 64.
> >
> > Even if we limited the argument to UINT_MAX, we would still need to
> > keep the !nr_pages check, as passing anything greater than INT_MAX
> > will give a nr_pages inside round_pipe_size() of (1 << 20) which
> > then gets truncated to 0 when we convert it to an unsigned int
> > (because (1 << 20) << PAGE_SHIFT == 1 << 32).
>
> Why wouldn't we limit it to LONG_MAX and change round_pipe_size() to
> take an unsigned long in argument instead ? On 64-bit it would allow
> more than 2GB (even if I really doubt anybody will ever need this).
>
> Also, strictly speaking in your case it's not INT_MAX which is the
> absolute limit but UINT_MAX - PAGE_SIZE since it's a round up issue
> before being a shift issue. But that's mostly a detail I guess.
>
> Overall I think your change is right.

Hi,

Thanks for having a look.

In both cases I found it better to be more conservative in what we
accept, i.e. I haven't checked whether the rest of the code would
support pipe buffers > INT_MAX on 64-bit and I think it's a slightly
bigger job to check that (not just for the person making the change, but
for everybody else looking at/reviewing it) -- it's already tricky
enough to verify that this change by itself is safe and correct IMHO.

But I completely agree that being consistent in our int vs. long usage
would make the code easier to follow in terms of when something is
truncated or overflowing, so we should do that if we can.

Maybe we can relax the restrictions in a follow-up patch?

I was also playing around with consistently counting buffer sizes in
pages rather than bytes to avoid some of the shifts by PAGE_SHIFT,
although it doesn't win us very much. What do you think of the attached
patch?


Vegard