From: David Heidelberg <david@ixit.cz>
To: Lee Jones <lee@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Simon Horman <horms@kernel.org>,
Samuel Ortiz <sameo@linux.intel.com>,
Szymon Janc <szymon.janc@tieto.com>,
oe-linux-nfc@lists.linux.dev, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/1] nfc: llcp: Fix race condition in accept_queue lifecycle
Date: Tue, 8 Sep 2026 12:29:38 +0200 [thread overview]
Message-ID: <6beddcea-bc04-471d-be5d-8e391cdedee3@ixit.cz> (raw)
In-Reply-To: <20260908080638.GY2133376@google.com>
Hello Lee,
thank you. Yes, it's.
David
On 08/09/2026 10:06, Lee Jones wrote:
> [INTENTIONAL TOP-POST]
>
> Good morning David,
>
> Just checking that this is on your radar this time [0].
>
> Please let me know if you'd prefer a [RESEND].
>
> [0] https://lore.kernel.org/all/016b7162-b633-4561-a003-526c9d9c5f21@ixit.cz/
>
> Kind regards,
> Lee
>
>> In nfc_llcp_socket_release(), sockets and listener accept queues are
>> walked under the local sockets rwlock and bh_lock_sock(). However,
>> bh_lock_sock() does not synchronise against process-context lock_sock()
>> held by nfc_llcp_accept_dequeue() during accept(). Because
>> socket_release() does not check sock_owned_by_user(), both paths can
>> concurrently unlink and release the same child socket, resulting in
>> use-after-free or a NULL pointer dereference of child->parent in
>> nfc_llcp_accept_unlink().
>>
>> Fix this synchronisation race by having nfc_llcp_socket_release() use
>> process-context lock_sock() instead of bh_lock_sock():
>>
>> 1. Pop sockets from the local sockets list under the write lock using
>> nfc_llcp_sock_list_pop() so lock_sock() can be acquired without
>> holding the rwlock.
>>
>> 2. Because lock_sock() can sleep, defer the final release of the
>> nfc_llcp_local structure to a workqueue (release_work). This avoids
>> a sleeping-in-atomic bug when the last local reference is dropped
>> from softirq context. Additionally, hold a single device reference
>> on local from registration until final destruction.
>>
>> 3. In nfc_llcp_local_get(), use kref_get_unless_zero() to prevent
>> resurrecting a local object whose teardown has been scheduled.
>>
>> 4. In llcp_sock_accept(), verify that the listener socket state is still
>> LLCP_LISTEN after waking from schedule_timeout() to prevent hangs if
>> the listener is closed concurrently.
>>
>> 5. When unlinking unaccepted child sockets during listener release,
>> unlink them from local->sockets, call sock_orphan(), and drop their
>> initial sk_alloc creation reference via sock_put().
>>
>> 6. Make nfc_llcp_accept_unlink() idempotent by guarding parent access with
>> a NULL check.
>>
>> Fixes: 50b78b2a6500 ("NFC: Fix sleeping in atomic when releasing socket")
>> Signed-off-by: Lee Jones <lee@kernel.org>
>> ---
>>
>> v1 -> v2:
>> - Defer local release to dedicated workqueue (llcp_wq) to avoid sleeping in atomic
>> - Drain and destroy llcp_wq on module unload to prevent module exit race
>> - Drop initial sk_alloc creation ref on unaccepted child sockets across all teardown paths
>> - Use kref_get_unless_zero() in local_get to prevent resurrecting dying local objects
>> - Check listener socket state after waking in llcp_sock_accept() to avoid hangs
>> - Standardise Parent (0) -> Child (1) lockdep subclass nesting
>>
>> v2 -> v3:
>> - Allocate dedicated workqueue (llcp_wq) with WQ_UNBOUND to fix syzbot warning
>>
>> net/nfc/llcp.h | 1 +
>> net/nfc/llcp_core.c | 123 +++++++++++++++++++++++++++-----------------
>> net/nfc/llcp_sock.c | 49 +++++++++++++-----
>> 3 files changed, 115 insertions(+), 58 deletions(-)
>>
>> diff --git a/net/nfc/llcp.h b/net/nfc/llcp.h
>> index d8345ed57c95..23ae7a0112d3 100644
>> --- a/net/nfc/llcp.h
>> +++ b/net/nfc/llcp.h
>> @@ -91,6 +91,7 @@ struct nfc_llcp_local {
>> struct hlist_head pending_sdreqs;
>> struct timer_list sdreq_timer;
>> struct work_struct sdreq_timeout_work;
>> + struct work_struct release_work;
>> u8 sdreq_next_tid;
>>
>> /* sockets array */
>> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
>> index bd6361e2efa4..23553e7426ec 100644
>> --- a/net/nfc/llcp_core.c
>> +++ b/net/nfc/llcp_core.c
>> @@ -20,6 +20,8 @@ static LIST_HEAD(llcp_devices);
>> /* Protects llcp_devices list */
>> static DEFINE_SPINLOCK(llcp_devices_lock);
>>
>> +static struct workqueue_struct *llcp_wq;
>> +
>> static void nfc_llcp_rx_skb(struct nfc_llcp_local *local, struct sk_buff *skb);
>>
>> void nfc_llcp_sock_link(struct llcp_sock_list *l, struct sock *sk)
>> @@ -63,21 +65,33 @@ static void nfc_llcp_socket_purge(struct nfc_llcp_sock *sock)
>> }
>> }
>>
>> +static struct sock *nfc_llcp_sock_list_pop(struct llcp_sock_list *l)
>> +{
>> + struct sock *sk;
>> +
>> + write_lock(&l->lock);
>> + sk = sk_head(&l->head);
>> + if (sk) {
>> + sock_hold(sk);
>> + sk_del_node_init(sk);
>> + }
>> + write_unlock(&l->lock);
>> +
>> + return sk;
>> +}
>> +
>> static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>> int err)
>> {
>> struct sock *sk;
>> - struct hlist_node *tmp;
>> struct nfc_llcp_sock *llcp_sock;
>>
>> skb_queue_purge(&local->tx_queue);
>>
>> - write_lock(&local->sockets.lock);
>> -
>> - 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);
>>
>> nfc_llcp_socket_purge(llcp_sock);
>>
>> @@ -91,17 +105,27 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>> list_for_each_entry_safe(lsk, n,
>> &llcp_sock->accept_queue,
>> accept_queue) {
>> + bool put_creation = false;
>> +
>> accept_sk = &lsk->sk;
>> - bh_lock_sock(accept_sk);
>> + lock_sock_nested(accept_sk,
>> + SINGLE_DEPTH_NESTING);
>>
>> - nfc_llcp_accept_unlink(accept_sk);
>> + if (nfc_llcp_sock(accept_sk)->parent == sk) {
>> + nfc_llcp_accept_unlink(accept_sk);
>> + nfc_llcp_sock_unlink(&local->sockets, accept_sk);
>>
>> - if (err)
>> - accept_sk->sk_err = err;
>> - accept_sk->sk_state = LLCP_CLOSED;
>> - accept_sk->sk_state_change(sk);
>> + if (err)
>> + accept_sk->sk_err = err;
>> + accept_sk->sk_state = LLCP_CLOSED;
>> + accept_sk->sk_state_change(accept_sk);
>> + sock_orphan(accept_sk);
>> + put_creation = true;
>> + }
>>
>> - bh_unlock_sock(accept_sk);
>> + release_sock(accept_sk);
>> + if (put_creation)
>> + sock_put(accept_sk); /* creation ref */
>> }
>> }
>>
>> @@ -110,23 +134,18 @@ 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->sockets.lock);
>> -
>> /* If we still have a device, we keep the RAW sockets alive */
>> if (device == true)
>> return;
>>
>> - write_lock(&local->raw_sockets.lock);
>> -
>> - sk_for_each_safe(sk, tmp, &local->raw_sockets.head) {
>> + while ((sk = nfc_llcp_sock_list_pop(&local->raw_sockets))) {
>> llcp_sock = nfc_llcp_sock(sk);
>>
>> - bh_lock_sock(sk);
>> + lock_sock(sk);
>>
>> nfc_llcp_socket_purge(llcp_sock);
>>
>> @@ -135,26 +154,20 @@ 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)
>> {
>> - /* Since using nfc_llcp_local may result in usage of nfc_dev, whenever
>> - * we hold a reference to local, we also need to hold a reference to
>> - * the device to avoid UAF.
>> - */
>> - if (!nfc_get_device(local->dev->idx))
>> + if (!local)
>> return NULL;
>>
>> - kref_get(&local->ref);
>> + if (kref_get_unless_zero(&local->ref))
>> + return local;
>>
>> - return local;
>> + return NULL;
>> }
>>
>> static void local_cleanup(struct nfc_llcp_local *local)
>> @@ -172,30 +185,34 @@ static void local_cleanup(struct nfc_llcp_local *local)
>> nfc_llcp_free_sdp_tlv_list(&local->pending_sdreqs);
>> }
>>
>> +static void local_release_work(struct work_struct *work)
>> +{
>> + struct nfc_llcp_local *local;
>> + struct nfc_dev *dev;
>> +
>> + local = container_of(work, struct nfc_llcp_local, release_work);
>> + dev = local->dev;
>> +
>> + local_cleanup(local);
>> + kfree(local);
>> + nfc_put_device(dev);
>> +}
>> +
>> static void local_release(struct kref *ref)
>> {
>> struct nfc_llcp_local *local;
>>
>> local = container_of(ref, struct nfc_llcp_local, ref);
>>
>> - local_cleanup(local);
>> - kfree(local);
>> + queue_work(llcp_wq, &local->release_work);
>> }
>>
>> int nfc_llcp_local_put(struct nfc_llcp_local *local)
>> {
>> - struct nfc_dev *dev;
>> - int ret;
>> -
>> - if (local == NULL)
>> + if (!local)
>> return 0;
>>
>> - dev = local->dev;
>> -
>> - ret = kref_put(&local->ref, local_release);
>> - nfc_put_device(dev);
>> -
>> - return ret;
>> + return kref_put(&local->ref, local_release);
>> }
>>
>> static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local,
>> @@ -1705,6 +1722,7 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
>> INIT_WORK(&local->rx_work, nfc_llcp_rx_work);
>>
>> INIT_WORK(&local->timeout_work, nfc_llcp_timeout_work);
>> + INIT_WORK(&local->release_work, local_release_work);
>>
>> rwlock_init(&local->sockets.lock);
>> rwlock_init(&local->connecting_sockets.lock);
>> @@ -1748,10 +1766,23 @@ void nfc_llcp_unregister_device(struct nfc_dev *dev)
>>
>> int __init nfc_llcp_init(void)
>> {
>> - return nfc_llcp_sock_init();
>> + int ret;
>> +
>> + llcp_wq = alloc_workqueue("nfc_llcp_wq", WQ_UNBOUND, 0);
>> + if (!llcp_wq)
>> + return -ENOMEM;
>> +
>> + ret = nfc_llcp_sock_init();
>> + if (ret) {
>> + destroy_workqueue(llcp_wq);
>> + return ret;
>> + }
>> +
>> + return 0;
>> }
>>
>> void nfc_llcp_exit(void)
>> {
>> nfc_llcp_sock_exit();
>> + destroy_workqueue(llcp_wq);
>> }
>> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
>> index 5558d8a4d48b..ce6875eb58fb 100644
>> --- a/net/nfc/llcp_sock.c
>> +++ b/net/nfc/llcp_sock.c
>> @@ -392,11 +392,12 @@ void nfc_llcp_accept_unlink(struct sock *sk)
>>
>> pr_debug("state %d\n", sk->sk_state);
>>
>> - list_del_init(&llcp_sock->accept_queue);
>> - sk_acceptq_removed(llcp_sock->parent);
>> - llcp_sock->parent = NULL;
>> -
>> - sock_put(sk);
>> + if (llcp_sock->parent) {
>> + list_del_init(&llcp_sock->accept_queue);
>> + sk_acceptq_removed(llcp_sock->parent);
>> + llcp_sock->parent = NULL;
>> + sock_put(sk);
>> + }
>> }
>>
>> void nfc_llcp_accept_enqueue(struct sock *parent, struct sock *sk)
>> @@ -423,12 +424,20 @@ struct sock *nfc_llcp_accept_dequeue(struct sock *parent,
>>
>> list_for_each_entry_safe(lsk, n, &llcp_parent->accept_queue,
>> accept_queue) {
>> + struct nfc_llcp_local *local;
>> +
>> sk = &lsk->sk;
>> - lock_sock(sk);
>> + lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>>
>> if (sk->sk_state == LLCP_CLOSED) {
>> - release_sock(sk);
>> + local = nfc_llcp_sock(sk)->local;
>> +
>> nfc_llcp_accept_unlink(sk);
>> + if (local)
>> + nfc_llcp_sock_unlink(&local->sockets, sk);
>> + sock_orphan(sk);
>> + release_sock(sk);
>> + sock_put(sk);
>> continue;
>> }
>>
>> @@ -464,7 +473,7 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
>>
>> pr_debug("parent %p\n", sk);
>>
>> - lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>> + lock_sock(sk);
>>
>> if (sk->sk_state != LLCP_LISTEN) {
>> ret = -EBADFD;
>> @@ -490,7 +499,12 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
>>
>> release_sock(sk);
>> timeo = schedule_timeout(timeo);
>> - lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>> + lock_sock(sk);
>> +
>> + if (sk->sk_state != LLCP_LISTEN) {
>> + ret = -EBADFD;
>> + break;
>> + }
>> }
>> __set_current_state(TASK_RUNNING);
>> remove_wait_queue(sk_sleep(sk), &wait);
>> @@ -629,13 +643,24 @@ static int llcp_sock_release(struct socket *sock)
>>
>> list_for_each_entry_safe(lsk, n, &llcp_sock->accept_queue,
>> accept_queue) {
>> + bool put_creation = false;
>> +
>> 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);
>> + if (nfc_llcp_sock(accept_sk)->parent == sk) {
>> + nfc_llcp_send_disconnect(lsk);
>> + nfc_llcp_accept_unlink(accept_sk);
>> + nfc_llcp_sock_unlink(&local->sockets, accept_sk);
>> +
>> + accept_sk->sk_state = LLCP_CLOSED;
>> + sock_orphan(accept_sk);
>> + put_creation = true;
>> + }
>>
>> release_sock(accept_sk);
>> + if (put_creation)
>> + sock_put(accept_sk); /* creation ref */
>> }
>> }
>>
>> --
>> 2.55.0.966.g6673acef38-goog
>>
>
prev parent reply other threads:[~2026-09-08 10:29 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-02 12:30 [PATCH v3 1/1] nfc: llcp: Fix race condition in accept_queue lifecycle Lee Jones
2026-09-08 8:06 ` Lee Jones
2026-09-08 10:29 ` David Heidelberg [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6beddcea-bc04-471d-be5d-8e391cdedee3@ixit.cz \
--to=david@ixit.cz \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=horms@kernel.org \
--cc=kuba@kernel.org \
--cc=lee@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=oe-linux-nfc@lists.linux.dev \
--cc=pabeni@redhat.com \
--cc=sameo@linux.intel.com \
--cc=szymon.janc@tieto.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).