* [PATCH 5.15 1/3] ipv6: annotate some data-races around sk->sk_prot
2023-04-17 16:53 [PATCH 5.15 0/3] ipv6/v4: Fix data races around sk->sk_prot and icsk->icsk_af_ops Kazunori Kobayashi
@ 2023-04-17 16:53 ` Kazunori Kobayashi
2024-07-02 9:42 ` Greg KH
2023-04-17 16:53 ` [PATCH 5.15 2/3] ipv6: Fix data races " Kazunori Kobayashi
2023-04-17 16:53 ` [PATCH 5.15 3/3] tcp: Fix data races around icsk->icsk_af_ops Kazunori Kobayashi
2 siblings, 1 reply; 6+ messages in thread
From: Kazunori Kobayashi @ 2023-04-17 16:53 UTC (permalink / raw)
To: netdev
Cc: stable, linux-kernel, hiraku.toyooka, Eric Dumazet,
David S . Miller, Kazunori Kobayashi
From: Eric Dumazet <edumazet@google.com>
commit 086d49058cd8471046ae9927524708820f5fd1c7 upstream.
IPv6 has this hack changing sk->sk_prot when an IPv6 socket
is 'converted' to an IPv4 one with IPV6_ADDRFORM option.
This operation is only performed for TCP and UDP, knowing
their 'struct proto' for the two network families are populated
in the same way, and can not disappear while a reader
might use and dereference sk->sk_prot.
If we think about it all reads of sk->sk_prot while
either socket lock or RTNL is not acquired should be using READ_ONCE().
Also note that other layers like MPTCP, XFRM, CHELSIO_TLS also
write over sk->sk_prot.
BUG: KCSAN: data-race in inet6_recvmsg / ipv6_setsockopt
write to 0xffff8881386f7aa8 of 8 bytes by task 26932 on cpu 0:
do_ipv6_setsockopt net/ipv6/ipv6_sockglue.c:492 [inline]
ipv6_setsockopt+0x3758/0x3910 net/ipv6/ipv6_sockglue.c:1019
udpv6_setsockopt+0x85/0x90 net/ipv6/udp.c:1649
sock_common_setsockopt+0x5d/0x70 net/core/sock.c:3489
__sys_setsockopt+0x209/0x2a0 net/socket.c:2180
__do_sys_setsockopt net/socket.c:2191 [inline]
__se_sys_setsockopt net/socket.c:2188 [inline]
__x64_sys_setsockopt+0x62/0x70 net/socket.c:2188
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
read to 0xffff8881386f7aa8 of 8 bytes by task 26911 on cpu 1:
inet6_recvmsg+0x7a/0x210 net/ipv6/af_inet6.c:659
____sys_recvmsg+0x16c/0x320
___sys_recvmsg net/socket.c:2674 [inline]
do_recvmmsg+0x3f5/0xae0 net/socket.c:2768
__sys_recvmmsg net/socket.c:2847 [inline]
__do_sys_recvmmsg net/socket.c:2870 [inline]
__se_sys_recvmmsg net/socket.c:2863 [inline]
__x64_sys_recvmmsg+0xde/0x160 net/socket.c:2863
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
value changed: 0xffffffff85e0e980 -> 0xffffffff85e01580
Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 26911 Comm: syz-executor.3 Not tainted 5.17.0-rc2-syzkaller-00316-g0457e5153e0e-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Kazunori Kobayashi <kazunori.kobayashi@miraclelinux.com>
---
net/ipv6/af_inet6.c | 24 ++++++++++++++++++------
net/ipv6/ipv6_sockglue.c | 6 ++++--
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 8e0c33b683010..bbfa5d3a16f01 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -452,11 +452,14 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
{
struct sock *sk = sock->sk;
u32 flags = BIND_WITH_LOCK;
+ const struct proto *prot;
int err = 0;
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ prot = READ_ONCE(sk->sk_prot);
/* If the socket has its own bind function then use it. */
- if (sk->sk_prot->bind)
- return sk->sk_prot->bind(sk, uaddr, addr_len);
+ if (prot->bind)
+ return prot->bind(sk, uaddr, addr_len);
if (addr_len < SIN6_LEN_RFC2133)
return -EINVAL;
@@ -572,6 +575,7 @@ int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
void __user *argp = (void __user *)arg;
struct sock *sk = sock->sk;
struct net *net = sock_net(sk);
+ const struct proto *prot;
switch (cmd) {
case SIOCADDRT:
@@ -589,9 +593,11 @@ int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
case SIOCSIFDSTADDR:
return addrconf_set_dstaddr(net, argp);
default:
- if (!sk->sk_prot->ioctl)
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ prot = READ_ONCE(sk->sk_prot);
+ if (!prot->ioctl)
return -ENOIOCTLCMD;
- return sk->sk_prot->ioctl(sk, cmd, arg);
+ return prot->ioctl(sk, cmd, arg);
}
/*NOTREACHED*/
return 0;
@@ -653,11 +659,14 @@ INDIRECT_CALLABLE_DECLARE(int udpv6_sendmsg(struct sock *, struct msghdr *,
int inet6_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
{
struct sock *sk = sock->sk;
+ const struct proto *prot;
if (unlikely(inet_send_prepare(sk)))
return -EAGAIN;
- return INDIRECT_CALL_2(sk->sk_prot->sendmsg, tcp_sendmsg, udpv6_sendmsg,
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ prot = READ_ONCE(sk->sk_prot);
+ return INDIRECT_CALL_2(prot->sendmsg, tcp_sendmsg, udpv6_sendmsg,
sk, msg, size);
}
@@ -667,13 +676,16 @@ int inet6_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
int flags)
{
struct sock *sk = sock->sk;
+ const struct proto *prot;
int addr_len = 0;
int err;
if (likely(!(flags & MSG_ERRQUEUE)))
sock_rps_record_flow(sk);
- err = INDIRECT_CALL_2(sk->sk_prot->recvmsg, tcp_recvmsg, udpv6_recvmsg,
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ prot = READ_ONCE(sk->sk_prot);
+ err = INDIRECT_CALL_2(prot->recvmsg, tcp_recvmsg, udpv6_recvmsg,
sk, msg, size, flags & MSG_DONTWAIT,
flags & ~MSG_DONTWAIT, &addr_len);
if (err >= 0)
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 197e12d5607f1..485d381aa4475 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -475,7 +475,8 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
sock_prot_inuse_add(net, sk->sk_prot, -1);
sock_prot_inuse_add(net, &tcp_prot, 1);
local_bh_enable();
- sk->sk_prot = &tcp_prot;
+ /* Paired with READ_ONCE(sk->sk_prot) in net/ipv6/af_inet6.c */
+ WRITE_ONCE(sk->sk_prot, &tcp_prot);
icsk->icsk_af_ops = &ipv4_specific;
sk->sk_socket->ops = &inet_stream_ops;
sk->sk_family = PF_INET;
@@ -489,7 +490,8 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
sock_prot_inuse_add(net, sk->sk_prot, -1);
sock_prot_inuse_add(net, prot, 1);
local_bh_enable();
- sk->sk_prot = prot;
+ /* Paired with READ_ONCE(sk->sk_prot) in net/ipv6/af_inet6.c */
+ WRITE_ONCE(sk->sk_prot, prot);
sk->sk_socket->ops = &inet_dgram_ops;
sk->sk_family = PF_INET;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 5.15 1/3] ipv6: annotate some data-races around sk->sk_prot
2023-04-17 16:53 ` [PATCH 5.15 1/3] ipv6: annotate some data-races around sk->sk_prot Kazunori Kobayashi
@ 2024-07-02 9:42 ` Greg KH
2024-07-03 6:45 ` Kazunori Kobayashi
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2024-07-02 9:42 UTC (permalink / raw)
To: Kazunori Kobayashi
Cc: netdev, stable, linux-kernel, hiraku.toyooka, Eric Dumazet,
David S . Miller
On Mon, Apr 17, 2023 at 04:53:46PM +0000, Kazunori Kobayashi wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> commit 086d49058cd8471046ae9927524708820f5fd1c7 upstream.
>
> IPv6 has this hack changing sk->sk_prot when an IPv6 socket
> is 'converted' to an IPv4 one with IPV6_ADDRFORM option.
>
> This operation is only performed for TCP and UDP, knowing
> their 'struct proto' for the two network families are populated
> in the same way, and can not disappear while a reader
> might use and dereference sk->sk_prot.
>
> If we think about it all reads of sk->sk_prot while
> either socket lock or RTNL is not acquired should be using READ_ONCE().
>
> Also note that other layers like MPTCP, XFRM, CHELSIO_TLS also
> write over sk->sk_prot.
>
> BUG: KCSAN: data-race in inet6_recvmsg / ipv6_setsockopt
>
> write to 0xffff8881386f7aa8 of 8 bytes by task 26932 on cpu 0:
> do_ipv6_setsockopt net/ipv6/ipv6_sockglue.c:492 [inline]
> ipv6_setsockopt+0x3758/0x3910 net/ipv6/ipv6_sockglue.c:1019
> udpv6_setsockopt+0x85/0x90 net/ipv6/udp.c:1649
> sock_common_setsockopt+0x5d/0x70 net/core/sock.c:3489
> __sys_setsockopt+0x209/0x2a0 net/socket.c:2180
> __do_sys_setsockopt net/socket.c:2191 [inline]
> __se_sys_setsockopt net/socket.c:2188 [inline]
> __x64_sys_setsockopt+0x62/0x70 net/socket.c:2188
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> read to 0xffff8881386f7aa8 of 8 bytes by task 26911 on cpu 1:
> inet6_recvmsg+0x7a/0x210 net/ipv6/af_inet6.c:659
> ____sys_recvmsg+0x16c/0x320
> ___sys_recvmsg net/socket.c:2674 [inline]
> do_recvmmsg+0x3f5/0xae0 net/socket.c:2768
> __sys_recvmmsg net/socket.c:2847 [inline]
> __do_sys_recvmmsg net/socket.c:2870 [inline]
> __se_sys_recvmmsg net/socket.c:2863 [inline]
> __x64_sys_recvmmsg+0xde/0x160 net/socket.c:2863
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> value changed: 0xffffffff85e0e980 -> 0xffffffff85e01580
>
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 1 PID: 26911 Comm: syz-executor.3 Not tainted 5.17.0-rc2-syzkaller-00316-g0457e5153e0e-dirty #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Kazunori Kobayashi <kazunori.kobayashi@miraclelinux.com>
This backport didn't apply at all, are you sure you made it against the
proper tree?
The original commit does seem to apply properly, so I'll go apply that
one instead...
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 5.15 1/3] ipv6: annotate some data-races around sk->sk_prot
2024-07-02 9:42 ` Greg KH
@ 2024-07-03 6:45 ` Kazunori Kobayashi
0 siblings, 0 replies; 6+ messages in thread
From: Kazunori Kobayashi @ 2024-07-03 6:45 UTC (permalink / raw)
To: Greg KH
Cc: netdev, stable, linux-kernel, hiraku.toyooka, Eric Dumazet,
David S . Miller
On 2024/07/02 18:42, Greg KH wrote:
> On Mon, Apr 17, 2023 at 04:53:46PM +0000, Kazunori Kobayashi wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> commit 086d49058cd8471046ae9927524708820f5fd1c7 upstream.
>>
>> IPv6 has this hack changing sk->sk_prot when an IPv6 socket
>> is 'converted' to an IPv4 one with IPV6_ADDRFORM option.
>>
>> This operation is only performed for TCP and UDP, knowing
>> their 'struct proto' for the two network families are populated
>> in the same way, and can not disappear while a reader
>> might use and dereference sk->sk_prot.
>>
>> If we think about it all reads of sk->sk_prot while
>> either socket lock or RTNL is not acquired should be using READ_ONCE().
>>
>> Also note that other layers like MPTCP, XFRM, CHELSIO_TLS also
>> write over sk->sk_prot.
>>
>> BUG: KCSAN: data-race in inet6_recvmsg / ipv6_setsockopt
>>
>> write to 0xffff8881386f7aa8 of 8 bytes by task 26932 on cpu 0:
>> do_ipv6_setsockopt net/ipv6/ipv6_sockglue.c:492 [inline]
>> ipv6_setsockopt+0x3758/0x3910 net/ipv6/ipv6_sockglue.c:1019
>> udpv6_setsockopt+0x85/0x90 net/ipv6/udp.c:1649
>> sock_common_setsockopt+0x5d/0x70 net/core/sock.c:3489
>> __sys_setsockopt+0x209/0x2a0 net/socket.c:2180
>> __do_sys_setsockopt net/socket.c:2191 [inline]
>> __se_sys_setsockopt net/socket.c:2188 [inline]
>> __x64_sys_setsockopt+0x62/0x70 net/socket.c:2188
>> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> read to 0xffff8881386f7aa8 of 8 bytes by task 26911 on cpu 1:
>> inet6_recvmsg+0x7a/0x210 net/ipv6/af_inet6.c:659
>> ____sys_recvmsg+0x16c/0x320
>> ___sys_recvmsg net/socket.c:2674 [inline]
>> do_recvmmsg+0x3f5/0xae0 net/socket.c:2768
>> __sys_recvmmsg net/socket.c:2847 [inline]
>> __do_sys_recvmmsg net/socket.c:2870 [inline]
>> __se_sys_recvmmsg net/socket.c:2863 [inline]
>> __x64_sys_recvmmsg+0xde/0x160 net/socket.c:2863
>> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> value changed: 0xffffffff85e0e980 -> 0xffffffff85e01580
>>
>> Reported by Kernel Concurrency Sanitizer on:
>> CPU: 1 PID: 26911 Comm: syz-executor.3 Not tainted 5.17.0-rc2-syzkaller-00316-g0457e5153e0e-dirty #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>>
>> Reported-by: syzbot <syzkaller@googlegroups.com>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Kazunori Kobayashi <kazunori.kobayashi@miraclelinux.com>
> This backport didn't apply at all, are you sure you made it against the
> proper tree?
>
> The original commit does seem to apply properly, so I'll go apply that
> one instead...
>
> greg k-h
I assumed the following commit is the latest version for 5.15 stable and
based the patch on this.
Is there any difference from your expectation?
commit 4878aadf2d1519f3731ae300ce1fef78fc63ee30 (tag: v5.15.161,
origin/linux-5.15.y, li
nux-5.15.y)
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Sun Jun 16 13:40:01 2024 +0200
Linux 5.15.161
Regards,
Kazunori
--
Kazunori Kobayashi
Cybertrust Japan Co., Ltd.
https://www.cybertrust.co.jp/
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 5.15 2/3] ipv6: Fix data races around sk->sk_prot.
2023-04-17 16:53 [PATCH 5.15 0/3] ipv6/v4: Fix data races around sk->sk_prot and icsk->icsk_af_ops Kazunori Kobayashi
2023-04-17 16:53 ` [PATCH 5.15 1/3] ipv6: annotate some data-races around sk->sk_prot Kazunori Kobayashi
@ 2023-04-17 16:53 ` Kazunori Kobayashi
2023-04-17 16:53 ` [PATCH 5.15 3/3] tcp: Fix data races around icsk->icsk_af_ops Kazunori Kobayashi
2 siblings, 0 replies; 6+ messages in thread
From: Kazunori Kobayashi @ 2023-04-17 16:53 UTC (permalink / raw)
To: netdev
Cc: stable, linux-kernel, hiraku.toyooka, Kuniyuki Iwashima,
Jakub Kicinski, Kazunori Kobayashi
From: Kuniyuki Iwashima <kuniyu@amazon.com>
commit 364f997b5cfe1db0d63a390fe7c801fa2b3115f6 upstream.
Commit 086d49058cd8 ("ipv6: annotate some data-races around sk->sk_prot")
fixed some data-races around sk->sk_prot but it was not enough.
Some functions in inet6_(stream|dgram)_ops still access sk->sk_prot
without lock_sock() or rtnl_lock(), so they need READ_ONCE() to avoid
load tearing.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Kazunori Kobayashi <kazunori.kobayashi@miraclelinux.com>
---
net/core/sock.c | 6 ++++--
net/ipv4/af_inet.c | 23 ++++++++++++++++-------
net/ipv6/ipv6_sockglue.c | 4 ++--
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c
index 62e376f09f957..399d0b801b097 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3413,7 +3413,8 @@ int sock_common_getsockopt(struct socket *sock, int level, int optname,
{
struct sock *sk = sock->sk;
- return sk->sk_prot->getsockopt(sk, level, optname, optval, optlen);
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ return READ_ONCE(sk->sk_prot)->getsockopt(sk, level, optname, optval, optlen);
}
EXPORT_SYMBOL(sock_common_getsockopt);
@@ -3440,7 +3441,8 @@ int sock_common_setsockopt(struct socket *sock, int level, int optname,
{
struct sock *sk = sock->sk;
- return sk->sk_prot->setsockopt(sk, level, optname, optval, optlen);
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ return READ_ONCE(sk->sk_prot)->setsockopt(sk, level, optname, optval, optlen);
}
EXPORT_SYMBOL(sock_common_setsockopt);
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 487f75993bf4f..110ac22d3ce20 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -565,22 +565,27 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr,
int addr_len, int flags)
{
struct sock *sk = sock->sk;
+ const struct proto *prot;
int err;
if (addr_len < sizeof(uaddr->sa_family))
return -EINVAL;
+
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ prot = READ_ONCE(sk->sk_prot);
+
if (uaddr->sa_family == AF_UNSPEC)
- return sk->sk_prot->disconnect(sk, flags);
+ return prot->disconnect(sk, flags);
if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) {
- err = sk->sk_prot->pre_connect(sk, uaddr, addr_len);
+ err = prot->pre_connect(sk, uaddr, addr_len);
if (err)
return err;
}
if (data_race(!inet_sk(sk)->inet_num) && inet_autobind(sk))
return -EAGAIN;
- return sk->sk_prot->connect(sk, uaddr, addr_len);
+ return prot->connect(sk, uaddr, addr_len);
}
EXPORT_SYMBOL(inet_dgram_connect);
@@ -743,10 +748,11 @@ EXPORT_SYMBOL(inet_stream_connect);
int inet_accept(struct socket *sock, struct socket *newsock, int flags,
bool kern)
{
- struct sock *sk1 = sock->sk;
+ struct sock *sk1 = sock->sk, *sk2;
int err = -EINVAL;
- struct sock *sk2 = sk1->sk_prot->accept(sk1, flags, &err, kern);
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ sk2 = READ_ONCE(sk1->sk_prot)->accept(sk1, flags, &err, kern);
if (!sk2)
goto do_err;
@@ -834,12 +840,15 @@ ssize_t inet_sendpage(struct socket *sock, struct page *page, int offset,
size_t size, int flags)
{
struct sock *sk = sock->sk;
+ const struct proto *prot;
if (unlikely(inet_send_prepare(sk)))
return -EAGAIN;
- if (sk->sk_prot->sendpage)
- return sk->sk_prot->sendpage(sk, page, offset, size, flags);
+ /* IPV6_ADDRFORM can change sk->sk_prot under us. */
+ prot = READ_ONCE(sk->sk_prot);
+ if (prot->sendpage)
+ return prot->sendpage(sk, page, offset, size, flags);
return sock_no_sendpage(sock, page, offset, size, flags);
}
EXPORT_SYMBOL(inet_sendpage);
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 485d381aa4475..b8970fdeb0dda 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -475,7 +475,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
sock_prot_inuse_add(net, sk->sk_prot, -1);
sock_prot_inuse_add(net, &tcp_prot, 1);
local_bh_enable();
- /* Paired with READ_ONCE(sk->sk_prot) in net/ipv6/af_inet6.c */
+ /* Paired with READ_ONCE(sk->sk_prot) in inet6_stream_ops */
WRITE_ONCE(sk->sk_prot, &tcp_prot);
icsk->icsk_af_ops = &ipv4_specific;
sk->sk_socket->ops = &inet_stream_ops;
@@ -490,7 +490,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
sock_prot_inuse_add(net, sk->sk_prot, -1);
sock_prot_inuse_add(net, prot, 1);
local_bh_enable();
- /* Paired with READ_ONCE(sk->sk_prot) in net/ipv6/af_inet6.c */
+ /* Paired with READ_ONCE(sk->sk_prot) in inet6_dgram_ops */
WRITE_ONCE(sk->sk_prot, prot);
sk->sk_socket->ops = &inet_dgram_ops;
sk->sk_family = PF_INET;
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 5.15 3/3] tcp: Fix data races around icsk->icsk_af_ops.
2023-04-17 16:53 [PATCH 5.15 0/3] ipv6/v4: Fix data races around sk->sk_prot and icsk->icsk_af_ops Kazunori Kobayashi
2023-04-17 16:53 ` [PATCH 5.15 1/3] ipv6: annotate some data-races around sk->sk_prot Kazunori Kobayashi
2023-04-17 16:53 ` [PATCH 5.15 2/3] ipv6: Fix data races " Kazunori Kobayashi
@ 2023-04-17 16:53 ` Kazunori Kobayashi
2 siblings, 0 replies; 6+ messages in thread
From: Kazunori Kobayashi @ 2023-04-17 16:53 UTC (permalink / raw)
To: netdev
Cc: stable, linux-kernel, hiraku.toyooka, Kuniyuki Iwashima,
Jakub Kicinski, Kazunori Kobayashi
From: Kuniyuki Iwashima <kuniyu@amazon.com>
commit f49cd2f4d6170d27a2c61f1fecb03d8a70c91f57 upstream.
setsockopt(IPV6_ADDRFORM) and tcp_v6_connect() change icsk->icsk_af_ops
under lock_sock(), but tcp_(get|set)sockopt() read it locklessly. To
avoid load/store tearing, we need to add READ_ONCE() and WRITE_ONCE()
for the reads and writes.
Thanks to Eric Dumazet for providing the syzbot report:
BUG: KCSAN: data-race in tcp_setsockopt / tcp_v6_connect
write to 0xffff88813c624518 of 8 bytes by task 23936 on cpu 0:
tcp_v6_connect+0x5b3/0xce0 net/ipv6/tcp_ipv6.c:240
__inet_stream_connect+0x159/0x6d0 net/ipv4/af_inet.c:660
inet_stream_connect+0x44/0x70 net/ipv4/af_inet.c:724
__sys_connect_file net/socket.c:1976 [inline]
__sys_connect+0x197/0x1b0 net/socket.c:1993
__do_sys_connect net/socket.c:2003 [inline]
__se_sys_connect net/socket.c:2000 [inline]
__x64_sys_connect+0x3d/0x50 net/socket.c:2000
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
read to 0xffff88813c624518 of 8 bytes by task 23937 on cpu 1:
tcp_setsockopt+0x147/0x1c80 net/ipv4/tcp.c:3789
sock_common_setsockopt+0x5d/0x70 net/core/sock.c:3585
__sys_setsockopt+0x212/0x2b0 net/socket.c:2252
__do_sys_setsockopt net/socket.c:2263 [inline]
__se_sys_setsockopt net/socket.c:2260 [inline]
__x64_sys_setsockopt+0x62/0x70 net/socket.c:2260
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
value changed: 0xffffffff8539af68 -> 0xffffffff8539aff8
Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 23937 Comm: syz-executor.5 Not tainted
6.0.0-rc4-syzkaller-00331-g4ed9c1e971b1-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 08/26/2022
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot <syzkaller@googlegroups.com>
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Kazunori Kobayashi <kazunori.kobayashi@miraclelinux.com>
---
net/ipv4/tcp.c | 10 ++++++----
net/ipv6/ipv6_sockglue.c | 3 ++-
net/ipv6/tcp_ipv6.c | 6 ++++--
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 9c7998377d6bd..e348b69ef0f5e 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3704,8 +3704,9 @@ int tcp_setsockopt(struct sock *sk, int level, int optname, sockptr_t optval,
const struct inet_connection_sock *icsk = inet_csk(sk);
if (level != SOL_TCP)
- return icsk->icsk_af_ops->setsockopt(sk, level, optname,
- optval, optlen);
+ /* Paired with WRITE_ONCE() in do_ipv6_setsockopt() and tcp_v6_connect() */
+ return READ_ONCE(icsk->icsk_af_ops)->setsockopt(sk, level, optname,
+ optval, optlen);
return do_tcp_setsockopt(sk, level, optname, optval, optlen);
}
EXPORT_SYMBOL(tcp_setsockopt);
@@ -4303,8 +4304,9 @@ int tcp_getsockopt(struct sock *sk, int level, int optname, char __user *optval,
struct inet_connection_sock *icsk = inet_csk(sk);
if (level != SOL_TCP)
- return icsk->icsk_af_ops->getsockopt(sk, level, optname,
- optval, optlen);
+ /* Paired with WRITE_ONCE() in do_ipv6_setsockopt() and tcp_v6_connect() */
+ return READ_ONCE(icsk->icsk_af_ops)->getsockopt(sk, level, optname,
+ optval, optlen);
return do_tcp_getsockopt(sk, level, optname, optval, optlen);
}
EXPORT_SYMBOL(tcp_getsockopt);
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index b8970fdeb0dda..a1a92480e935f 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -477,7 +477,8 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
local_bh_enable();
/* Paired with READ_ONCE(sk->sk_prot) in inet6_stream_ops */
WRITE_ONCE(sk->sk_prot, &tcp_prot);
- icsk->icsk_af_ops = &ipv4_specific;
+ /* Paired with READ_ONCE() in tcp_(get|set)sockopt() */
+ WRITE_ONCE(icsk->icsk_af_ops, &ipv4_specific);
sk->sk_socket->ops = &inet_stream_ops;
sk->sk_family = PF_INET;
tcp_sync_mss(sk, icsk->icsk_pmtu_cookie);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index c18fdddbfa09d..29c8d386274bf 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -237,7 +237,8 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
sin.sin_port = usin->sin6_port;
sin.sin_addr.s_addr = usin->sin6_addr.s6_addr32[3];
- icsk->icsk_af_ops = &ipv6_mapped;
+ /* Paired with READ_ONCE() in tcp_(get|set)sockopt() */
+ WRITE_ONCE(icsk->icsk_af_ops, &ipv6_mapped);
if (sk_is_mptcp(sk))
mptcpv6_handle_mapped(sk, true);
sk->sk_backlog_rcv = tcp_v4_do_rcv;
@@ -249,7 +250,8 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
if (err) {
icsk->icsk_ext_hdr_len = exthdrlen;
- icsk->icsk_af_ops = &ipv6_specific;
+ /* Paired with READ_ONCE() in tcp_(get|set)sockopt() */
+ WRITE_ONCE(icsk->icsk_af_ops, &ipv6_specific);
if (sk_is_mptcp(sk))
mptcpv6_handle_mapped(sk, false);
sk->sk_backlog_rcv = tcp_v6_do_rcv;
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread