Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

rxrpc: Fix data-race warning and potential load/store tearing

Fix the following:

BUG: KCSAN: data-race in rxrpc_peer_keepalive_worker / rxrpc_send_data_packet

which is reporting an issue with the reads and writes to ->last_tx_at in:

conn->peer->last_tx_at = ktime_get_seconds();

and:

keepalive_at = peer->last_tx_at + RXRPC_KEEPALIVE_TIME;

The lockless accesses to these to values aren't actually a problem as the
read only needs an approximate time of last transmission for the purposes
of deciding whether or not the transmission of a keepalive packet is
warranted yet.

Also, as ->last_tx_at is a 64-bit value, tearing can occur on a 32-bit
arch.

Fix both of these by switching to an unsigned int for ->last_tx_at and only
storing the LSW of the time64_t. It can then be reconstructed at need
provided no more than 68 years has elapsed since the last transmission.

Fixes: ace45bec6d77 ("rxrpc: Fix firewall route keepalive")
Reported-by: syzbot+6182afad5045e6703b3d@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/r/695e7cfb.050a0220.1c677c.036b.GAE@google.com/
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@kernel.org
Link: https://patch.msgid.link/1107124.1768903985@warthog.procyon.org.uk
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

authored by

David Howells and committed by
Jakub Kicinski
5d5fe8bc 869f3f7f

+36 -14
+8 -1
net/rxrpc/ar-internal.h
··· 387 387 struct rb_root service_conns; /* Service connections */ 388 388 struct list_head keepalive_link; /* Link in net->peer_keepalive[] */ 389 389 unsigned long app_data; /* Application data (e.g. afs_server) */ 390 - time64_t last_tx_at; /* Last time packet sent here */ 390 + unsigned int last_tx_at; /* Last time packet sent here (time64_t LSW) */ 391 391 seqlock_t service_conn_lock; 392 392 spinlock_t lock; /* access lock */ 393 393 int debug_id; /* debug ID for printks */ ··· 1378 1378 void rxrpc_peer_keepalive_worker(struct work_struct *); 1379 1379 void rxrpc_input_probe_for_pmtud(struct rxrpc_connection *conn, rxrpc_serial_t acked_serial, 1380 1380 bool sendmsg_fail); 1381 + 1382 + /* Update the last transmission time on a peer for keepalive purposes. */ 1383 + static inline void rxrpc_peer_mark_tx(struct rxrpc_peer *peer) 1384 + { 1385 + /* To avoid tearing on 32-bit systems, we only keep the LSW. */ 1386 + WRITE_ONCE(peer->last_tx_at, ktime_get_seconds()); 1387 + } 1381 1388 1382 1389 /* 1383 1390 * peer_object.c
+1 -1
net/rxrpc/conn_event.c
··· 194 194 } 195 195 196 196 ret = kernel_sendmsg(conn->local->socket, &msg, iov, ioc, len); 197 - conn->peer->last_tx_at = ktime_get_seconds(); 197 + rxrpc_peer_mark_tx(conn->peer); 198 198 if (ret < 0) 199 199 trace_rxrpc_tx_fail(chan->call_debug_id, serial, ret, 200 200 rxrpc_tx_point_call_final_resend);
+7 -7
net/rxrpc/output.c
··· 275 275 rxrpc_local_dont_fragment(conn->local, why == rxrpc_propose_ack_ping_for_mtu_probe); 276 276 277 277 ret = do_udp_sendmsg(conn->local->socket, &msg, len); 278 - call->peer->last_tx_at = ktime_get_seconds(); 278 + rxrpc_peer_mark_tx(call->peer); 279 279 if (ret < 0) { 280 280 trace_rxrpc_tx_fail(call->debug_id, serial, ret, 281 281 rxrpc_tx_point_call_ack); ··· 411 411 412 412 iov_iter_kvec(&msg.msg_iter, WRITE, iov, 1, sizeof(pkt)); 413 413 ret = do_udp_sendmsg(conn->local->socket, &msg, sizeof(pkt)); 414 - conn->peer->last_tx_at = ktime_get_seconds(); 414 + rxrpc_peer_mark_tx(conn->peer); 415 415 if (ret < 0) 416 416 trace_rxrpc_tx_fail(call->debug_id, serial, ret, 417 417 rxrpc_tx_point_call_abort); ··· 698 698 ret = 0; 699 699 trace_rxrpc_tx_data(call, txb->seq, txb->serial, txb->flags, 700 700 rxrpc_txdata_inject_loss); 701 - conn->peer->last_tx_at = ktime_get_seconds(); 701 + rxrpc_peer_mark_tx(conn->peer); 702 702 goto done; 703 703 } 704 704 } ··· 711 711 */ 712 712 rxrpc_inc_stat(call->rxnet, stat_tx_data_send); 713 713 ret = do_udp_sendmsg(conn->local->socket, &msg, len); 714 - conn->peer->last_tx_at = ktime_get_seconds(); 714 + rxrpc_peer_mark_tx(conn->peer); 715 715 716 716 if (ret == -EMSGSIZE) { 717 717 rxrpc_inc_stat(call->rxnet, stat_tx_data_send_msgsize); ··· 797 797 798 798 trace_rxrpc_tx_packet(conn->debug_id, &whdr, rxrpc_tx_point_conn_abort); 799 799 800 - conn->peer->last_tx_at = ktime_get_seconds(); 800 + rxrpc_peer_mark_tx(conn->peer); 801 801 } 802 802 803 803 /* ··· 917 917 trace_rxrpc_tx_packet(peer->debug_id, &whdr, 918 918 rxrpc_tx_point_version_keepalive); 919 919 920 - peer->last_tx_at = ktime_get_seconds(); 920 + rxrpc_peer_mark_tx(peer); 921 921 _leave(""); 922 922 } 923 923 ··· 973 973 if (ret < 0) 974 974 goto fail; 975 975 976 - conn->peer->last_tx_at = ktime_get_seconds(); 976 + rxrpc_peer_mark_tx(conn->peer); 977 977 return; 978 978 979 979 fail:
+16 -1
net/rxrpc/peer_event.c
··· 238 238 } 239 239 240 240 /* 241 + * Reconstruct the last transmission time. The difference calculated should be 242 + * valid provided no more than ~68 years elapsed since the last transmission. 243 + */ 244 + static time64_t rxrpc_peer_get_tx_mark(const struct rxrpc_peer *peer, time64_t base) 245 + { 246 + s32 last_tx_at = READ_ONCE(peer->last_tx_at); 247 + s32 base_lsw = base; 248 + s32 diff = last_tx_at - base_lsw; 249 + 250 + diff = clamp(diff, -RXRPC_KEEPALIVE_TIME, RXRPC_KEEPALIVE_TIME); 251 + 252 + return diff + base; 253 + } 254 + 255 + /* 241 256 * Perform keep-alive pings. 242 257 */ 243 258 static void rxrpc_peer_keepalive_dispatch(struct rxrpc_net *rxnet, ··· 280 265 spin_unlock_bh(&rxnet->peer_hash_lock); 281 266 282 267 if (use) { 283 - keepalive_at = peer->last_tx_at + RXRPC_KEEPALIVE_TIME; 268 + keepalive_at = rxrpc_peer_get_tx_mark(peer, base) + RXRPC_KEEPALIVE_TIME; 284 269 slot = keepalive_at - base; 285 270 _debug("%02x peer %u t=%d {%pISp}", 286 271 cursor, peer->debug_id, slot, &peer->srx.transport);
+2 -2
net/rxrpc/proc.c
··· 296 296 297 297 now = ktime_get_seconds(); 298 298 seq_printf(seq, 299 - "UDP %-47.47s %-47.47s %3u %4u %5u %6llus %8d %8d\n", 299 + "UDP %-47.47s %-47.47s %3u %4u %5u %6ds %8d %8d\n", 300 300 lbuff, 301 301 rbuff, 302 302 refcount_read(&peer->ref), 303 303 peer->cong_ssthresh, 304 304 peer->max_data, 305 - now - peer->last_tx_at, 305 + (s32)now - (s32)READ_ONCE(peer->last_tx_at), 306 306 READ_ONCE(peer->recent_srtt_us), 307 307 READ_ONCE(peer->recent_rto_us)); 308 308
+1 -1
net/rxrpc/rxgk.c
··· 678 678 679 679 ret = do_udp_sendmsg(conn->local->socket, &msg, len); 680 680 if (ret > 0) 681 - conn->peer->last_tx_at = ktime_get_seconds(); 681 + rxrpc_peer_mark_tx(conn->peer); 682 682 __free_page(page); 683 683 684 684 if (ret < 0) {
+1 -1
net/rxrpc/rxkad.c
··· 694 694 return -EAGAIN; 695 695 } 696 696 697 - conn->peer->last_tx_at = ktime_get_seconds(); 697 + rxrpc_peer_mark_tx(conn->peer); 698 698 trace_rxrpc_tx_packet(conn->debug_id, &whdr, 699 699 rxrpc_tx_point_rxkad_challenge); 700 700 _leave(" = 0");