Hi, On Fri, Aug 12, 2016 at 02:35:40PM +0200, Vegard Nossum wrote: > I got this: > > > ================================================================================ > UBSAN: Undefined behaviour in ./include/linux/log2.h:63:13 > shift exponent 64 is too large for 64-bit type 'long unsigned int' > CPU: 0 PID: 5351 Comm: trinity-c0 Not tainted 4.8.0-rc1+ #84 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014 > 0000000000000000 ffff880115c67c08 ffffffff82344f40 0000000041b58ab3 > ffffffff84f98000 ffffffff82344e94 ffff880115c67c30 ffff880115c67be0 > 0000000000000001 ffff880115c679e8 dffffc0000000000 ffffffff85bf0820 > Call Trace: > [<ffffffff82344f40>] dump_stack+0xac/0xfc > [<ffffffff8242f5a8>] ubsan_epilogue+0xd/0x8a > [<ffffffff82430c31>] __ubsan_handle_shift_out_of_bounds+0x255/0x29a > [<ffffffff818229ab>] pipe_fcntl+0x59b/0x800 > [<ffffffff8184504a>] SyS_fcntl+0x69a/0xe50 > [<ffffffff81007bd3>] do_syscall_64+0x1b3/0x4b0 > [<ffffffff845f946a>] entry_SYSCALL64_slow_path+0x25/0x25 > > ================================================================================ > > 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. Regards, Willy
