* [PATCH net] udp: fix data-race in udp_set_dev_scratch()
@ 2019-10-24 18:43 Eric Dumazet
2019-10-25 8:00 ` Paolo Abeni
2019-10-28 20:54 ` David Miller
0 siblings, 2 replies; 3+ messages in thread
From: Eric Dumazet @ 2019-10-24 18:43 UTC (permalink / raw
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, syzbot, Paolo Abeni
KCSAN reported a data-race in udp_set_dev_scratch() [1]
The issue here is that we must not write over skb fields
if skb is shared. A similar issue has been fixed in commit
89c22d8c3b27 ("net: Fix skb csum races when peeking")
While we are at it, use a helper only dealing with
udp_skb_scratch(skb)->csum_unnecessary, as this allows
udp_set_dev_scratch() to be called once and thus inlined.
[1]
BUG: KCSAN: data-race in udp_set_dev_scratch / udpv6_recvmsg
write to 0xffff888120278317 of 1 bytes by task 10411 on cpu 1:
udp_set_dev_scratch+0xea/0x200 net/ipv4/udp.c:1308
__first_packet_length+0x147/0x420 net/ipv4/udp.c:1556
first_packet_length+0x68/0x2a0 net/ipv4/udp.c:1579
udp_poll+0xea/0x110 net/ipv4/udp.c:2720
sock_poll+0xed/0x250 net/socket.c:1256
vfs_poll include/linux/poll.h:90 [inline]
do_select+0x7d0/0x1020 fs/select.c:534
core_sys_select+0x381/0x550 fs/select.c:677
do_pselect.constprop.0+0x11d/0x160 fs/select.c:759
__do_sys_pselect6 fs/select.c:784 [inline]
__se_sys_pselect6 fs/select.c:769 [inline]
__x64_sys_pselect6+0x12e/0x170 fs/select.c:769
do_syscall_64+0xcc/0x370 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x44/0xa9
read to 0xffff888120278317 of 1 bytes by task 10413 on cpu 0:
udp_skb_csum_unnecessary include/net/udp.h:358 [inline]
udpv6_recvmsg+0x43e/0xe90 net/ipv6/udp.c:310
inet6_recvmsg+0xbb/0x240 net/ipv6/af_inet6.c:592
sock_recvmsg_nosec+0x5c/0x70 net/socket.c:871
___sys_recvmsg+0x1a0/0x3e0 net/socket.c:2480
do_recvmmsg+0x19a/0x5c0 net/socket.c:2601
__sys_recvmmsg+0x1ef/0x200 net/socket.c:2680
__do_sys_recvmmsg net/socket.c:2703 [inline]
__se_sys_recvmmsg net/socket.c:2696 [inline]
__x64_sys_recvmmsg+0x89/0xb0 net/socket.c:2696
do_syscall_64+0xcc/0x370 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10413 Comm: syz-executor.0 Not tainted 5.4.0-rc3+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Fixes: 2276f58ac589 ("udp: use a separate rx queue for packet reception")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Paolo Abeni <pabeni@redhat.com>
---
net/ipv4/udp.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 345a3d43f5a655e009e99c16bb19e047cdf003c6..d1ed160af202c054839387201abd3f13b55d00e9 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1316,6 +1316,20 @@ static void udp_set_dev_scratch(struct sk_buff *skb)
scratch->_tsize_state |= UDP_SKB_IS_STATELESS;
}
+static void udp_skb_csum_unnecessary_set(struct sk_buff *skb)
+{
+ /* We come here after udp_lib_checksum_complete() returned 0.
+ * This means that __skb_checksum_complete() might have
+ * set skb->csum_valid to 1.
+ * On 64bit platforms, we can set csum_unnecessary
+ * to true, but only if the skb is not shared.
+ */
+#if BITS_PER_LONG == 64
+ if (!skb_shared(skb))
+ udp_skb_scratch(skb)->csum_unnecessary = true;
+#endif
+}
+
static int udp_skb_truesize(struct sk_buff *skb)
{
return udp_skb_scratch(skb)->_tsize_state & ~UDP_SKB_IS_STATELESS;
@@ -1550,10 +1564,7 @@ static struct sk_buff *__first_packet_length(struct sock *sk,
*total += skb->truesize;
kfree_skb(skb);
} else {
- /* the csum related bits could be changed, refresh
- * the scratch area
- */
- udp_set_dev_scratch(skb);
+ udp_skb_csum_unnecessary_set(skb);
break;
}
}
--
2.23.0.866.gb869b98d4c-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] udp: fix data-race in udp_set_dev_scratch()
2019-10-24 18:43 [PATCH net] udp: fix data-race in udp_set_dev_scratch() Eric Dumazet
@ 2019-10-25 8:00 ` Paolo Abeni
2019-10-28 20:54 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: Paolo Abeni @ 2019-10-25 8:00 UTC (permalink / raw
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet, syzbot
On Thu, 2019-10-24 at 11:43 -0700, Eric Dumazet wrote:
> KCSAN reported a data-race in udp_set_dev_scratch() [1]
>
> The issue here is that we must not write over skb fields
> if skb is shared. A similar issue has been fixed in commit
> 89c22d8c3b27 ("net: Fix skb csum races when peeking")
>
> While we are at it, use a helper only dealing with
> udp_skb_scratch(skb)->csum_unnecessary, as this allows
> udp_set_dev_scratch() to be called once and thus inlined.
>
> [1]
> BUG: KCSAN: data-race in udp_set_dev_scratch / udpv6_recvmsg
>
> write to 0xffff888120278317 of 1 bytes by task 10411 on cpu 1:
> udp_set_dev_scratch+0xea/0x200 net/ipv4/udp.c:1308
> __first_packet_length+0x147/0x420 net/ipv4/udp.c:1556
> first_packet_length+0x68/0x2a0 net/ipv4/udp.c:1579
> udp_poll+0xea/0x110 net/ipv4/udp.c:2720
> sock_poll+0xed/0x250 net/socket.c:1256
> vfs_poll include/linux/poll.h:90 [inline]
> do_select+0x7d0/0x1020 fs/select.c:534
> core_sys_select+0x381/0x550 fs/select.c:677
> do_pselect.constprop.0+0x11d/0x160 fs/select.c:759
> __do_sys_pselect6 fs/select.c:784 [inline]
> __se_sys_pselect6 fs/select.c:769 [inline]
> __x64_sys_pselect6+0x12e/0x170 fs/select.c:769
> do_syscall_64+0xcc/0x370 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> read to 0xffff888120278317 of 1 bytes by task 10413 on cpu 0:
> udp_skb_csum_unnecessary include/net/udp.h:358 [inline]
> udpv6_recvmsg+0x43e/0xe90 net/ipv6/udp.c:310
> inet6_recvmsg+0xbb/0x240 net/ipv6/af_inet6.c:592
> sock_recvmsg_nosec+0x5c/0x70 net/socket.c:871
> ___sys_recvmsg+0x1a0/0x3e0 net/socket.c:2480
> do_recvmmsg+0x19a/0x5c0 net/socket.c:2601
> __sys_recvmmsg+0x1ef/0x200 net/socket.c:2680
> __do_sys_recvmmsg net/socket.c:2703 [inline]
> __se_sys_recvmmsg net/socket.c:2696 [inline]
> __x64_sys_recvmmsg+0x89/0xb0 net/socket.c:2696
> do_syscall_64+0xcc/0x370 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 0 PID: 10413 Comm: syz-executor.0 Not tainted 5.4.0-rc3+ #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>
> Fixes: 2276f58ac589 ("udp: use a separate rx queue for packet reception")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> ---
> net/ipv4/udp.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 345a3d43f5a655e009e99c16bb19e047cdf003c6..d1ed160af202c054839387201abd3f13b55d00e9 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1316,6 +1316,20 @@ static void udp_set_dev_scratch(struct sk_buff *skb)
> scratch->_tsize_state |= UDP_SKB_IS_STATELESS;
> }
>
> +static void udp_skb_csum_unnecessary_set(struct sk_buff *skb)
> +{
> + /* We come here after udp_lib_checksum_complete() returned 0.
> + * This means that __skb_checksum_complete() might have
> + * set skb->csum_valid to 1.
> + * On 64bit platforms, we can set csum_unnecessary
> + * to true, but only if the skb is not shared.
> + */
> +#if BITS_PER_LONG == 64
> + if (!skb_shared(skb))
> + udp_skb_scratch(skb)->csum_unnecessary = true;
> +#endif
> +}
> +
> static int udp_skb_truesize(struct sk_buff *skb)
> {
> return udp_skb_scratch(skb)->_tsize_state & ~UDP_SKB_IS_STATELESS;
> @@ -1550,10 +1564,7 @@ static struct sk_buff *__first_packet_length(struct sock *sk,
> *total += skb->truesize;
> kfree_skb(skb);
> } else {
> - /* the csum related bits could be changed, refresh
> - * the scratch area
> - */
> - udp_set_dev_scratch(skb);
> + udp_skb_csum_unnecessary_set(skb);
> break;
> }
> }
LGTM, Thanks Eric!
Reviewed-by: Paolo Abeni <pabeni@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] udp: fix data-race in udp_set_dev_scratch()
2019-10-24 18:43 [PATCH net] udp: fix data-race in udp_set_dev_scratch() Eric Dumazet
2019-10-25 8:00 ` Paolo Abeni
@ 2019-10-28 20:54 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2019-10-28 20:54 UTC (permalink / raw
To: edumazet; +Cc: netdev, eric.dumazet, syzkaller, pabeni
From: Eric Dumazet <edumazet@google.com>
Date: Thu, 24 Oct 2019 11:43:31 -0700
> KCSAN reported a data-race in udp_set_dev_scratch() [1]
>
> The issue here is that we must not write over skb fields
> if skb is shared. A similar issue has been fixed in commit
> 89c22d8c3b27 ("net: Fix skb csum races when peeking")
>
> While we are at it, use a helper only dealing with
> udp_skb_scratch(skb)->csum_unnecessary, as this allows
> udp_set_dev_scratch() to be called once and thus inlined.
>
> [1]
...
> Fixes: 2276f58ac589 ("udp: use a separate rx queue for packet reception")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-28 20:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-24 18:43 [PATCH net] udp: fix data-race in udp_set_dev_scratch() Eric Dumazet
2019-10-25 8:00 ` Paolo Abeni
2019-10-28 20:54 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.