| From bbab7ef235031f6733b5429ae7877bfa22339712 Mon Sep 17 00:00:00 2001 |
| From: Eric Dumazet <edumazet@google.com> |
| Date: Fri, 8 Nov 2019 10:34:47 -0800 |
| Subject: net: icmp: fix data-race in cmp_global_allow() |
| |
| From: Eric Dumazet <edumazet@google.com> |
| |
| commit bbab7ef235031f6733b5429ae7877bfa22339712 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: 4cdf507d5452 ("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> |
| |
| --- |
| net/ipv4/icmp.c | 11 ++++++----- |
| 1 file changed, 6 insertions(+), 5 deletions(-) |
| |
| --- a/net/ipv4/icmp.c |
| +++ b/net/ipv4/icmp.c |
| @@ -255,10 +255,11 @@ bool icmp_global_allow(void) |
| bool rc = false; |
| |
| /* 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) { |
| - delta = min_t(u32, now - icmp_global.stamp, HZ); |
| + if (!READ_ONCE(icmp_global.credit)) { |
| + delta = min_t(u32, now - READ_ONCE(icmp_global.stamp), HZ); |
| if (delta < HZ / 50) |
| return false; |
| } |
| @@ -268,14 +269,14 @@ bool icmp_global_allow(void) |
| if (delta >= HZ / 50) { |
| incr = sysctl_icmp_msgs_per_sec * delta / HZ ; |
| if (incr) |
| - icmp_global.stamp = now; |
| + WRITE_ONCE(icmp_global.stamp, now); |
| } |
| credit = min_t(u32, icmp_global.credit + incr, sysctl_icmp_msgs_burst); |
| if (credit) { |
| credit--; |
| rc = true; |
| } |
| - icmp_global.credit = credit; |
| + WRITE_ONCE(icmp_global.credit, credit); |
| spin_unlock(&icmp_global.lock); |
| return rc; |
| } |