rxrpc: serialize kernel accept preallocation with socket teardown
Subsystems (3)
PATCH 1/1 rxrpc: serialize kernel accept preallocation with socket teardown
In reply to a message not in the archiveFrom: Li Daming <[email protected]>
rxrpc_kernel_charge_accept() reads rx->backlog without any socket/backlog synchronization and passes that raw pointer into rxrpc_service_prealloc_one(). A concurrent rxrpc_discard_prealloc() sets rx->backlog = NULL and frees the backlog rings, so a kernel preallocation worker can keep using a freed struct rxrpc_backlog while updating *_backlog_head/tail and array slots.
Serialize the state check and backlog lookup with the socket lock, and reject kernel preallocation once teardown has disabled listening or discarded the service backlog.
--- net/rxrpc/call_accept.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
net/rxrpc/call_accept.c +19 -6
@@ -471,13 +471,26 @@ int rxrpc_kernel_charge_accept(struct socket *sock, rxrpc_notify_rx_t notify_rx, unsigned long user_call_ID, gfp_t gfp, unsigned int debug_id) { - struct rxrpc_sock *rx = rxrpc_sk(sock->sk); -struct rxrpc_backlog *b = rx->backlog; + struct rxrpc_backlog *b; +struct rxrpc_sock *rx; +struct sock *sk; +int ret; -if (sock->sk->sk_state ==RXRPC_CLOSE ) -return -ESHUTDOWN ; + sk = sock->sk; + rx =rxrpc_sk (sk); + +lock_sock (sk); +if (sk->sk_state !=RXRPC_SERVER_LISTENING || !rx->backlog) { + ret = -ESHUTDOWN ; + goto out; + } + + b = rx->backlog; + ret =rxrpc_service_prealloc_one (rx, b, notify_rx, user_call_ID, + gfp, debug_id); -return rxrpc_service_prealloc_one (rx, b, notify_rx, user_call_ID, - gfp, debug_id); +out: +release_sock (sk); +return ret; }EXPORT_SYMBOL (rxrpc_kernel_charge_accept);
Signature · 1 line
2.43.0
Simon Horman
reviewerFrom: 'Simon Horman' <[email protected]>
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
rxrpc: serialize kernel accept preallocation with socket teardown
This commit attempts to fix a use-after-free in the rxrpc accept preallocation path by serializing the state check and backlog lookup with the socket lock.
Quoted 8 lines
net/rxrpc/call_accept.c +0
-0@@ -471,13 +471,26 @@ int rxrpc_kernel_charge_accept(struct socket *sock, rxrpc_notify_rx_t notify_rx, unsigned long user_call_ID, gfp_t gfp, unsigned int debug_id) {
net/rxrpc/call_accept.c int rxrpc_kernel_charge_accept(struct socket *sock, rxrpc_notify_rx_t notify_rx, · line 479
Quoted 10 lines
net/rxrpc/call_accept.c +5
-4-struct rxrpc_sock *rx =rxrpc_sk (sock->sk); -struct rxrpc_backlog *b = rx->backlog; +struct rxrpc_backlog *b; +struct rxrpc_sock *rx; +struct sock *sk; +int ret; -if (sock->sk->sk_state ==RXRPC_CLOSE ) -return -ESHUTDOWN ; + sk = sock->sk;
Could the sock pointer passed to this function already be freed by the time it executes?
The AFS subsystem afs_rx_new_call() queues charge_preallocation_work on afs_wq. However, during socket teardown, afs_close_socket() appears to flush afs_async_calls instead of afs_wq and doesn't cancel the work item with cancel_work_sync().
If the stray work executes asynchronously after sock_release() has destroyed the socket, wouldn't dereferencing sock->sk result in a use-after-free?
net/rxrpc/call_accept.c int rxrpc_kernel_charge_accept(struct socket *sock, rxrpc_notify_rx_t notify_rx, · line 482
Quoted 3 lines
net/rxrpc/call_accept.c +3
-0+ rx =rxrpc_sk (sk); + +lock_sock (sk);
Does lock_sock() adequately protect against concurrent socket release?
Looking at the socket release path, rxrpc_release_sock() modifies sk->sk_state and calls rxrpc_discard_prealloc() (which frees the backlog) without holding lock_sock().
If rxrpc_kernel_charge_accept() runs concurrently with socket release, could rxrpc_release_sock() locklessly free the backlog from under it while this function holds the socket lock, leading to a use-after-free when accessing the backlog below?
Quoted 8 lines
net/rxrpc/call_accept.c +8
-0+if (sk->sk_state !=RXRPC_SERVER_LISTENING || !rx->backlog) { + ret = -ESHUTDOWN ; + goto out; + } + + b = rx->backlog; + ret =rxrpc_service_prealloc_one (rx, b, notify_rx, user_call_ID, + gfp, debug_id);
[ ... ]