Re: [PATCH net v2 05/10] afs: Fix uncancelled rxrpc OOB message handler

[Simon Horman <horms@kernel.org>]

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
---
afs: Fix uncancelled rxrpc OOB message handler

This patch modifies AFS to cancel its OOB message processing during
shutdown. It moves OOB message processing to afs_wq and adds checks
for the net namespace's live state to prevent processing when no
longer live.

Link: https://sashiko.dev/#/patchset/20260609140911.838677-1-dhowells%40redhat.com

> diff --git a/fs/afs/cm_security.c b/fs/afs/cm_security.c
> index edcbd249d2024..103168c70dd4d 100644
> --- a/fs/afs/cm_security.c
> +++ b/fs/afs/cm_security.c

[ ... ]

> @@ -101,7 +101,8 @@ void afs_process_oob_queue(struct work_struct *work)
>  	struct sk_buff *oob;
>  	enum rxrpc_oob_type type;
>  
> -	while ((oob = rxrpc_kernel_dequeue_oob(net->socket, &type))) {
> +	while (READ_ONCE(net->live) &&
> +	       (oob = rxrpc_kernel_dequeue_oob(net->socket, &type))) {
>  		switch (type) {
>  		case RXRPC_OOB_CHALLENGE:
>  			afs_respond_to_challenge(oob);

[Severity: Critical]
This isn't a bug introduced by this patch, but can afs_respond_to_challenge()
cause a null pointer dereference or use-after-free here?

When afs_respond_to_challenge() queries peer_data and casts it to an
afs_server pointer, it unconditionally dereferences it and locks its mutex:

fs/afs/cm_security.c:afs_respond_to_challenge() {
    ...
    server = (struct afs_server *)peer_data;
    if (!server->cm_rxgk_appdata.data) {
        mutex_lock(&server->cm_token_lock);
    ...
}

If a challenge is received for an unknown peer, or if afs_server_destroyer()
concurrently clears the peer app_data to null during teardown, could 
peer_data be null?

If the pointer was read just before being cleared, could the server object
be freed concurrently since no RCU read lock or refcount is held?

> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index 6714a189d58fb..e8af2a6614405 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c

[ ... ]

> @@ -989,5 +991,6 @@ static void afs_rx_notify_oob(struct sock *sk, struct sk_buff *oob)
>  {
>  	struct afs_net *net = sk->sk_user_data;
>  
> -	schedule_work(&net->rx_oob_work);
> +	if (net->live)
> +		queue_work(afs_wq, &net->rx_oob_work);
>  }

[Severity: Medium]
Should net->live be read with READ_ONCE() here?

Since net->live can be modified concurrently by namespace teardown code
such as afs_close_socket() or afs_net_exit(), reading it without READ_ONCE()
might allow the compiler to optimize the read, leading to a stale evaluation.

The updated afs_process_oob_queue() function correctly uses READ_ONCE().
Should afs_rx_notify_oob() do the same to prevent data races?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a similar
issue in fs/afs/server.c when retrieving the server pointer.

Can afs_find_server() increment the refcount of a freed object?

afs_find_server() fetches the pointer and increments its refcount:

fs/afs/server.c:afs_find_server() {
    ...
    struct afs_server *server = (struct afs_server *)rxrpc_kernel_get_peer_data(peer);
    
    if (!server)
        return NULL;
    return afs_use_server(server, false, afs_server_trace_use_cm_call);
    ...
}

If afs_server_destroyer() runs concurrently, unbinds the peer, and frees
the server, could afs_find_server() fetch the pointer just before it is
cleared?

If preempted, it seems this operation lacks RCU read lock protection and
could increment the refcount of a freed object without using
refcount_inc_not_zero().