Skip to content

Commit f866fbc

Browse files
edumazetdavem330
authored andcommitted
ipv4: fix data-races around inet->inet_id
UDP sendmsg() is lockless, so ip_select_ident_segs() can very well be run from multiple cpus [1] Convert inet->inet_id to an atomic_t, but implement a dedicated path for TCP, avoiding cost of a locked instruction (atomic_add_return()) Note that this patch will cause a trivial merge conflict because we added inet->flags in net-next tree. v2: added missing change in drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c (David Ahern) [1] BUG: KCSAN: data-race in __ip_make_skb / __ip_make_skb read-write to 0xffff888145af952a of 2 bytes by task 7803 on cpu 1: ip_select_ident_segs include/net/ip.h:542 [inline] ip_select_ident include/net/ip.h:556 [inline] __ip_make_skb+0x844/0xc70 net/ipv4/ip_output.c:1446 ip_make_skb+0x233/0x2c0 net/ipv4/ip_output.c:1560 udp_sendmsg+0x1199/0x1250 net/ipv4/udp.c:1260 inet_sendmsg+0x63/0x80 net/ipv4/af_inet.c:830 sock_sendmsg_nosec net/socket.c:725 [inline] sock_sendmsg net/socket.c:748 [inline] ____sys_sendmsg+0x37c/0x4d0 net/socket.c:2494 ___sys_sendmsg net/socket.c:2548 [inline] __sys_sendmmsg+0x269/0x500 net/socket.c:2634 __do_sys_sendmmsg net/socket.c:2663 [inline] __se_sys_sendmmsg net/socket.c:2660 [inline] __x64_sys_sendmmsg+0x57/0x60 net/socket.c:2660 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd read to 0xffff888145af952a of 2 bytes by task 7804 on cpu 0: ip_select_ident_segs include/net/ip.h:541 [inline] ip_select_ident include/net/ip.h:556 [inline] __ip_make_skb+0x817/0xc70 net/ipv4/ip_output.c:1446 ip_make_skb+0x233/0x2c0 net/ipv4/ip_output.c:1560 udp_sendmsg+0x1199/0x1250 net/ipv4/udp.c:1260 inet_sendmsg+0x63/0x80 net/ipv4/af_inet.c:830 sock_sendmsg_nosec net/socket.c:725 [inline] sock_sendmsg net/socket.c:748 [inline] ____sys_sendmsg+0x37c/0x4d0 net/socket.c:2494 ___sys_sendmsg net/socket.c:2548 [inline] __sys_sendmmsg+0x269/0x500 net/socket.c:2634 __do_sys_sendmmsg net/socket.c:2663 [inline] __se_sys_sendmmsg net/socket.c:2660 [inline] __x64_sys_sendmmsg+0x57/0x60 net/socket.c:2660 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd value changed: 0x184d -> 0x184e Reported by Kernel Concurrency Sanitizer on: CPU: 0 PID: 7804 Comm: syz-executor.1 Not tainted 6.5.0-rc6-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023 ================================================================== Fixes: 23f5740 ("ipv4: avoid using shared IP generator for connected sockets") Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Eric Dumazet <edumazet@google.com> Reviewed-by: David Ahern <dsahern@kernel.org> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent f534f65 commit f866fbc

File tree

8 files changed

+22
-11
lines changed

8 files changed

+22
-11
lines changed

drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1466,7 +1466,7 @@ static void make_established(struct sock *sk, u32 snd_isn, unsigned int opt)
14661466
tp->write_seq = snd_isn;
14671467
tp->snd_nxt = snd_isn;
14681468
tp->snd_una = snd_isn;
1469-
inet_sk(sk)->inet_id = get_random_u16();
1469+
atomic_set(&inet_sk(sk)->inet_id, get_random_u16());
14701470
assign_rxopt(sk, opt);
14711471

14721472
if (tp->rcv_wnd > (RCV_BUFSIZ_M << 10))

include/net/inet_sock.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,8 +222,8 @@ struct inet_sock {
222222
__s16 uc_ttl;
223223
__u16 cmsg_flags;
224224
struct ip_options_rcu __rcu *inet_opt;
225+
atomic_t inet_id;
225226
__be16 inet_sport;
226-
__u16 inet_id;
227227

228228
__u8 tos;
229229
__u8 min_ttl;

include/net/ip.h

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -538,8 +538,19 @@ static inline void ip_select_ident_segs(struct net *net, struct sk_buff *skb,
538538
* generator as much as we can.
539539
*/
540540
if (sk && inet_sk(sk)->inet_daddr) {
541-
iph->id = htons(inet_sk(sk)->inet_id);
542-
inet_sk(sk)->inet_id += segs;
541+
int val;
542+
543+
/* avoid atomic operations for TCP,
544+
* as we hold socket lock at this point.
545+
*/
546+
if (sk_is_tcp(sk)) {
547+
sock_owned_by_me(sk);
548+
val = atomic_read(&inet_sk(sk)->inet_id);
549+
atomic_set(&inet_sk(sk)->inet_id, val + segs);
550+
} else {
551+
val = atomic_add_return(segs, &inet_sk(sk)->inet_id);
552+
}
553+
iph->id = htons(val);
543554
return;
544555
}
545556
if ((iph->frag_off & htons(IP_DF)) && !skb->ignore_df) {

net/dccp/ipv4.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ int dccp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
130130
inet->inet_daddr,
131131
inet->inet_sport,
132132
inet->inet_dport);
133-
inet->inet_id = get_random_u16();
133+
atomic_set(&inet->inet_id, get_random_u16());
134134

135135
err = dccp_connect(sk);
136136
rt = NULL;
@@ -432,7 +432,7 @@ struct sock *dccp_v4_request_recv_sock(const struct sock *sk,
432432
RCU_INIT_POINTER(newinet->inet_opt, rcu_dereference(ireq->ireq_opt));
433433
newinet->mc_index = inet_iif(skb);
434434
newinet->mc_ttl = ip_hdr(skb)->ttl;
435-
newinet->inet_id = get_random_u16();
435+
atomic_set(&newinet->inet_id, get_random_u16());
436436

437437
if (dst == NULL && (dst = inet_csk_route_child_sock(sk, newsk, req)) == NULL)
438438
goto put_and_exit;

net/ipv4/af_inet.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ static int inet_create(struct net *net, struct socket *sock, int protocol,
340340
else
341341
inet->pmtudisc = IP_PMTUDISC_WANT;
342342

343-
inet->inet_id = 0;
343+
atomic_set(&inet->inet_id, 0);
344344

345345
sock_init_data(sock, sk);
346346

net/ipv4/datagram.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
7373
reuseport_has_conns_set(sk);
7474
sk->sk_state = TCP_ESTABLISHED;
7575
sk_set_txhash(sk);
76-
inet->inet_id = get_random_u16();
76+
atomic_set(&inet->inet_id, get_random_u16());
7777

7878
sk_dst_set(sk, &rt->dst);
7979
err = 0;

net/ipv4/tcp_ipv4.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
312312
inet->inet_daddr));
313313
}
314314

315-
inet->inet_id = get_random_u16();
315+
atomic_set(&inet->inet_id, get_random_u16());
316316

317317
if (tcp_fastopen_defer_connect(sk, &err))
318318
return err;
@@ -1596,7 +1596,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
15961596
inet_csk(newsk)->icsk_ext_hdr_len = 0;
15971597
if (inet_opt)
15981598
inet_csk(newsk)->icsk_ext_hdr_len = inet_opt->opt.optlen;
1599-
newinet->inet_id = get_random_u16();
1599+
atomic_set(&newinet->inet_id, get_random_u16());
16001600

16011601
/* Set ToS of the new socket based upon the value of incoming SYN.
16021602
* ECT bits are set later in tcp_init_transfer().

net/sctp/socket.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9479,7 +9479,7 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk,
94799479
newinet->inet_rcv_saddr = inet->inet_rcv_saddr;
94809480
newinet->inet_dport = htons(asoc->peer.port);
94819481
newinet->pmtudisc = inet->pmtudisc;
9482-
newinet->inet_id = get_random_u16();
9482+
atomic_set(&newinet->inet_id, get_random_u16());
94839483

94849484
newinet->uc_ttl = inet->uc_ttl;
94859485
newinet->mc_loop = 1;

0 commit comments

Comments
 (0)