Re: [PATCH 1/1] nfc: llcp: Fix race condition in accept_queue lifecycle

[David Heidelberg]

On 11/08/2026 15:14, Lee Jones wrote:
> [INTENTIONAL TOP POST]
>
> This one has been knocking around for a very long time.
>
> Would you like me to [RESEND] it, or does everyone still have it?
Hello Lee,

sorry, I've must missed the patch. Could you check the Sashiko comments (below)?

Feel free to ignore the pre-existing issue, the main complaint is about the 
description.

Thank you
David

commit 74abfc44e401a556abde5003d20ce7cf50ee958d
Author: Lee Jones <lee@xxxxxxxxxx>

nfc: llcp: Fix race condition in accept_queue lifecycle

This commit attempts to fix a race condition between nfc_llcp_socket_release()
and nfc_llcp_accept_dequeue() by replacing bh_lock_sock() with lock_sock()
and lock_sock_nested(). It also introduces a socket popping helper and
adds a parent check in nfc_llcp_accept_unlink().

> nfc_llcp_socket_release() walks the local sockets list and the listener
> socket's accept_queue under bh_lock_sock().  However, bh_lock_sock()
> does not provide mutual exclusion against process-context lock_sock()
> held by nfc_llcp_accept_dequeue() during accept().  Since socket_release()
> runs in process context (workqueue or kref cleanup), it never checks

Is this description accurate?

nfc_llcp_data_received() routinely runs from softirq/interrupt context
via URB completion handlers like pn533_recv_response(). When it drops
the last reference via nfc_llcp_local_put(), this triggers local_cleanup()
and nfc_llcp_socket_release().

Could this lead to a sleeping in atomic bug since lock_sock() acquires
a mutex?

> sock_owned_by_user(), allowing both paths to manipulate the same child
> socket concurrently.  This leads to a double sock_put() (Use-After-Free)
> or a NULL pointer dereference of child->parent in nfc_llcp_accept_unlink().

[ ... ]

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index dc65c719f35f2..e51bd0d081b62 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -63,21 +63,33 @@ static void nfc_llcp_socket_purge(struct nfc_llcp_sock *sock)
>  	}
>  }
>
[ ... ]
> @@ -92,7 +104,8 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local 
*local, bool device,
> -	sk_for_each_safe(sk, tmp, &local->sockets.head) {
> +	while ((sk = nfc_llcp_sock_list_pop(&local->sockets))) {
>  		llcp_sock = nfc_llcp_sock(sk);
>
> -		bh_lock_sock(sk);
> +		lock_sock(sk);

Since nfc_llcp_socket_release() can be invoked from softirq teardown paths,
does calling lock_sock() here cause a sleeping in atomic bug because it
acquires a mutex?

[ ... ]
>  						 &llcp_sock->accept_queue,
>  						 accept_queue) {
>  				accept_sk = &lsk->sk;
> -				bh_lock_sock(accept_sk);
> +				lock_sock_nested(accept_sk,
> +						 SINGLE_DEPTH_NESTING);

Does this nested lock_sock_nested() also cause a sleep in atomic context
when nfc_llcp_socket_release() is invoked from softirq teardown paths?

>  				nfc_llcp_accept_unlink(accept_sk);

This isn't a bug introduced by this patch, but does this leak the initial
sk_alloc reference count?

Sockets created for incoming connections receive an initial sk_alloc
reference. When the listener is closed, unaccepted child sockets are
unlinked here and their list references are dropped.

Since they were never accepted by userspace, sock_release() is never called.
Do these unaccepted sockets permanently leak?

[ ... ]
> @@ -135,12 +143,9 @@ static void nfc_llcp_socket_release(struct 
nfc_llcp_local *local, bool device,
>  		sk->sk_state = LLCP_CLOSED;
>  		sk->sk_state_change(sk);
>
> -		bh_unlock_sock(sk);
> -
> -		sk_del_node_init(sk);
> +		release_sock(sk);
> +		sock_put(sk);
>  	}
> -
> -	write_unlock(&local->raw_sockets.lock);
>  }
>
>  static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)

This isn't a bug introduced by this patch, but is it possible for a dying
object to be resurrected here?

If local_release() runs local_cleanup(), it purges sockets via
nfc_llcp_socket_release() and then waits for rx_work via cancel_work_sync().

If rx_work executes concurrently, it calls nfc_llcp_local_get() which uses
kref_get() without checking for zero. Can rx_work then allocate and link a
new socket just before local is freed, leading to a Use-After-Free?

> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index feab29fc62f44..3d30f0233b986 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
[ ... ]
> @@ -622,7 +623,7 @@ static int llcp_sock_release(struct socket *sock)
>  		list_for_each_entry_safe(lsk, n, &llcp_sock->accept_queue,
>  					 accept_queue) {
>  			accept_sk = &lsk->sk;
> -			lock_sock(accept_sk);
> +			lock_sock_nested(accept_sk, SINGLE_DEPTH_NESTING);
>
>  			nfc_llcp_send_disconnect(lsk);
>  			nfc_llcp_accept_unlink(accept_sk);

This isn't a bug introduced by this patch, but as mentioned above, does
unlinking unaccepted child sockets here permanently leak their initial
reference count?

This isn't a bug introduced by this patch, but can the blocking loop in
llcp_sock_accept() hang indefinitely?

If the listener socket is shut down or released concurrently, the thread
wakes up but doesn't check sk->sk_state:

llcp_sock_accept() {
    ...
    while (!(new_sk = nfc_llcp_accept_dequeue(sk, newsock))) {
        set_current_state(TASK_INTERRUPTIBLE);
        ...
        release_sock(sk);
        timeo = schedule_timeout(timeo);
        lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
    }
    ...
}

Will it just go back to sleep waiting for a closed socket if no new
socket was dequeued?