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

[Willy Tarreau]

On Mon, Aug 15, 2016 at 10:22:38AM +0200, Vegard Nossum wrote:
> 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.

Well in fact in my opinion it's the opposite, because if we ensure the
function works well over all its argument type's range, the caller has
less trouble figuring what sub-part of the range is OK. This is exactly
the current issue where you have to ensure that :

     unsigned int arg <= INT_MAX

> 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?

Yes possibly.

> 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?

I'm not sure it improves readability. And in general I don't like having
two variables to set a same limit, because some code parts tend to rely
on one of them and other code parts use the other one, so it's harder to
keep consistency all over the code. If you manage to totally get rid of
pipe_max_size and make pipe_proc_fn() completely emulate it using
pipe_max_pages, then it may be clearer and will also get rid of some of
the comments that are here to explain the inconsistencies.

Willy