| Message ID | 20240606154652.360331-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d37fe4255abe8e7b419b90c5847e8ec2b8debb08 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tcp: fix race in tcp_v6_syn_recv_sock() | expand |
+ David Ahern On Thu, Jun 06, 2024 at 03:46:51PM +0000, Eric Dumazet wrote: > tcp_v6_syn_recv_sock() calls ip6_dst_store() before > inet_sk(newsk)->pinet6 has been set up. > > This means ip6_dst_store() writes over the parent (listener) > np->dst_cookie. > > This is racy because multiple threads could share the same > parent and their final np->dst_cookie could be wrong. > > Move ip6_dst_store() call after inet_sk(newsk)->pinet6 > has been changed and after the copy of parent ipv6_pinfo. > > Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets") > Signed-off-by: Eric Dumazet <edumazet@google.com> Reviewed-by: Simon Horman <horms@kernel.org> > --- > net/ipv6/tcp_ipv6.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c > index 8c577b651bfcd2f94b45e339ed4a2b47e93ff17a..729faf8bd366ad25d093a4ae931fb46ebd45b79c 100644 > --- a/net/ipv6/tcp_ipv6.c > +++ b/net/ipv6/tcp_ipv6.c > @@ -1439,7 +1439,6 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff * > */ > > newsk->sk_gso_type = SKB_GSO_TCPV6; > - ip6_dst_store(newsk, dst, NULL, NULL); > inet6_sk_rx_dst_set(newsk, skb); > > inet_sk(newsk)->pinet6 = tcp_inet6_sk(newsk); > @@ -1450,6 +1449,8 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff * > > memcpy(newnp, np, sizeof(struct ipv6_pinfo)); > > + ip6_dst_store(newsk, dst, NULL, NULL); > + > newsk->sk_v6_daddr = ireq->ir_v6_rmt_addr; > newnp->saddr = ireq->ir_v6_loc_addr; > newsk->sk_v6_rcv_saddr = ireq->ir_v6_loc_addr; > -- > 2.45.1.467.gbab1589fc0-goog > >
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Thu, 6 Jun 2024 15:46:51 +0000 you wrote: > tcp_v6_syn_recv_sock() calls ip6_dst_store() before > inet_sk(newsk)->pinet6 has been set up. > > This means ip6_dst_store() writes over the parent (listener) > np->dst_cookie. > > This is racy because multiple threads could share the same > parent and their final np->dst_cookie could be wrong. > > [...] Here is the summary with links: - [net] tcp: fix race in tcp_v6_syn_recv_sock() https://git.kernel.org/netdev/net/c/d37fe4255abe You are awesome, thank you!
On 6/8/24 7:25 AM, Simon Horman wrote: > + David Ahern > > On Thu, Jun 06, 2024 at 03:46:51PM +0000, Eric Dumazet wrote: >> tcp_v6_syn_recv_sock() calls ip6_dst_store() before >> inet_sk(newsk)->pinet6 has been set up. >> >> This means ip6_dst_store() writes over the parent (listener) >> np->dst_cookie. >> >> This is racy because multiple threads could share the same >> parent and their final np->dst_cookie could be wrong. >> >> Move ip6_dst_store() call after inet_sk(newsk)->pinet6 >> has been changed and after the copy of parent ipv6_pinfo. >> >> Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets") >> Signed-off-by: Eric Dumazet <edumazet@google.com> > > Reviewed-by: Simon Horman <horms@kernel.org> > >> --- >> net/ipv6/tcp_ipv6.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> Reviewed-by: David Ahern <dsahern@kernel.org>
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 8c577b651bfcd2f94b45e339ed4a2b47e93ff17a..729faf8bd366ad25d093a4ae931fb46ebd45b79c 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -1439,7 +1439,6 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff * */ newsk->sk_gso_type = SKB_GSO_TCPV6; - ip6_dst_store(newsk, dst, NULL, NULL); inet6_sk_rx_dst_set(newsk, skb); inet_sk(newsk)->pinet6 = tcp_inet6_sk(newsk); @@ -1450,6 +1449,8 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff * memcpy(newnp, np, sizeof(struct ipv6_pinfo)); + ip6_dst_store(newsk, dst, NULL, NULL); + newsk->sk_v6_daddr = ireq->ir_v6_rmt_addr; newnp->saddr = ireq->ir_v6_loc_addr; newsk->sk_v6_rcv_saddr = ireq->ir_v6_loc_addr;
tcp_v6_syn_recv_sock() calls ip6_dst_store() before inet_sk(newsk)->pinet6 has been set up. This means ip6_dst_store() writes over the parent (listener) np->dst_cookie. This is racy because multiple threads could share the same parent and their final np->dst_cookie could be wrong. Move ip6_dst_store() call after inet_sk(newsk)->pinet6 has been changed and after the copy of parent ipv6_pinfo. Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets") Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/ipv6/tcp_ipv6.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)