Re: [PATCH net v7 1/1] llc: fix listener child socket leaks before passive open completes
From: Simon Horman
Date: Sun Sep 06 2026 - 08:50:25 EST
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://netdev-ai.bots.linux.dev/sashiko/#/patchset/0b8964c9cd294898c9657fd34c9d04cec77782eb.1788414881.git.zihanx@xxxxxxxxxx?part=1
---
llc: fix listener child socket leaks before passive open completes
This restricts passive-open child creation to SABME commands, answers other
listener-directed commands with a DM response generated from the packet
source address, and adds an incoming_state / incoming_children lifecycle so
children that never reach accept() are released on drop, backlog failure and
listener close. llc_sk_free() gains a sync argument and the final child
teardown moves to a work item.
Some questions below about the new locking and reference accounting.
> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> index b0447c33dbf0..e8054809cf0c 100644
> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
[ ... ]
> @@ -722,6 +729,17 @@ static int llc_ui_accept(struct socket *sock, struct socket *newsock,
> goto frees;
> rc = 0;
> newsk = skb->sk;
> + lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
> + if (llc_sk(newsk)->state < LLC_CONN_STATE_ADM ||
> + !llc_accept_incoming_sock(newsk)) {
> + if (atomic_read(&llc_sk(newsk)->incoming_state) !=
> + LLC_INCOMING_NONE)
> + llc_release_incoming_sock(newsk);
> + release_sock(newsk);
> + sock_put(newsk);
> + rc = -ECONNABORTED;
> + goto frees;
> + }
[Severity: High]
Can this lock_sock_nested() block on the lock_sock(sk) that llc_ui_accept()
already holds? There is no test that skb->sk is a distinct child.
A connection indication can end up on a socket's own receive queue with
skb->sk == sk. After bind(), llc->daddr is all zero, so a SABME whose
h_source is 00:00:00:00:00:00 and whose SSAP masks to 0 matches
llc_estab_match():
net/llc/llc_conn.c:llc_estab_match() {
...
llc->daddr.lsap == daddr->lsap &&
ether_addr_equal(llc->laddr.mac, laddr->mac) &&
ether_addr_equal(llc->daddr.mac, daddr->mac);
}
__llc_lookup_established() returns that socket, so llc_conn_handler() takes
the non-listener branch and sets skb->sk = sk.
llc_adm_state_trans_2 (action llc_conn_ac_conn_ind) then sets
ind_prim = LLC_CONN_PRIM, and llc_conn_state_process() queues the skb on
sk->sk_receive_queue with skb->sk == sk. llc_ui_listen() does not drain
sk_receive_queue, so the socket can enter TCP_LISTEN with that skb queued.
accept() dequeues it, the !skb->sk test does not fire, newsk == sk, and
lock_sock_nested() waits in __lock_sock() for a lock this task owns.
For such an skb the conditional hold added in llc_conn_state_process() was
never taken (sk != skb->sk is false there), so is the sock_put(newsk) on the
abort path, and on the success path below, also unbalanced?
> /* attach connection to a new socket. */
> llc_ui_sk_init(newsock, newsk);
> sock_reset_flag(newsk, SOCK_ZAPPED);
> @@ -737,6 +755,8 @@ static int llc_ui_accept(struct socket *sock, struct socket *newsock,
> sk_acceptq_removed(sk);
> dprintk("%s: ok success on %02X, client on %02X\n", __func__,
> llc_sk(sk)->addr.sllc_sap, newllc->daddr.lsap);
> + release_sock(newsk);
> + sock_put(newsk);
> frees:
> kfree_skb(skb);
> out:
> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 260460d50f54..092625e28b3b 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
[ ... ]
> @@ -88,6 +89,13 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)
> * skb->sk pointing to the newly created struct sock in
> * llc_conn_handler. -acme
> */
> + if (sk != skb->sk &&
> + atomic_read(&llc_sk(skb->sk)->incoming_state) ==
> + LLC_INCOMING_PENDING) {
> + sock_hold(skb->sk);
> + atomic_set(&llc_sk(skb->sk)->incoming_state,
> + LLC_INCOMING_QUEUED);
> + }
> skb_get(skb);
> skb_queue_tail(&sk->sk_receive_queue, skb);
> sk->sk_state_change(sk);
[ ... ]
> @@ -765,27 +773,196 @@ static struct sock *llc_create_incoming_sock(struct sock *sk,
[ ... ]
> +static void llc_incoming_sock_work(struct work_struct *work)
> +{
> + struct llc_sock *llc = container_of(work, struct llc_sock,
> + incoming_work);
> + struct sock *listener = llc->incoming_listener;
> + struct sock *sk = &llc->sk;
> +
> + lock_sock(sk);
> + llc_sk_stop_all_timers(sk, false);
> + sock_orphan(sk);
> + release_sock(sk);
> + llc_sk_stop_all_timers(sk, true);
> + dev_put(llc->dev);
> + llc->dev = NULL;
> + llc_sk_free(sk, false);
> + sock_put(sk);
> + sock_put(listener);
> +}
> +
> +void llc_release_incoming_sock(struct sock *sk)
> +{
> + struct llc_sock *llc = llc_sk(sk);
> +
> + if (atomic_xchg(&llc->incoming_state, LLC_INCOMING_NONE) ==
> + LLC_INCOMING_NONE)
> + return;
> +
> + WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
> + spin_lock_bh(&llc->sap->sk_lock);
> + list_del_init(&llc->incoming_node);
> + spin_unlock_bh(&llc->sap->sk_lock);
> + sock_hold(sk);
> + llc_sap_remove_socket(llc->sap, sk);
> + schedule_work(&llc->incoming_work);
> +}
> +
> +bool llc_accept_incoming_sock(struct sock *sk)
> +{
> + struct llc_sock *llc = llc_sk(sk);
> +
> + if (atomic_cmpxchg(&llc->incoming_state, LLC_INCOMING_QUEUED,
> + LLC_INCOMING_NONE) != LLC_INCOMING_QUEUED)
> + return false;
> +
> + spin_lock_bh(&llc->sap->sk_lock);
> + list_del_init(&llc->incoming_node);
> + spin_unlock_bh(&llc->sap->sk_lock);
> + sock_put(llc->incoming_listener);
> + return true;
> +}
> +
> +void llc_release_incoming_children(struct sock *sk)
> +{
> + struct llc_sock *llc = llc_sk(sk);
> + struct sk_buff *skb;
> +
> + local_bh_disable();
> + while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
> + struct sock *newsk = skb->sk;
> +
> + if (newsk && newsk != sk) {
> + int incoming_state;
> +
> + bh_lock_sock_nested(newsk);
> + incoming_state =
> + atomic_read(&llc_sk(newsk)->incoming_state);
> + if (incoming_state != LLC_INCOMING_NONE) {
> + llc_release_incoming_sock(newsk);
> + if (incoming_state == LLC_INCOMING_QUEUED)
> + sock_put(newsk);
> + }
> + bh_unlock_sock(newsk);
> + }
> + kfree_skb(skb);
> + }
> + if (llc->sap) {
> + spin_lock(&llc->sap->sk_lock);
> + while (!list_empty(&llc->incoming_children)) {
> + struct llc_sock *child;
> + struct sock *newsk;
> +
> + child = list_first_entry(&llc->incoming_children,
> + struct llc_sock,
> + incoming_node);
> + list_del_init(&child->incoming_node);
> + newsk = &child->sk;
> + sock_hold(newsk);
> + spin_unlock(&llc->sap->sk_lock);
> +
> + bh_lock_sock_nested(newsk);
> + if (atomic_read(&child->incoming_state) !=
> + LLC_INCOMING_NONE)
> + llc_release_incoming_sock(newsk);
> + bh_unlock_sock(newsk);
> + sock_put(newsk);
> + spin_lock(&llc->sap->sk_lock);
> + }
> + spin_unlock(&llc->sap->sk_lock);
> + }
> + local_bh_enable();
> +}
[Severity: High]
The receive-queue loop above drops the accept-queue reference explicitly:
if (incoming_state != LLC_INCOMING_NONE) {
llc_release_incoming_sock(newsk);
if (incoming_state == LLC_INCOMING_QUEUED)
sock_put(newsk);
}
The incoming_children walk releases children whose incoming_state is
!= LLC_INCOMING_NONE, which includes LLC_INCOMING_QUEUED, but the only
sock_put() there balances the walk's own sock_hold().
Counting the references of a QUEUED child: llc_release_incoming_sock() adds
one hold, and llc_incoming_sock_work() drops two (llc_sk_free()'s sock_put()
plus its own), so the hold taken in llc_conn_state_process() has to be
dropped by the caller. Does a QUEUED child that reaches this walk leak its
struct llc_sock?
A path that gets a QUEUED child onto this walk:
llc_ui_connect() has no TCP_LISTEN guard, so the listener can be moved to
TCP_SYN_SENT/TCP_CLOSE. llc_ui_recvmsg() only refuses sk_state ==
TCP_LISTEN, so after that it treats the pending connection indication as
stream data:
net/llc/af_llc.c:llc_ui_recvmsg() {
...
skb_unlink(skb, &sk->sk_receive_queue);
kfree_skb(skb);
}
That drops only the skb reference; the child stays QUEUED on
incoming_children, and close() then reaches the walk above.
[ ... ]
> @@ -813,18 +1010,49 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
> skb->sk = sk;
> skb->destructor = sock_efree;
> }
> - if (!sock_owned_by_user(sk))
> + if (unlikely(llc_sk(skb->sk)->state < LLC_CONN_STATE_ADM)) {
> + if (newsk) {
> + if (atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING)
> + llc_release_incoming_sock(newsk);
> + } else if (atomic_read(&llc_sk(sk)->incoming_state) ==
> + LLC_INCOMING_PENDING) {
> + llc_release_incoming_sock(sk);
> + }
> + goto drop_unlock;
> + }
[Severity: Medium]
This out-of-service lower bound is not described anywhere in the commit
message body, and the v7 changelog says the work was removed from the
series:
"Drop the companion LLC_CONN_OUT_OF_SVC bounds patch due to overlap with
Kees Cook's net-next series"
"That series also covers ... rejecting out-of-service states before table
lookup"
Five copies of the check are still here though: this one in
llc_conn_handler(), three in llc_backlog_rcv(), and one in llc_ui_accept().
In the non-listener branch just above, skb->sk is the ordinary looked-up
socket, so this changes behaviour for every socket on the direct receive
path, and in llc_ui_accept() it converts an otherwise successful accept()
into -ECONNABORTED. Should the message describe the new semantics, or
should these checks be dropped as the changelog states?
> if (!sock_owned_by_user(sk)) {
> llc_conn_rcv(sk, skb);
> - else {
> + if (newsk &&
> + atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING)
> + llc_release_incoming_sock(newsk);
> + } else {
> dprintk("%s: adding to backlog...\n", __func__);
> llc_set_backlog_type(skb, LLC_PACKET);
> - if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf)))
> + if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf))) {
> + if (newsk && !newsk_lookup_ref)
> + llc_release_incoming_sock(newsk);
> goto drop_unlock;
> + }
> }
[ ... ]
> @@ -852,12 +1080,52 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> {
> int rc = 0;
> struct llc_sock *llc = llc_sk(sk);
> + struct sock *newsk = skb->sk;
>
> if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
> - if (likely(llc->state > 1)) /* not closed */
> + if (newsk &&
> + atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING) {
> + local_bh_disable();
> + bh_lock_sock_nested(newsk);
> + if (atomic_read(&llc_sk(newsk)->incoming_state) !=
> + LLC_INCOMING_PENDING) {
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + goto retry;
> + }
> + if (sock_flag(sk, SOCK_DEAD) ||
> + sk->sk_state != TCP_LISTEN ||
> + llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
> + llc_release_incoming_sock(newsk);
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + goto out_kfree_skb;
> + }
> rc = llc_conn_rcv(sk, skb);
> - else
> + if (atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING)
> + llc_release_incoming_sock(newsk);
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + } else if (newsk &&
> + atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_QUEUED) {
> + local_bh_disable();
> + bh_lock_sock_nested(newsk);
> + if (llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
[Severity: Medium]
Two different "socket is usable" boundaries now live in this one function.
LLC_CONN_OUT_OF_SVC is 0 and LLC_CONN_STATE_ADM is 1, so the new checks
dispatch a socket in ADM while the legacy branch a few lines below,
llc->state > 1 /* not closed */, drops the same frame as closed. Which
boundary is intended for a child in ADM?
The same permissive bound is reused in llc_ui_accept(). A peer DISC returns
a connected child to ADM:
net/llc/llc_c_st.c:
static const struct llc_conn_state_trans llc_common_state_trans_4 = {
.ev = llc_conn_ev_rx_disc_cmd_pbit_set_x,
.next_state = LLC_CONN_STATE_ADM,
The LLC_DISC_PRIM handling in llc_conn_state_process() is gated on
sk_state == TCP_ESTABLISHED, which a pre-accept child is not, so the queued
indication stays in the accept queue. accept() then passes
state >= LLC_CONN_STATE_ADM and sets newsk->sk_state = TCP_ESTABLISHED and
newsock->state = SS_CONNECTED for a connection that is already down.
Should the boundary instead be the data-capable states used by
llc_data_accept_state(), that is NORMAL/BUSY/REJ?
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + goto out_kfree_skb;
> + }
> + rc = llc_conn_rcv(newsk, skb);
[Severity: Medium]
Here sk and skb->sk are both the unaccepted child, but the LLC_CONN_PRIM
branch of llc_conn_state_process() assumes sk is the listener owning the
accept queue and skb->sk is the child, which is what the new
sk != skb->sk bookkeeping encodes.
With three frames queued on the listener backlog while the listener is
user-owned:
f1 SABME -> PENDING branch -> LLC_CONN_PRIM -> child becomes QUEUED
f2 DISC -> QUEUED branch -> child returns to LLC_CONN_STATE_ADM
f3 SABME -> QUEUED branch -> llc_conn_rcv(child, f3)
For f3, llc_adm_state_trans_2 / llc_conn_ac_conn_ind sets
ind_prim = LLC_CONN_PRIM, and since sk == skb->sk the indication is queued
on the child's own sk_receive_queue with no sock_hold() and no state
transition.
Does that lose the indication for accept(), and leave a raw SABME frame on
the child's receive queue where llc_ui_recvmsg() would later hand its bytes
to userspace as stream data?
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + } else if (likely(llc->state > 1)) {
> + rc = llc_conn_rcv(sk, skb);
[Severity: High]
This fallback branch checks the listener's llc->state, but llc_conn_rcv()
runs the state machine on skb->sk, which can be a child that has already
been put out of service:
net/llc/llc_conn.c:llc_release_incoming_sock() {
WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
...
schedule_work(&llc->incoming_work);
}
After that the child's incoming_state is LLC_INCOMING_NONE, so neither new
branch above matches and its backlogged skbs land here, with no child lock
held. llc_ui_release() releases the children before the backlog is drained:
net/llc/af_llc.c:llc_ui_release() {
llc_sap_remove_socket(llc->sap, sk);
llc_release_incoming_children(sk);
release_sock(sk);
}
llc_conn_service() only rejects llc->state > NBR_CONN_STATES, so state 0
reaches:
net/llc/llc_conn.c:llc_qualify_conn_ev() {
struct llc_conn_state *curr_state =
&llc_conn_state_table[llc->state - 1];
Can this index llc_conn_state_table[] and llc_offset_table[][] at -1 and
then dereference the resulting curr_state->transitions?
Reaching this branch needs the listener's own llc->state to be > 1, which a
bound socket that received a matching SABME before listen() satisfies, since
llc_ui_listen() does not reset llc->state.
Separately, is running llc_conn_rcv() on that child safe against
llc_incoming_sock_work(), which concurrently does sock_orphan(),
dev_put(llc->dev), llc->dev = NULL and llc_sk_free(sk, false) with no
serialization against this path?
> + } else {
> goto out_kfree_skb;
> + }
> } else if (llc_backlog_type(skb) == LLC_EVENT) {
> /* timer expiration event */
> if (likely(llc->state > 1)) /* not closed */
[ ... ]