* UBSAN reports issue in ip_idents_reserve
@ 2016-09-20 12:00 Jiri Pirko
2016-09-20 13:28 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2016-09-20 12:00 UTC (permalink / raw)
To: netdev, eric.dumazet
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] ffffffff847bf8c0 00000000987b8f47 ffff8803829af5a8 ffffffff818354e3
[ 47.596165] 0000000041b58ab3 ffffffff8277e711 ffffffff81835431 ffff8803829af5d0
[ 47.603722] ffff8803829af580 ffffffffd6879e3a 1ffffffff08f8214 ffffed0070535e6c
[ 47.611298] Call Trace:
[ 47.613795] [<ffffffff818354e3>] dump_stack+0xb2/0x10f
[ 47.619077] [<ffffffff81835431>] ? _atomic_dec_and_lock+0xa1/0xa1
[ 47.625327] [<ffffffff818a884f>] ubsan_epilogue+0xd/0x4e
[ 47.630811] [<ffffffff818a9821>] handle_overflow+0x190/0x1de
[ 47.636627] [<ffffffff818a9691>] ? __ubsan_handle_negate_overflow+0x140/0x140
[ 47.643914] [<ffffffff81863130>] ? iov_iter_copy_from_user_atomic+0x6e0/0x6e0
[ 47.651219] [<ffffffff811e6f79>] ? __lock_acquire.isra.17+0xb79/0xe50
[ 47.657832] [<ffffffff81e581f2>] ? ip_generic_getfrag+0xd2/0x190
[ 47.664011] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
[ 47.669827] [<ffffffff818a987d>] __ubsan_handle_add_overflow+0xe/0x10
[ 47.676444] [<ffffffff81e41d52>] ip_idents_reserve+0xb2/0xe0
[ 47.682254] [<ffffffff81e443e9>] __ip_select_ident+0x159/0x1b0
[ 47.688248] [<ffffffff81e44290>] ? update_or_create_fnhe+0x850/0x850
[ 47.694782] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
[ 47.700624] [<ffffffff81e5ef40>] __ip_make_skb+0x8a0/0xab0
[ 47.706259] [<ffffffff81e5f3fd>] ip_make_skb+0x17d/0x1d0
[ 47.711717] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
[ 47.717526] [<ffffffff81e5f280>] ? ip_flush_pending_frames+0x20/0x20
[ 47.724032] [<ffffffff81e46ef0>] ? ip_rt_update_pmtu+0x4f0/0x4f0
[ 47.730231] [<ffffffff81f35291>] ? xfrm_lookup_route+0x21/0xe0
[ 47.736216] [<ffffffff81ec0cdb>] udp_sendmsg+0x9db/0xf60
[ 47.741668] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
[ 47.747472] [<ffffffff81ec0300>] ? udp_abort+0x70/0x70
[ 47.752763] [<ffffffff81ede3d8>] inet_sendmsg+0x198/0x220
[ 47.758324] [<ffffffff81ede292>] ? inet_sendmsg+0x52/0x220
[ 47.763982] [<ffffffff81ede240>] ? inet_recvmsg+0x300/0x300
[ 47.769728] [<ffffffff81d6fd25>] sock_sendmsg+0xa5/0xd0
[ 47.775100] [<ffffffff81d72f70>] SYSC_sendto+0x1d0/0x280
[ 47.780551] [<ffffffff81d72da0>] ? SYSC_connect+0x200/0x200
[ 47.786283] [<ffffffff814f66df>] ? poll_select_copy_remaining+0x2af/0x310
[ 47.793265] [<ffffffff814f6430>] ? set_fd_set+0x60/0x60
[ 47.798665] [<ffffffff811ee360>] ? do_raw_spin_trylock+0x90/0x90
[ 47.804853] [<ffffffff814f80e3>] ? SyS_select+0x1a3/0x200
[ 47.810399] [<ffffffff814f7f40>] ? core_sys_select+0x570/0x570
[ 47.816415] [<ffffffff8100467c>] ? exit_to_usermode_loop+0xec/0x110
[ 47.822842] [<ffffffff811e8abd>] ? lockdep_sys_exit+0x2d/0xb0
[ 47.828769] [<ffffffff81004016>] ? lockdep_sys_exit_thunk+0x16/0x30
[ 47.835199] [<ffffffff81d7433e>] SyS_sendto+0xe/0x10
[ 47.840321] [<ffffffff820381f2>] entry_SYSCALL_64_fastpath+0x1a/0xa9
[ 47.846826] ================================================================================
Looks like this might be result of following commit:
commit 04ca6973f7c1a0d8537f2d9906a0cf8e69886d75
Author: Eric Dumazet <edumazet@google.com>
Date: Sat Jul 26 08:58:10 2014 +0200
ip: make IP identifiers less predictable
Eric, could you please take look at that?
Thanks.
Jiri
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 12:00 UBSAN reports issue in ip_idents_reserve Jiri Pirko
@ 2016-09-20 13:28 ` Eric Dumazet
2016-09-20 13:36 ` David Laight
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Eric Dumazet @ 2016-09-20 13:28 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
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] ffffffff847bf8c0 00000000987b8f47 ffff8803829af5a8 ffffffff818354e3
> [ 47.596165] 0000000041b58ab3 ffffffff8277e711 ffffffff81835431 ffff8803829af5d0
> [ 47.603722] ffff8803829af580 ffffffffd6879e3a 1ffffffff08f8214 ffffed0070535e6c
> [ 47.611298] Call Trace:
> [ 47.613795] [<ffffffff818354e3>] dump_stack+0xb2/0x10f
> [ 47.619077] [<ffffffff81835431>] ? _atomic_dec_and_lock+0xa1/0xa1
> [ 47.625327] [<ffffffff818a884f>] ubsan_epilogue+0xd/0x4e
> [ 47.630811] [<ffffffff818a9821>] handle_overflow+0x190/0x1de
> [ 47.636627] [<ffffffff818a9691>] ? __ubsan_handle_negate_overflow+0x140/0x140
> [ 47.643914] [<ffffffff81863130>] ? iov_iter_copy_from_user_atomic+0x6e0/0x6e0
> [ 47.651219] [<ffffffff811e6f79>] ? __lock_acquire.isra.17+0xb79/0xe50
> [ 47.657832] [<ffffffff81e581f2>] ? ip_generic_getfrag+0xd2/0x190
> [ 47.664011] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
> [ 47.669827] [<ffffffff818a987d>] __ubsan_handle_add_overflow+0xe/0x10
> [ 47.676444] [<ffffffff81e41d52>] ip_idents_reserve+0xb2/0xe0
> [ 47.682254] [<ffffffff81e443e9>] __ip_select_ident+0x159/0x1b0
> [ 47.688248] [<ffffffff81e44290>] ? update_or_create_fnhe+0x850/0x850
> [ 47.694782] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
> [ 47.700624] [<ffffffff81e5ef40>] __ip_make_skb+0x8a0/0xab0
> [ 47.706259] [<ffffffff81e5f3fd>] ip_make_skb+0x17d/0x1d0
> [ 47.711717] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
> [ 47.717526] [<ffffffff81e5f280>] ? ip_flush_pending_frames+0x20/0x20
> [ 47.724032] [<ffffffff81e46ef0>] ? ip_rt_update_pmtu+0x4f0/0x4f0
> [ 47.730231] [<ffffffff81f35291>] ? xfrm_lookup_route+0x21/0xe0
> [ 47.736216] [<ffffffff81ec0cdb>] udp_sendmsg+0x9db/0xf60
> [ 47.741668] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
> [ 47.747472] [<ffffffff81ec0300>] ? udp_abort+0x70/0x70
> [ 47.752763] [<ffffffff81ede3d8>] inet_sendmsg+0x198/0x220
> [ 47.758324] [<ffffffff81ede292>] ? inet_sendmsg+0x52/0x220
> [ 47.763982] [<ffffffff81ede240>] ? inet_recvmsg+0x300/0x300
> [ 47.769728] [<ffffffff81d6fd25>] sock_sendmsg+0xa5/0xd0
> [ 47.775100] [<ffffffff81d72f70>] SYSC_sendto+0x1d0/0x280
> [ 47.780551] [<ffffffff81d72da0>] ? SYSC_connect+0x200/0x200
> [ 47.786283] [<ffffffff814f66df>] ? poll_select_copy_remaining+0x2af/0x310
> [ 47.793265] [<ffffffff814f6430>] ? set_fd_set+0x60/0x60
> [ 47.798665] [<ffffffff811ee360>] ? do_raw_spin_trylock+0x90/0x90
> [ 47.804853] [<ffffffff814f80e3>] ? SyS_select+0x1a3/0x200
> [ 47.810399] [<ffffffff814f7f40>] ? core_sys_select+0x570/0x570
> [ 47.816415] [<ffffffff8100467c>] ? exit_to_usermode_loop+0xec/0x110
> [ 47.822842] [<ffffffff811e8abd>] ? lockdep_sys_exit+0x2d/0xb0
> [ 47.828769] [<ffffffff81004016>] ? lockdep_sys_exit_thunk+0x16/0x30
> [ 47.835199] [<ffffffff81d7433e>] SyS_sendto+0xe/0x10
> [ 47.840321] [<ffffffff820381f2>] entry_SYSCALL_64_fastpath+0x1a/0xa9
> [ 47.846826] ================================================================================
>
> Looks like this might be result of following commit:
>
> commit 04ca6973f7c1a0d8537f2d9906a0cf8e69886d75
> Author: Eric Dumazet <edumazet@google.com>
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: UBSAN reports issue in ip_idents_reserve
2016-09-20 13:28 ` Eric Dumazet
@ 2016-09-20 13:36 ` David Laight
2016-09-20 13:45 ` Eric Dumazet
2016-09-20 13:39 ` Jiri Pirko
2016-09-21 1:06 ` [PATCH net] net: get rid of an signed integer overflow in ip_idents_reserve() Eric Dumazet
2 siblings, 1 reply; 14+ messages in thread
From: David Laight @ 2016-09-20 13:36 UTC (permalink / raw)
To: 'Eric Dumazet', Jiri Pirko; +Cc: netdev
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 13:28 ` Eric Dumazet
2016-09-20 13:36 ` David Laight
@ 2016-09-20 13:39 ` Jiri Pirko
2016-09-20 14:11 ` Eric Dumazet
2016-09-21 1:06 ` [PATCH net] net: get rid of an signed integer overflow in ip_idents_reserve() Eric Dumazet
2 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2016-09-20 13:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
Tue, Sep 20, 2016 at 03:28:35PM CEST, eric.dumazet@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] ffffffff847bf8c0 00000000987b8f47 ffff8803829af5a8 ffffffff818354e3
>> [ 47.596165] 0000000041b58ab3 ffffffff8277e711 ffffffff81835431 ffff8803829af5d0
>> [ 47.603722] ffff8803829af580 ffffffffd6879e3a 1ffffffff08f8214 ffffed0070535e6c
>> [ 47.611298] Call Trace:
>> [ 47.613795] [<ffffffff818354e3>] dump_stack+0xb2/0x10f
>> [ 47.619077] [<ffffffff81835431>] ? _atomic_dec_and_lock+0xa1/0xa1
>> [ 47.625327] [<ffffffff818a884f>] ubsan_epilogue+0xd/0x4e
>> [ 47.630811] [<ffffffff818a9821>] handle_overflow+0x190/0x1de
>> [ 47.636627] [<ffffffff818a9691>] ? __ubsan_handle_negate_overflow+0x140/0x140
>> [ 47.643914] [<ffffffff81863130>] ? iov_iter_copy_from_user_atomic+0x6e0/0x6e0
>> [ 47.651219] [<ffffffff811e6f79>] ? __lock_acquire.isra.17+0xb79/0xe50
>> [ 47.657832] [<ffffffff81e581f2>] ? ip_generic_getfrag+0xd2/0x190
>> [ 47.664011] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
>> [ 47.669827] [<ffffffff818a987d>] __ubsan_handle_add_overflow+0xe/0x10
>> [ 47.676444] [<ffffffff81e41d52>] ip_idents_reserve+0xb2/0xe0
>> [ 47.682254] [<ffffffff81e443e9>] __ip_select_ident+0x159/0x1b0
>> [ 47.688248] [<ffffffff81e44290>] ? update_or_create_fnhe+0x850/0x850
>> [ 47.694782] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
>> [ 47.700624] [<ffffffff81e5ef40>] __ip_make_skb+0x8a0/0xab0
>> [ 47.706259] [<ffffffff81e5f3fd>] ip_make_skb+0x17d/0x1d0
>> [ 47.711717] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
>> [ 47.717526] [<ffffffff81e5f280>] ? ip_flush_pending_frames+0x20/0x20
>> [ 47.724032] [<ffffffff81e46ef0>] ? ip_rt_update_pmtu+0x4f0/0x4f0
>> [ 47.730231] [<ffffffff81f35291>] ? xfrm_lookup_route+0x21/0xe0
>> [ 47.736216] [<ffffffff81ec0cdb>] udp_sendmsg+0x9db/0xf60
>> [ 47.741668] [<ffffffff81e58120>] ? ip_setup_cork+0x320/0x320
>> [ 47.747472] [<ffffffff81ec0300>] ? udp_abort+0x70/0x70
>> [ 47.752763] [<ffffffff81ede3d8>] inet_sendmsg+0x198/0x220
>> [ 47.758324] [<ffffffff81ede292>] ? inet_sendmsg+0x52/0x220
>> [ 47.763982] [<ffffffff81ede240>] ? inet_recvmsg+0x300/0x300
>> [ 47.769728] [<ffffffff81d6fd25>] sock_sendmsg+0xa5/0xd0
>> [ 47.775100] [<ffffffff81d72f70>] SYSC_sendto+0x1d0/0x280
>> [ 47.780551] [<ffffffff81d72da0>] ? SYSC_connect+0x200/0x200
>> [ 47.786283] [<ffffffff814f66df>] ? poll_select_copy_remaining+0x2af/0x310
>> [ 47.793265] [<ffffffff814f6430>] ? set_fd_set+0x60/0x60
>> [ 47.798665] [<ffffffff811ee360>] ? do_raw_spin_trylock+0x90/0x90
>> [ 47.804853] [<ffffffff814f80e3>] ? SyS_select+0x1a3/0x200
>> [ 47.810399] [<ffffffff814f7f40>] ? core_sys_select+0x570/0x570
>> [ 47.816415] [<ffffffff8100467c>] ? exit_to_usermode_loop+0xec/0x110
>> [ 47.822842] [<ffffffff811e8abd>] ? lockdep_sys_exit+0x2d/0xb0
>> [ 47.828769] [<ffffffff81004016>] ? lockdep_sys_exit_thunk+0x16/0x30
>> [ 47.835199] [<ffffffff81d7433e>] SyS_sendto+0xe/0x10
>> [ 47.840321] [<ffffffff820381f2>] entry_SYSCALL_64_fastpath+0x1a/0xa9
>> [ 47.846826] ================================================================================
>>
>> Looks like this might be result of following commit:
>>
>> commit 04ca6973f7c1a0d8537f2d9906a0cf8e69886d75
>> Author: Eric Dumazet <edumazet@google.com>
>> 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.
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 13:36 ` David Laight
@ 2016-09-20 13:45 ` Eric Dumazet
0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2016-09-20 13:45 UTC (permalink / raw)
To: David Laight; +Cc: Jiri Pirko, netdev
On Tue, 2016-09-20 at 13:36 +0000, 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 13:39 ` Jiri Pirko
@ 2016-09-20 14:11 ` Eric Dumazet
2016-09-20 14:18 ` Eric Dumazet
2016-09-20 15:25 ` Eric Dumazet
0 siblings, 2 replies; 14+ messages in thread
From: Eric Dumazet @ 2016-09-20 14:11 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 14:11 ` Eric Dumazet
@ 2016-09-20 14:18 ` Eric Dumazet
2016-09-20 14:24 ` Eric Dumazet
2016-09-20 17:46 ` Jiri Pirko
2016-09-20 15:25 ` Eric Dumazet
1 sibling, 2 replies; 14+ messages in thread
From: Eric Dumazet @ 2016-09-20 14:18 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
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);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 14:18 ` Eric Dumazet
@ 2016-09-20 14:24 ` Eric Dumazet
2016-09-20 17:46 ` Jiri Pirko
1 sibling, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2016-09-20 14:24 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 14:11 ` Eric Dumazet
2016-09-20 14:18 ` Eric Dumazet
@ 2016-09-20 15:25 ` Eric Dumazet
2016-09-20 16:25 ` Jiri Pirko
1 sibling, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2016-09-20 15:25 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 15:25 ` Eric Dumazet
@ 2016-09-20 16:25 ` Jiri Pirko
0 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2016-09-20 16:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
Tue, Sep 20, 2016 at 05:25:16PM CEST, eric.dumazet@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.
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 14:18 ` Eric Dumazet
2016-09-20 14:24 ` Eric Dumazet
@ 2016-09-20 17:46 ` Jiri Pirko
2016-09-20 18:42 ` Eric Dumazet
1 sibling, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2016-09-20 17:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
Tue, Sep 20, 2016 at 04:18:12PM CEST, eric.dumazet@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.
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBSAN reports issue in ip_idents_reserve
2016-09-20 17:46 ` Jiri Pirko
@ 2016-09-20 18:42 ` Eric Dumazet
0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2016-09-20 18:42 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev
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 ;)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net] net: get rid of an signed integer overflow in ip_idents_reserve()
2016-09-20 13:28 ` Eric Dumazet
2016-09-20 13:36 ` David Laight
2016-09-20 13:39 ` Jiri Pirko
@ 2016-09-21 1:06 ` Eric Dumazet
2016-09-22 6:42 ` David Miller
2 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2016-09-21 1:06 UTC (permalink / raw)
To: Jiri Pirko, David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
Jiri Pirko reported an UBSAN warning happening in ip_idents_reserve()
[] UBSAN: Undefined behaviour in ./arch/x86/include/asm/atomic.h:156:11
[] signed integer overflow:
[] -2117905507 + -695755206 cannot be represented in type 'int'
Since we do not have uatomic_add_return() yet, use atomic_cmpxchg()
so that the arithmetics can be done using unsigned int.
Fixes: 04ca6973f7c1 ("ip: make IP identifiers less predictable")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Jiri Pirko <jiri@resnulli.us>
---
David, Jiri, I removed the prandom_u32() stuff in favor of a traditional
loop to meet stable requirements. Thanks !
net/ipv4/route.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index b52496fd51075821c39435f50ac62f813967aecc..654a9af201366887652a4e19a6f1261e5e747056 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -476,12 +476,18 @@ 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;
+ /* Do not use atomic_add_return() as it makes UBSAN unhappy */
+ do {
+ old = (u32)atomic_read(p_id);
+ new = old + delta + segs;
+ } while (atomic_cmpxchg(p_id, old, new) != old);
+
+ return new - segs;
}
EXPORT_SYMBOL(ip_idents_reserve);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net] net: get rid of an signed integer overflow in ip_idents_reserve()
2016-09-21 1:06 ` [PATCH net] net: get rid of an signed integer overflow in ip_idents_reserve() Eric Dumazet
@ 2016-09-22 6:42 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2016-09-22 6:42 UTC (permalink / raw)
To: eric.dumazet; +Cc: jiri, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 20 Sep 2016 18:06:17 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> Jiri Pirko reported an UBSAN warning happening in ip_idents_reserve()
>
> [] UBSAN: Undefined behaviour in ./arch/x86/include/asm/atomic.h:156:11
> [] signed integer overflow:
> [] -2117905507 + -695755206 cannot be represented in type 'int'
>
> Since we do not have uatomic_add_return() yet, use atomic_cmpxchg()
> so that the arithmetics can be done using unsigned int.
>
> Fixes: 04ca6973f7c1 ("ip: make IP identifiers less predictable")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Jiri Pirko <jiri@resnulli.us>
> ---
> David, Jiri, I removed the prandom_u32() stuff in favor of a traditional
> loop to meet stable requirements. Thanks !
Applied.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-09-22 6:43 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-20 12:00 UBSAN reports issue in ip_idents_reserve Jiri Pirko
2016-09-20 13:28 ` Eric Dumazet
2016-09-20 13:36 ` David Laight
2016-09-20 13:45 ` Eric Dumazet
2016-09-20 13:39 ` Jiri Pirko
2016-09-20 14:11 ` Eric Dumazet
2016-09-20 14:18 ` Eric Dumazet
2016-09-20 14:24 ` Eric Dumazet
2016-09-20 17:46 ` Jiri Pirko
2016-09-20 18:42 ` Eric Dumazet
2016-09-20 15:25 ` Eric Dumazet
2016-09-20 16:25 ` Jiri Pirko
2016-09-21 1:06 ` [PATCH net] net: get rid of an signed integer overflow in ip_idents_reserve() Eric Dumazet
2016-09-22 6:42 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).