net: icmp: fix data-race in cmp_global_allow()
commitbbab7ef235
upstream. This code reads two global variables without protection of a lock. We need READ_ONCE()/WRITE_ONCE() pairs to avoid load/store-tearing and better document the intent. KCSAN reported : BUG: KCSAN: data-race in icmp_global_allow / icmp_global_allow read to 0xffffffff861a8014 of 4 bytes by task 11201 on cpu 0: icmp_global_allow+0x36/0x1b0 net/ipv4/icmp.c:254 icmpv6_global_allow net/ipv6/icmp.c:184 [inline] icmpv6_global_allow net/ipv6/icmp.c:179 [inline] icmp6_send+0x493/0x1140 net/ipv6/icmp.c:514 icmpv6_send+0x71/0xb0 net/ipv6/ip6_icmp.c:43 ip6_link_failure+0x43/0x180 net/ipv6/route.c:2640 dst_link_failure include/net/dst.h:419 [inline] vti_xmit net/ipv4/ip_vti.c:243 [inline] vti_tunnel_xmit+0x27f/0xa50 net/ipv4/ip_vti.c:279 __netdev_start_xmit include/linux/netdevice.h:4420 [inline] netdev_start_xmit include/linux/netdevice.h:4434 [inline] xmit_one net/core/dev.c:3280 [inline] dev_hard_start_xmit+0xef/0x430 net/core/dev.c:3296 __dev_queue_xmit+0x14c9/0x1b60 net/core/dev.c:3873 dev_queue_xmit+0x21/0x30 net/core/dev.c:3906 neigh_direct_output+0x1f/0x30 net/core/neighbour.c:1530 neigh_output include/net/neighbour.h:511 [inline] ip6_finish_output2+0x7a6/0xec0 net/ipv6/ip6_output.c:116 __ip6_finish_output net/ipv6/ip6_output.c:142 [inline] __ip6_finish_output+0x2d7/0x330 net/ipv6/ip6_output.c:127 ip6_finish_output+0x41/0x160 net/ipv6/ip6_output.c:152 NF_HOOK_COND include/linux/netfilter.h:294 [inline] ip6_output+0xf2/0x280 net/ipv6/ip6_output.c:175 dst_output include/net/dst.h:436 [inline] ip6_local_out+0x74/0x90 net/ipv6/output_core.c:179 write to 0xffffffff861a8014 of 4 bytes by task 11183 on cpu 1: icmp_global_allow+0x174/0x1b0 net/ipv4/icmp.c:272 icmpv6_global_allow net/ipv6/icmp.c:184 [inline] icmpv6_global_allow net/ipv6/icmp.c:179 [inline] icmp6_send+0x493/0x1140 net/ipv6/icmp.c:514 icmpv6_send+0x71/0xb0 net/ipv6/ip6_icmp.c:43 ip6_link_failure+0x43/0x180 net/ipv6/route.c:2640 dst_link_failure include/net/dst.h:419 [inline] vti_xmit net/ipv4/ip_vti.c:243 [inline] vti_tunnel_xmit+0x27f/0xa50 net/ipv4/ip_vti.c:279 __netdev_start_xmit include/linux/netdevice.h:4420 [inline] netdev_start_xmit include/linux/netdevice.h:4434 [inline] xmit_one net/core/dev.c:3280 [inline] dev_hard_start_xmit+0xef/0x430 net/core/dev.c:3296 __dev_queue_xmit+0x14c9/0x1b60 net/core/dev.c:3873 dev_queue_xmit+0x21/0x30 net/core/dev.c:3906 neigh_direct_output+0x1f/0x30 net/core/neighbour.c:1530 neigh_output include/net/neighbour.h:511 [inline] ip6_finish_output2+0x7a6/0xec0 net/ipv6/ip6_output.c:116 __ip6_finish_output net/ipv6/ip6_output.c:142 [inline] __ip6_finish_output+0x2d7/0x330 net/ipv6/ip6_output.c:127 ip6_finish_output+0x41/0x160 net/ipv6/ip6_output.c:152 NF_HOOK_COND include/linux/netfilter.h:294 [inline] ip6_output+0xf2/0x280 net/ipv6/ip6_output.c:175 Reported by Kernel Concurrency Sanitizer on: CPU: 1 PID: 11183 Comm: syz-executor.2 Not tainted 5.4.0-rc3+ #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Fixes:4cdf507d54
("icmp: add a global rate limitation") Signed-off-by: Eric Dumazet <edumazet@google.com> Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
This commit is contained in:
parent
cf0bcc9958
commit
3fe9be220c
|
@ -249,10 +249,11 @@ bool icmp_global_allow(void)
|
||||||
bool rc = false;
|
bool rc = false;
|
||||||
|
|
||||||
/* Check if token bucket is empty and cannot be refilled
|
/* Check if token bucket is empty and cannot be refilled
|
||||||
* without taking the spinlock.
|
* without taking the spinlock. The READ_ONCE() are paired
|
||||||
|
* with the following WRITE_ONCE() in this same function.
|
||||||
*/
|
*/
|
||||||
if (!icmp_global.credit) {
|
if (!READ_ONCE(icmp_global.credit)) {
|
||||||
delta = min_t(u32, now - icmp_global.stamp, HZ);
|
delta = min_t(u32, now - READ_ONCE(icmp_global.stamp), HZ);
|
||||||
if (delta < HZ / 50)
|
if (delta < HZ / 50)
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
@ -262,14 +263,14 @@ bool icmp_global_allow(void)
|
||||||
if (delta >= HZ / 50) {
|
if (delta >= HZ / 50) {
|
||||||
incr = sysctl_icmp_msgs_per_sec * delta / HZ ;
|
incr = sysctl_icmp_msgs_per_sec * delta / HZ ;
|
||||||
if (incr)
|
if (incr)
|
||||||
icmp_global.stamp = now;
|
WRITE_ONCE(icmp_global.stamp, now);
|
||||||
}
|
}
|
||||||
credit = min_t(u32, icmp_global.credit + incr, sysctl_icmp_msgs_burst);
|
credit = min_t(u32, icmp_global.credit + incr, sysctl_icmp_msgs_burst);
|
||||||
if (credit) {
|
if (credit) {
|
||||||
credit--;
|
credit--;
|
||||||
rc = true;
|
rc = true;
|
||||||
}
|
}
|
||||||
icmp_global.credit = credit;
|
WRITE_ONCE(icmp_global.credit, credit);
|
||||||
spin_unlock(&icmp_global.lock);
|
spin_unlock(&icmp_global.lock);
|
||||||
return rc;
|
return rc;
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue