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

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 <[email protected]>

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?