Re: UBSAN reports issue in ip_idents_reserve
On Tue, 2016-09-20 at 19:46 +0200, Jiri Pirko wrote: > > This patch makes ubsan silent. Thanks Jiri, I will post an official patch then ;)
Re: UBSAN reports issue in ip_idents_reserve
Tue, Sep 20, 2016 at 04:18:12PM CEST, eric.duma...@gmail.com wrote: >On Tue, 2016-09-20 at 07:11 -0700, Eric Dumazet wrote: >> On Tue, 2016-09-20 at 15:39 +0200, Jiri Pirko wrote: >> >> > I see. So how to silent the warning? >> > >> >> We can replace the atomic_add_return() and use a loop around >> atomic_read() and atomic_cmpxhg() >> >> This would change the nice property of x86 xadd into a loop. >> >> Or we also could fallback to random generation if the atomic_cmpxchg() >> fails. >> >> I'll provide a patch, thanks. >> > >Could you try the following ? > >diff --git a/net/ipv4/route.c b/net/ipv4/route.c >index >b52496fd51075821c39435f50ac62f813967aecc..91dc108ef6dc75df80f0e73b6fa062d98dc9a58a > 100644 >--- a/net/ipv4/route.c >+++ b/net/ipv4/route.c >@@ -476,12 +476,19 @@ u32 ip_idents_reserve(u32 hash, int segs) > atomic_t *p_id = ip_idents + hash % IP_IDENTS_SZ; > u32 old = ACCESS_ONCE(*p_tstamp); > u32 now = (u32)jiffies; >- u32 delta = 0; >+ u32 new, delta = 0; > > if (old != now && cmpxchg(p_tstamp, old, now) == old) > delta = prandom_u32_max(now - old); > >- return atomic_add_return(segs + delta, p_id) - segs; >+ old = (u32)atomic_read(p_id); >+ new = old + delta + segs; >+ /* Do not try too hard, if multiple cpus are there, >+ * just fallback to pseudo random number. >+ */ >+ if (unlikely(atomic_cmpxchg(p_id, old, new) != old)) >+ new = prandom_u32(); >+ return new; > } > EXPORT_SYMBOL(ip_idents_reserve); > This patch makes ubsan silent. > >
Re: UBSAN reports issue in ip_idents_reserve
Tue, Sep 20, 2016 at 05:25:16PM CEST, eric.duma...@gmail.com wrote: >On Tue, 2016-09-20 at 07:11 -0700, Eric Dumazet wrote: >> On Tue, 2016-09-20 at 15:39 +0200, Jiri Pirko wrote: >> >> > I see. So how to silent the warning? >> > >> >> We can replace the atomic_add_return() and use a loop around >> atomic_read() and atomic_cmpxhg() >> >> This would change the nice property of x86 xadd into a loop. >> >> Or we also could fallback to random generation if the atomic_cmpxchg() >> fails. >> >> I'll provide a patch, thanks. >> I'm going to test your patch now. > >I looks at other places, I am surprised you do not see other UBSAN >issues in networking :) Not yet :) > >netdev_refcnt_read() can potentially gives errors as well. > > >
Re: UBSAN reports issue in ip_idents_reserve
On Tue, 2016-09-20 at 07:11 -0700, Eric Dumazet wrote: > On Tue, 2016-09-20 at 15:39 +0200, Jiri Pirko wrote: > > > I see. So how to silent the warning? > > > > We can replace the atomic_add_return() and use a loop around > atomic_read() and atomic_cmpxhg() > > This would change the nice property of x86 xadd into a loop. > > Or we also could fallback to random generation if the atomic_cmpxchg() > fails. > > I'll provide a patch, thanks. > I looks at other places, I am surprised you do not see other UBSAN issues in networking :) netdev_refcnt_read() can potentially gives errors as well.
Re: UBSAN reports issue in ip_idents_reserve
On Tue, 2016-09-20 at 15:39 +0200, Jiri Pirko wrote: > I see. So how to silent the warning? > We can replace the atomic_add_return() and use a loop around atomic_read() and atomic_cmpxhg() This would change the nice property of x86 xadd into a loop. Or we also could fallback to random generation if the atomic_cmpxchg() fails. I'll provide a patch, thanks.
Re: UBSAN reports issue in ip_idents_reserve
On Tue, 2016-09-20 at 07:11 -0700, Eric Dumazet wrote: > On Tue, 2016-09-20 at 15:39 +0200, Jiri Pirko wrote: > > > I see. So how to silent the warning? > > > > We can replace the atomic_add_return() and use a loop around > atomic_read() and atomic_cmpxhg() > > This would change the nice property of x86 xadd into a loop. > > Or we also could fallback to random generation if the atomic_cmpxchg() > fails. > > I'll provide a patch, thanks. > Could you try the following ? diff --git a/net/ipv4/route.c b/net/ipv4/route.c index b52496fd51075821c39435f50ac62f813967aecc..91dc108ef6dc75df80f0e73b6fa062d98dc9a58a 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -476,12 +476,19 @@ u32 ip_idents_reserve(u32 hash, int segs) atomic_t *p_id = ip_idents + hash % IP_IDENTS_SZ; u32 old = ACCESS_ONCE(*p_tstamp); u32 now = (u32)jiffies; - u32 delta = 0; + u32 new, delta = 0; if (old != now && cmpxchg(p_tstamp, old, now) == old) delta = prandom_u32_max(now - old); - return atomic_add_return(segs + delta, p_id) - segs; + old = (u32)atomic_read(p_id); + new = old + delta + segs; + /* Do not try too hard, if multiple cpus are there, +* just fallback to pseudo random number. +*/ + if (unlikely(atomic_cmpxchg(p_id, old, new) != old)) + new = prandom_u32(); + return new; } EXPORT_SYMBOL(ip_idents_reserve);
Re: UBSAN reports issue in ip_idents_reserve
On Tue, 2016-09-20 at 07:18 -0700, Eric Dumazet wrote: > + */ > + if (unlikely(atomic_cmpxchg(p_id, old, new) != old)) > + new = prandom_u32(); > + return new; Looks like we should return new - segs;
Re: UBSAN reports issue in ip_idents_reserve
On Tue, 2016-09-20 at 13:36 +, David Laight wrote: > From: Eric Dumazet > > Sent: 20 September 2016 14:29 > ... > > > [ 47.565420] -2117905507 + -695755206 cannot be represented in type > > > 'int' > ... > > I do not think we have to worry here. > > > > These is best effort, and unfortunately atomic_t are int. > > Not until we compile on a cpu where int arithmetic doesn't wrap. Then I guess the guy adding this kind of arches in the kernel will have to add all the core kernel infra. If you have an idea, I will happily review a patch.
RE: UBSAN reports issue in ip_idents_reserve
From: Eric Dumazet > Sent: 20 September 2016 14:29 ... > > [ 47.565420] -2117905507 + -695755206 cannot be represented in type 'int' ... > I do not think we have to worry here. > > These is best effort, and unfortunately atomic_t are int. Not until we compile on a cpu where int arithmetic doesn't wrap. While I expect that various other parts of the kernel (and userspace) wouldn't like such cpu, they do exist. David
Re: UBSAN reports issue in ip_idents_reserve
Tue, Sep 20, 2016 at 03:28:35PM CEST, eric.duma...@gmail.com wrote: >On Tue, 2016-09-20 at 14:00 +0200, Jiri Pirko wrote: >> Hi. >> >> I'm consistently getting following UBSAN warning on every bootup: >> >> [ 47.545820] >> >> [ 47.554340] UBSAN: Undefined behaviour in >> ./arch/x86/include/asm/atomic.h:156:11 >> [ 47.561808] signed integer overflow: >> [ 47.565420] -2117905507 + -695755206 cannot be represented in type 'int' >> [ 47.572226] CPU: 0 PID: 389 Comm: ntpd Not tainted 4.8.0-rc6jiri+ #1 >> [ 47.578636] Hardware name: Mellanox Technologies Ltd. Mellanox >> switch/Mellanox switch, BIOS 4.6.5 05/21/2015 >> [ 47.588586] 847bf8c0 987b8f47 8803829af5a8 >> 818354e3 >> [ 47.596165] 41b58ab3 8277e711 81835431 >> 8803829af5d0 >> [ 47.603722] 8803829af580 d6879e3a 108f8214 >> ed0070535e6c >> [ 47.611298] Call Trace: >> [ 47.613795] [] dump_stack+0xb2/0x10f >> [ 47.619077] [] ? _atomic_dec_and_lock+0xa1/0xa1 >> [ 47.625327] [] ubsan_epilogue+0xd/0x4e >> [ 47.630811] [] handle_overflow+0x190/0x1de >> [ 47.636627] [] ? >> __ubsan_handle_negate_overflow+0x140/0x140 >> [ 47.643914] [] ? >> iov_iter_copy_from_user_atomic+0x6e0/0x6e0 >> [ 47.651219] [] ? __lock_acquire.isra.17+0xb79/0xe50 >> [ 47.657832] [] ? ip_generic_getfrag+0xd2/0x190 >> [ 47.664011] [] ? ip_setup_cork+0x320/0x320 >> [ 47.669827] [] __ubsan_handle_add_overflow+0xe/0x10 >> [ 47.676444] [] ip_idents_reserve+0xb2/0xe0 >> [ 47.682254] [] __ip_select_ident+0x159/0x1b0 >> [ 47.688248] [] ? update_or_create_fnhe+0x850/0x850 >> [ 47.694782] [] ? ip_setup_cork+0x320/0x320 >> [ 47.700624] [] __ip_make_skb+0x8a0/0xab0 >> [ 47.706259] [] ip_make_skb+0x17d/0x1d0 >> [ 47.711717] [] ? ip_setup_cork+0x320/0x320 >> [ 47.717526] [] ? ip_flush_pending_frames+0x20/0x20 >> [ 47.724032] [] ? ip_rt_update_pmtu+0x4f0/0x4f0 >> [ 47.730231] [] ? xfrm_lookup_route+0x21/0xe0 >> [ 47.736216] [] udp_sendmsg+0x9db/0xf60 >> [ 47.741668] [] ? ip_setup_cork+0x320/0x320 >> [ 47.747472] [] ? udp_abort+0x70/0x70 >> [ 47.752763] [] inet_sendmsg+0x198/0x220 >> [ 47.758324] [] ? inet_sendmsg+0x52/0x220 >> [ 47.763982] [] ? inet_recvmsg+0x300/0x300 >> [ 47.769728] [] sock_sendmsg+0xa5/0xd0 >> [ 47.775100] [] SYSC_sendto+0x1d0/0x280 >> [ 47.780551] [] ? SYSC_connect+0x200/0x200 >> [ 47.786283] [] ? poll_select_copy_remaining+0x2af/0x310 >> [ 47.793265] [] ? set_fd_set+0x60/0x60 >> [ 47.798665] [] ? do_raw_spin_trylock+0x90/0x90 >> [ 47.804853] [] ? SyS_select+0x1a3/0x200 >> [ 47.810399] [] ? core_sys_select+0x570/0x570 >> [ 47.816415] [] ? exit_to_usermode_loop+0xec/0x110 >> [ 47.822842] [] ? lockdep_sys_exit+0x2d/0xb0 >> [ 47.828769] [] ? lockdep_sys_exit_thunk+0x16/0x30 >> [ 47.835199] [] SyS_sendto+0xe/0x10 >> [ 47.840321] [] entry_SYSCALL_64_fastpath+0x1a/0xa9 >> [ 47.846826] >> >> >> Looks like this might be result of following commit: >> >> commit 04ca6973f7c1a0d8537f2d9906a0cf8e69886d75 >> Author: Eric Dumazet >> Date: Sat Jul 26 08:58:10 2014 +0200 >> >> ip: make IP identifiers less predictable >> >> Eric, could you please take look at that? > >Sure > >I do not think we have to worry here. > >These is best effort, and unfortunately atomic_t are int. I see. So how to silent the warning? > >Adding uatomic_t helpers in the kernel with unsigned int would be a huge >effort, given this would touch all arches. > > >
Re: UBSAN reports issue in ip_idents_reserve
On Tue, 2016-09-20 at 14:00 +0200, Jiri Pirko wrote: > Hi. > > I'm consistently getting following UBSAN warning on every bootup: > > [ 47.545820] > > [ 47.554340] UBSAN: Undefined behaviour in > ./arch/x86/include/asm/atomic.h:156:11 > [ 47.561808] signed integer overflow: > [ 47.565420] -2117905507 + -695755206 cannot be represented in type 'int' > [ 47.572226] CPU: 0 PID: 389 Comm: ntpd Not tainted 4.8.0-rc6jiri+ #1 > [ 47.578636] Hardware name: Mellanox Technologies Ltd. Mellanox > switch/Mellanox switch, BIOS 4.6.5 05/21/2015 > [ 47.588586] 847bf8c0 987b8f47 8803829af5a8 > 818354e3 > [ 47.596165] 41b58ab3 8277e711 81835431 > 8803829af5d0 > [ 47.603722] 8803829af580 d6879e3a 108f8214 > ed0070535e6c > [ 47.611298] Call Trace: > [ 47.613795] [] dump_stack+0xb2/0x10f > [ 47.619077] [] ? _atomic_dec_and_lock+0xa1/0xa1 > [ 47.625327] [] ubsan_epilogue+0xd/0x4e > [ 47.630811] [] handle_overflow+0x190/0x1de > [ 47.636627] [] ? > __ubsan_handle_negate_overflow+0x140/0x140 > [ 47.643914] [] ? > iov_iter_copy_from_user_atomic+0x6e0/0x6e0 > [ 47.651219] [] ? __lock_acquire.isra.17+0xb79/0xe50 > [ 47.657832] [] ? ip_generic_getfrag+0xd2/0x190 > [ 47.664011] [] ? ip_setup_cork+0x320/0x320 > [ 47.669827] [] __ubsan_handle_add_overflow+0xe/0x10 > [ 47.676444] [] ip_idents_reserve+0xb2/0xe0 > [ 47.682254] [] __ip_select_ident+0x159/0x1b0 > [ 47.688248] [] ? update_or_create_fnhe+0x850/0x850 > [ 47.694782] [] ? ip_setup_cork+0x320/0x320 > [ 47.700624] [] __ip_make_skb+0x8a0/0xab0 > [ 47.706259] [] ip_make_skb+0x17d/0x1d0 > [ 47.711717] [] ? ip_setup_cork+0x320/0x320 > [ 47.717526] [] ? ip_flush_pending_frames+0x20/0x20 > [ 47.724032] [] ? ip_rt_update_pmtu+0x4f0/0x4f0 > [ 47.730231] [] ? xfrm_lookup_route+0x21/0xe0 > [ 47.736216] [] udp_sendmsg+0x9db/0xf60 > [ 47.741668] [] ? ip_setup_cork+0x320/0x320 > [ 47.747472] [] ? udp_abort+0x70/0x70 > [ 47.752763] [] inet_sendmsg+0x198/0x220 > [ 47.758324] [] ? inet_sendmsg+0x52/0x220 > [ 47.763982] [] ? inet_recvmsg+0x300/0x300 > [ 47.769728] [] sock_sendmsg+0xa5/0xd0 > [ 47.775100] [] SYSC_sendto+0x1d0/0x280 > [ 47.780551] [] ? SYSC_connect+0x200/0x200 > [ 47.786283] [] ? poll_select_copy_remaining+0x2af/0x310 > [ 47.793265] [] ? set_fd_set+0x60/0x60 > [ 47.798665] [] ? do_raw_spin_trylock+0x90/0x90 > [ 47.804853] [] ? SyS_select+0x1a3/0x200 > [ 47.810399] [] ? core_sys_select+0x570/0x570 > [ 47.816415] [] ? exit_to_usermode_loop+0xec/0x110 > [ 47.822842] [] ? lockdep_sys_exit+0x2d/0xb0 > [ 47.828769] [] ? lockdep_sys_exit_thunk+0x16/0x30 > [ 47.835199] [] SyS_sendto+0xe/0x10 > [ 47.840321] [] entry_SYSCALL_64_fastpath+0x1a/0xa9 > [ 47.846826] > > > Looks like this might be result of following commit: > > commit 04ca6973f7c1a0d8537f2d9906a0cf8e69886d75 > Author: Eric Dumazet > Date: Sat Jul 26 08:58:10 2014 +0200 > > ip: make IP identifiers less predictable > > Eric, could you please take look at that? Sure I do not think we have to worry here. These is best effort, and unfortunately atomic_t are int. Adding uatomic_t helpers in the kernel with unsigned int would be a huge effort, given this would touch all arches.
