[PATCH net v6 1/2] llc: fix listener child socket leaks before passive open completes

[PATCH net v6 1/2] llc: fix listener child socket leaks before passive open completes

[Zihan Xi]

llc_conn_handler() creates and publishes a child whenever a listener
matches a packet. A non-SABME frame never completes the passive open, so
the child remains in the SAP tables, keeps its device reference, and
cannot be returned by accept().

Create children only for SABME commands. Handle the listener's required
DM replies directly, using the packet source address, and do not run the
listener through the connection state machine.

Keep SABME children in the SAP tables during the passive open so that
established lookup continues to select them. Track children until the
connection indication is queued for accept(), and release any child that
fails before then, including direct and backlog failures and listener
close. Reject redirected packets after the listener leaves TCP_LISTEN.

The child socket lock is acquired with bottom halves disabled whenever
the cleanup or backlog path runs in process context.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: Codex:gpt-5.4
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
changes in v6:
  - Hold a reference for children queued for accept() and release it when they
    are dequeued, while retaining SAP publication so tuple lookup still finds
    a pending child before the passive open completes.
  - Make direct receive, backlog, accept-queue, and listener-close cleanup
    symmetric, with bottom-half-disabled child locking in process context.
  - Keep the LLC_CONN_OUT_OF_SVC lower-bound check in its separate patch and
    use the ADM state boundary consistently.
  - v5 Link: https://lore.kernel.org/all/20260822082354.3109-1-zihanx@nebusec.ai/
changes in v5:
  - Make listener child cleanup unconditional so queued children are also
    released if the socket leaves TCP_LISTEN before close.
  - Serialize process-context child cleanup and backlog dispatch with bottom
    halves disabled, avoiding child-lock acquisition races with LLC receive
    and timer paths.
  - Drop packets redirected through a pending child after its listener is no
    longer listening, and release children left out of service instead of
    dispatching them.
  - Split the LLC_CONN_OUT_OF_SVC lower-bound check into a separate patch.
  - v4 Link: https://lore.kernel.org/all/20260814185843.4748-1-zihanx@nebusec.ai/
changes in v4:
  - Create a passive-open child only for SABME and generate listener-side DM
    replies directly for non-SABME commands.
  - Use an atomic incoming-child lifecycle and serialize pending-child lookup,
    backlog processing, rollback, and listener close with the child lock.
  - Keep immediate SAP publication for passive-open tuple matching, but release
    unaccepted children on direct and backlog failures and on listener close.
  - Defer final incoming-child cleanup to workqueue context so timer
    synchronization does not run in the receive softirq path.
  - Add an LLC state lower-bound check before state-table dispatch.
  - v3 Link: https://lore.kernel.org/all/20260805175945.10698-1-zihanx@nebusec.ai/
changes in v3:
  - Drop the unused llc_conn_handler() local rc variable reported in review.
  - Rebase the numbered patch and cover onto commit
    ede76849012e45ffb2193ad110b42027eec02c5c.
  - v2 Link: https://lore.kernel.org/all/cover.1785386749.git.zihanx@nebusec.ai/
changes in v2:
  - Rework the fix to preserve the existing passive-open tuple matching
    semantics instead of deferring child publication until LLC_CONN_PRIM.
  - Track listener-created children pending publication to accept(), and roll
    them back on every earlier failure or drop path.
  - Cover the original non-SABME leak and SABME paths which fail before
    LLC_CONN_PRIM, including backlog enqueue and backlog drop failures.
  - Correct Fixes to 1da177e4c3f4 ("Linux-2.6.12-rc2") based on the earliest
    locally visible history carrying the same root-cause fact.
  - Clarify panic_on_oom crash evidence and packetdrill selection.
  - v1 Link: https://lore.kernel.org/all/cover.1784725007.git.zihanx@nebusec.ai/
 include/net/llc_conn.h |  13 +-
 net/llc/af_llc.c       |  22 +++-
 net/llc/llc_conn.c     | 271 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 292 insertions(+), 14 deletions(-)

diff --git a/include/net/llc_conn.h b/include/net/llc_conn.h
index e1a302696723..e8003db110ad 100644
--- a/include/net/llc_conn.h
+++ b/include/net/llc_conn.h
@@ -6,6 +6,7 @@
  * 		 2001, 2002 by Arnaldo Carvalho de Melo <acme@conectiva.com.br>
  */
 #include <linux/timer.h>
+#include <linux/workqueue.h>
 #include <net/llc_if.h>
 #include <net/sock.h>
 #include <linux/llc.h>
@@ -13,6 +14,10 @@
 #define LLC_EVENT                1
 #define LLC_PACKET               2
 
+#define LLC_INCOMING_NONE        0
+#define LLC_INCOMING_PENDING     1
+#define LLC_INCOMING_QUEUED      2
+
 #define LLC2_P_TIME               2
 #define LLC2_ACK_TIME             1
 #define LLC2_REJ_TIME             3
@@ -72,6 +77,9 @@ struct llc_sock {
 					      received and caused sending FRMR.
 					      Used for resending FRMR */
 	u32		    cmsg_flags;
+	atomic_t	    incoming_state;
+	struct sock	    *incoming_listener;
+	struct work_struct incoming_work;
 	struct hlist_node   dev_hash_node;
 };
 
@@ -93,7 +101,10 @@ static __inline__ char llc_backlog_type(struct sk_buff *skb)
 struct sock *llc_sk_alloc(struct net *net, int family, gfp_t priority,
 			  struct proto *prot, int kern);
 void llc_sk_stop_all_timers(struct sock *sk, bool sync);
-void llc_sk_free(struct sock *sk);
+void llc_sk_free(struct sock *sk, bool sync);
+void llc_release_incoming_sock(struct sock *sk);
+bool llc_accept_incoming_sock(struct sock *sk);
+void llc_release_incoming_children(struct sock *sk);
 
 void llc_sk_reset(struct sock *sk);
 
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
@@ -27,6 +27,7 @@
 #include <net/llc_sap.h>
 #include <net/llc_pdu.h>
 #include <net/llc_conn.h>
+#include <net/llc_c_st.h>
 #include <net/tcp_states.h>
 
 /* remember: uninitialized global data is zeroed because its in .bss */
@@ -196,6 +197,7 @@ static int llc_ui_release(struct socket *sock)
 {
 	struct sock *sk = sock->sk;
 	struct llc_sock *llc;
+	bool listener;
 
 	if (unlikely(sk == NULL))
 		goto out;
@@ -206,6 +208,9 @@ static int llc_ui_release(struct socket *sock)
 		llc->laddr.lsap, llc->daddr.lsap);
 	if (!llc_send_disc(sk))
 		llc_ui_wait_for_disc(sk, READ_ONCE(sk->sk_rcvtimeo));
+	listener = sk->sk_state == TCP_LISTEN;
+	if (listener)
+		sock_set_flag(sk, SOCK_DEAD);
 	if (!sock_flag(sk, SOCK_ZAPPED)) {
 		struct llc_sap *sap = llc->sap;
 
@@ -214,16 +219,18 @@ static int llc_ui_release(struct socket *sock)
 		 */
 		llc_sap_hold(sap);
 		llc_sap_remove_socket(llc->sap, sk);
+		llc_release_incoming_children(sk);
 		release_sock(sk);
 		llc_sap_put(sap);
 	} else {
+		llc_release_incoming_children(sk);
 		release_sock(sk);
 	}
 	netdev_put(llc->dev, &llc->dev_tracker);
 	sock_put(sk);
 	sock_orphan(sk);
 	sock->sk = NULL;
-	llc_sk_free(sk);
+	llc_sk_free(sk, true);
 out:
 	return 0;
 }
@@ -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;
+	}
 	/* 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..885a5c33024c 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -32,6 +32,7 @@ static int llc_exec_conn_trans_actions(struct sock *sk,
 				       struct sk_buff *ev);
 static const struct llc_conn_state_trans *llc_qualify_conn_ev(struct sock *sk,
 							      struct sk_buff *skb);
+static void llc_incoming_sock_work(struct work_struct *work);
 
 /* Offset table on connection states transition diagram */
 static int llc_offset_table[NBR_CONN_STATES][NBR_CONN_EV];
@@ -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,162 @@ static struct sock *llc_create_incoming_sock(struct sock *sk,
 	memcpy(&newllc->laddr, daddr, sizeof(newllc->laddr));
 	memcpy(&newllc->daddr, saddr, sizeof(newllc->daddr));
 	newllc->dev = dev;
+	newllc->incoming_listener = sk;
+	atomic_set(&newllc->incoming_state, LLC_INCOMING_PENDING);
+	INIT_WORK(&newllc->incoming_work, llc_incoming_sock_work);
+	sock_hold(sk);
 	dev_hold(dev);
 	llc_sap_add_socket(llc->sap, newsk);
 out:
 	return newsk;
 }
 
+static void llc_incoming_sock_work(struct work_struct *work)
+{
+	struct llc_sock *llc = container_of(work, struct llc_sock,
+						    incoming_work);
+	struct sock *sk = &llc->sk;
+	struct sock *listener = llc->incoming_listener;
+
+	lock_sock(listener);
+	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+	llc_sk_free(sk, false);
+	sock_orphan(sk);
+	release_sock(sk);
+	llc_sk_stop_all_timers(sk, true);
+	release_sock(listener);
+	dev_put(llc->dev);
+	llc->dev = NULL;
+	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);
+	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;
+
+	sock_put(llc->incoming_listener);
+	return true;
+}
+
+void llc_release_incoming_children(struct sock *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);
+	}
+	local_bh_enable();
+}
+
+/*
+ * This mirrors the ADM-state DM actions, but a listener has no peer
+ * address in llc->daddr yet.
+ */
+static void llc_conn_send_dm_rsp(struct llc_sap *sap, struct sk_buff *skb,
+				 struct llc_addr *saddr, u8 f_bit)
+{
+	struct sk_buff *nskb;
+	int rc;
+
+	nskb = llc_alloc_frame(NULL, skb->dev, LLC_PDU_TYPE_U, 0);
+	if (!nskb)
+		return;
+
+	llc_pdu_header_init(nskb, LLC_PDU_TYPE_U, sap->laddr.lsap,
+			    saddr->lsap, LLC_PDU_RSP);
+	llc_pdu_init_as_dm_rsp(nskb, f_bit);
+	rc = llc_mac_hdr_init(nskb, skb->dev->dev_addr, saddr->mac);
+	if (unlikely(rc))
+		kfree_skb(nskb);
+	else
+		dev_queue_xmit(nskb);
+}
+
 void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
 {
 	struct llc_addr saddr, daddr;
-	struct sock *sk;
+	struct sock *sk, *newsk = NULL;
+	bool newsk_lookup_ref = false;
+	bool newsk_locked = false;
 
 	llc_pdu_decode_sa(skb, saddr.mac);
 	llc_pdu_decode_ssap(skb, &saddr.lsap);
 	llc_pdu_decode_da(skb, daddr.mac);
 	llc_pdu_decode_dsap(skb, &daddr.lsap);
 
+lookup:
 	sk = __llc_lookup(sap, &saddr, &daddr, dev_net(skb->dev));
 	if (!sk)
 		goto drop;
 
+	if (atomic_read(&llc_sk(sk)->incoming_state) ==
+	    LLC_INCOMING_PENDING) {
+		newsk = sk;
+		bh_lock_sock(newsk);
+		if (atomic_read(&llc_sk(newsk)->incoming_state) !=
+		    LLC_INCOMING_PENDING) {
+			bh_unlock_sock(newsk);
+			sock_put(newsk);
+			newsk = NULL;
+			goto lookup;
+		}
+		sk = llc_sk(newsk)->incoming_listener;
+		sock_hold(sk);
+		newsk_lookup_ref = true;
+		bh_unlock_sock(newsk);
+	}
+
 	bh_lock_sock(sk);
+	if (unlikely(sk->sk_state == TCP_LISTEN &&
+		     sock_flag(sk, SOCK_DEAD)))
+		goto drop_unlock;
+	if (newsk_lookup_ref) {
+		bh_lock_sock_nested(newsk);
+		newsk_locked = true;
+		if (atomic_read(&llc_sk(newsk)->incoming_state) !=
+		    LLC_INCOMING_PENDING)
+			goto retry_unlock;
+		if (unlikely(sk->sk_state != TCP_LISTEN ||
+			     sock_flag(sk, SOCK_DEAD))) {
+			llc_release_incoming_sock(newsk);
+			goto drop_unlock;
+		}
+	}
 	/*
 	 * This has to be done here and not at the upper layer ->accept
 	 * method because of the way the PROCOM state machine works:
@@ -795,10 +938,25 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
 	 * in the newly created struct sock private area. -acme
 	 */
 	if (unlikely(sk->sk_state == TCP_LISTEN)) {
-		struct sock *newsk = llc_create_incoming_sock(sk, skb->dev,
-							      &saddr, &daddr);
-		if (!newsk)
-			goto drop_unlock;
+		if (!newsk) {
+			if (llc_conn_ev_rx_sabme_cmd_pbit_set_x(sk, skb)) {
+				if (!llc_conn_ev_rx_disc_cmd_pbit_set_x(sk, skb)) {
+					u8 f_bit;
+
+					llc_pdu_decode_pf_bit(skb, &f_bit);
+					llc_conn_send_dm_rsp(sap, skb, &saddr, f_bit);
+				} else if (!llc_conn_ev_rx_xxx_cmd_pbit_set_1(sk, skb)) {
+					llc_conn_send_dm_rsp(sap, skb, &saddr, 1);
+				}
+				goto drop_unlock;
+			}
+			newsk = llc_create_incoming_sock(sk, skb->dev, &saddr,
+							 &daddr);
+			if (!newsk)
+				goto drop_unlock;
+			bh_lock_sock_nested(newsk);
+			newsk_locked = true;
+		}
 		skb_set_owner_r(skb, newsk);
 	} else {
 		/*
@@ -813,18 +971,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;
+	}
+	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)
+				llc_release_incoming_sock(newsk);
 			goto drop_unlock;
+		}
 	}
 out:
+	if (newsk_locked)
+		bh_unlock_sock(newsk);
 	bh_unlock_sock(sk);
 	sock_put(sk);
+	if (newsk_lookup_ref)
+		sock_put(newsk);
 	return;
+retry_unlock:
+	bh_unlock_sock(newsk);
+	newsk_locked = false;
+	bh_unlock_sock(sk);
+	sock_put(sk);
+	sock_put(newsk);
+	newsk = NULL;
+	newsk_lookup_ref = false;
+	goto lookup;
 drop:
 	kfree_skb(skb);
 	return;
@@ -852,12 +1041,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) {
+				bh_unlock_sock(newsk);
+				local_bh_enable();
+				goto out_kfree_skb;
+			}
+			rc = llc_conn_rcv(newsk, skb);
+			bh_unlock_sock(newsk);
+			local_bh_enable();
+		} else if (likely(llc->state > 1)) {
+			rc = llc_conn_rcv(sk, skb);
+		} else {
 			goto out_kfree_skb;
+		}
 	} else if (llc_backlog_type(skb) == LLC_EVENT) {
 		/* timer expiration event */
 		if (likely(llc->state > 1))  /* not closed */
@@ -870,6 +1099,23 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 	}
 out:
 	return rc;
+retry:
+	if (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)
+			rc = llc_conn_rcv(newsk, skb);
+		else {
+			bh_unlock_sock(newsk);
+			local_bh_enable();
+			goto out_kfree_skb;
+		}
+		bh_unlock_sock(newsk);
+		local_bh_enable();
+		goto out;
+	}
+	goto out_kfree_skb;
 out_kfree_skb:
 	kfree_skb(skb);
 	goto out;
@@ -960,16 +1206,17 @@ void llc_sk_stop_all_timers(struct sock *sk, bool sync)
 /**
  *	llc_sk_free - Frees a LLC socket
  *	@sk: - socket to free
+ *	@sync: whether to synchronously stop timers
  *
  *	Frees a LLC socket
  */
-void llc_sk_free(struct sock *sk)
+void llc_sk_free(struct sock *sk, bool sync)
 {
 	struct llc_sock *llc = llc_sk(sk);
 
 	llc->state = LLC_CONN_OUT_OF_SVC;
 	/* Stop all (possibly) running timers */
-	llc_sk_stop_all_timers(sk, true);
+	llc_sk_stop_all_timers(sk, sync);
 #ifdef DEBUG_LLC_CONN_ALLOC
 	printk(KERN_INFO "%s: unackq=%d, txq=%d\n", __func__,
 		skb_queue_len(&llc->pdu_unack_q),
-- 
2.43.0

[PATCH net v6 2/2] llc: reject out-of-service state before state lookup

[Zihan Xi]

llc_conn_service() checks only the upper bound of the connection state
before llc_qualify_conn_ev() indexes the state table. A socket in
LLC_CONN_OUT_OF_SVC therefore reaches llc_conn_state_table[state - 1]
with a negative index and can read and call data outside the table.

Reject states below LLC_CONN_STATE_ADM before the state-table lookup.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: Codex:gpt-5.4
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
changes in v6:
  - Hold a reference for children queued for accept() and release it when they
    are dequeued, while retaining SAP publication so tuple lookup still finds
    a pending child before the passive open completes.
  - Make direct receive, backlog, accept-queue, and listener-close cleanup
    symmetric, with bottom-half-disabled child locking in process context.
  - Keep the LLC_CONN_OUT_OF_SVC lower-bound check in its separate patch and
    use the ADM state boundary consistently.
  - v5 Link: https://lore.kernel.org/all/20260822082354.3109-1-zihanx@nebusec.ai/
changes in v5:
  - Make listener child cleanup unconditional so queued children are also
    released if the socket leaves TCP_LISTEN before close.
  - Serialize process-context child cleanup and backlog dispatch with bottom
    halves disabled, avoiding child-lock acquisition races with LLC receive
    and timer paths.
  - Drop packets redirected through a pending child after its listener is no
    longer listening, and release children left out of service instead of
    dispatching them.
  - Split the LLC_CONN_OUT_OF_SVC lower-bound check into a separate patch.
  - v4 Link: https://lore.kernel.org/all/20260814185843.4748-1-zihanx@nebusec.ai/
changes in v4:
  - Create a passive-open child only for SABME and generate listener-side DM
    replies directly for non-SABME commands.
  - Use an atomic incoming-child lifecycle and serialize pending-child lookup,
    backlog processing, rollback, and listener close with the child lock.
  - Keep immediate SAP publication for passive-open tuple matching, but release
    unaccepted children on direct and backlog failures and on listener close.
  - Defer final incoming-child cleanup to workqueue context so timer
    synchronization does not run in the receive softirq path.
  - Add an LLC state lower-bound check before state-table dispatch.
  - v3 Link: https://lore.kernel.org/all/20260805175945.10698-1-zihanx@nebusec.ai/
changes in v3:
  - Drop the unused llc_conn_handler() local rc variable reported in review.
  - Rebase the numbered patch and cover onto commit
    ede76849012e45ffb2193ad110b42027eec02c5c.
  - v2 Link: https://lore.kernel.org/all/cover.1785386749.git.zihanx@nebusec.ai/
changes in v2:
  - Rework the fix to preserve the existing passive-open tuple matching
    semantics instead of deferring child publication until LLC_CONN_PRIM.
  - Track listener-created children pending publication to accept(), and roll
    them back on every earlier failure or drop path.
  - Cover the original non-SABME leak and SABME paths which fail before
    LLC_CONN_PRIM, including backlog enqueue and backlog drop failures.
  - Correct Fixes to 1da177e4c3f4 ("Linux-2.6.12-rc2") based on the earliest
    locally visible history carrying the same root-cause fact.
  - Clarify panic_on_oom crash evidence and packetdrill selection.
  - v1 Link: https://lore.kernel.org/all/cover.1784725007.git.zihanx@nebusec.ai/
 net/llc/llc_conn.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 885a5c33024c..4a34f240ad2c 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -362,7 +362,8 @@ static int llc_conn_service(struct sock *sk, struct sk_buff *skb)
 	struct llc_sock *llc = llc_sk(sk);
 	int rc = 1;
 
-	if (llc->state > NBR_CONN_STATES)
+	if (llc->state < LLC_CONN_STATE_ADM ||
+	    llc->state > NBR_CONN_STATES)
 		goto out;
 	rc = 0;
 	trans = llc_qualify_conn_ev(sk, skb);
-- 
2.43.0

Re: [PATCH net v6 2/2] llc: reject out-of-service state before state lookup

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
llc: reject out-of-service state before state lookup

This adds a lower-bound check on llc->state in llc_conn_service() so that a
socket sitting in LLC_CONN_OUT_OF_SVC is rejected before llc_qualify_conn_ev()
indexes llc_conn_state_table[state - 1].  A few questions about the new
check below.

> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 885a5c33024c3..4a34f240ad2cc 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -362,7 +362,8 @@ static int llc_conn_service(struct sock *sk, struct sk_buff *skb)
>  	struct llc_sock *llc = llc_sk(sk);
>  	int rc = 1;
>  
> -	if (llc->state > NBR_CONN_STATES)
> +	if (llc->state < LLC_CONN_STATE_ADM ||
> +	    llc->state > NBR_CONN_STATES)
>  		goto out;

[Severity: High]
Does this check actually protect the table access, given that the validated
value is not latched?  llc_qualify_conn_ev() re-reads llc->state twice to do
the indexing:

net/llc/llc_conn.c:llc_qualify_conn_ev() {
	struct llc_conn_state *curr_state =
					&llc_conn_state_table[llc->state - 1];
	...
	for (next_trans = curr_state->transitions +
		llc_find_offset(llc->state - 1, ev->type);
	     (*next_trans)->ev; next_trans++) {
		if (!((*next_trans)->ev)(sk, skb)) {
	...
}

The store that produces state 0 is done with no socket lock at all:

net/llc/llc_conn.c:llc_sk_free() {
	llc->state = LLC_CONN_OUT_OF_SVC;
	/* Stop all (possibly) running timers */
	llc_sk_stop_all_timers(sk, sync);
	...
}

and llc_ui_release() calls it after dropping the socket lock and before the
timers are synchronized:

net/llc/af_llc.c:llc_ui_release() {
	...
		llc_release_incoming_children(sk);
		release_sock(sk);
	...
	llc_sk_free(sk, true);
}

So can this interleaving happen for a socket closed while an ack timer is
still armed (for example a non-blocking connect() left in
LLC_CONN_STATE_SETUP, where llc_send_disc() returns early because sk_state
is not TCP_ESTABLISHED and stops nothing)?

CPU0 (softirq, ack timer)
llc_conn_ack_tmr_cb() -> llc_conn_tmr_common_cb()
	bh_lock_sock(sk);          /* excludes nothing here */
	llc_process_tmr_ev()       /* sees state 2, !sock_owned_by_user() */
	  llc_conn_state_process() -> llc_conn_service()
		reads llc->state == 2, passes the new bound check

CPU1 (close(2))
llc_ui_release() -> release_sock(sk) -> llc_sk_free()
	llc->state = LLC_CONN_OUT_OF_SVC;     /* unlocked store of 0 */

CPU0 continues into llc_qualify_conn_ev()
	&llc_conn_state_table[llc->state - 1]   /* now index -1 */
	llc_find_offset(llc->state - 1, ev->type)
	(*next_trans)->ev(sk, skb)              /* indirect call */

Since bh_lock_sock() in llc_conn_tmr_common_cb() does not exclude a writer
that holds no lock, is the negative index and the indirect call through the
pointer read from before the tables still reachable after this patch?

Would latching the value once, for example

	u8 state = READ_ONCE(llc->state);

and passing that value down to llc_qualify_conn_ev() and llc_find_offset(),
or moving the llc_sk_free() state store under the socket lock, be a better
fit for the problem described in the changelog?

>  	rc = 0;
>  	trans = llc_qualify_conn_ev(sk, skb);

[Severity: Medium]
The goto out here leaves rc at its initial value of 1, which was previously
unreachable for a state of 0.  Where does that 1 end up?

net/llc/llc_conn.c:llc_conn_state_process() {
	rc = llc_conn_service(skb->sk, skb);
	if (unlikely(rc != 0)) {
		printk(KERN_ERR "%s: llc_conn_service failed\n", __func__);
		goto out_skb_put;
	}
	...
}

net/llc/llc_if.c:llc_establish_connection() {
	...
		rc = llc_conn_state_process(sk, skb);
	}
out_put:
	sock_put(sk);
	return rc;
}

net/llc/af_llc.c:llc_ui_connect() {
	rc = llc_establish_connection(sk, llc->dev->dev_addr,
				      addr->sllc_mac, addr->sllc_sap);
	if (rc) {
		sock->state  = SS_UNCONNECTED;
		sk->sk_state = TCP_CLOSE;
		goto out;
	}
	...
out:
	release_sock(sk);
	return rc;
}

Can connect(2) then return +1 to userspace on a socket that was just reset
to SS_UNCONNECTED and TCP_CLOSE?  Callers testing for a negative return
would read that as success.

Is the printk also a concern, since it is not ratelimited and now fires once
per event for a state this patch treats as expected?

The sequence I believe reaches it on a still-hashed socket:

  1. socket(AF_LLC, SOCK_STREAM) + bind() leaves llc->state at
     LLC_CONN_STATE_ADM
  2. a failing connect() leaves state at ADM with llc->daddr set and the
     socket still in the SAP list
  3. any further frame from the peer matches the ADM catch-all transition

net/llc/llc_c_st.c:
static const struct llc_conn_state_trans llc_adm_state_trans_5 = {
	.ev	       = llc_conn_ev_rx_any_frame,
	.next_state    = LLC_CONN_OUT_OF_SVC,
	.ev_qualifiers = NONE,
	.ev_actions    = llc_adm_actions_5,
};

     which sets llc->state to 0
  4. connect(2) again (sock->state is SS_UNCONNECTED, so it is accepted)
     reaches the new reject path

Would it be more consistent to translate out-of-service into -ECONNABORTED
at the llc_if.c entry points?  llc_send_disc() already does an explicit
check:

net/llc/llc_if.c:llc_send_disc() {
	if (sk->sk_type != SOCK_STREAM || sk->sk_state != TCP_ESTABLISHED ||
	    llc_sk(sk)->state == LLC_CONN_STATE_ADM ||
	    llc_sk(sk)->state == LLC_CONN_OUT_OF_SVC)
		goto out;
	...
}

while llc_establish_connection() has no such check.  On a related note, the
kernel-doc for llc_build_and_send_pkt() still documents only 0,
-ECONNABORTED and -EBUSY, although state 0 is filtered there only
incidentally by llc_data_accept_state().

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the callers
of llc_conn_service() there is an asymmetry in llc_backlog_rcv() worth
mentioning.  The fallback branch tests the owner socket's state and then
dispatches on it, while llc_conn_state_process() operates on skb->sk:

net/llc/llc_conn.c:llc_backlog_rcv() {
	...
	} else if (likely(llc->state > 1)) {
		rc = llc_conn_rcv(sk, skb);
	} else {
		goto out_kfree_skb;
	}
	...
}

The two branches above it take bh_lock_sock_nested(newsk) before touching a
child, this one does not.  For a listener-owned backlog skb whose skb->sk is
a child, the child's state machine runs without the child's lock, even
though llc_conn_state_process() documents that the socket lock has to be
held.

Every child reachable through this branch appears to have already been
released by llc_release_incoming_sock(), whose state store the new check now
rejects, so this looks defused for the concretely reachable case.  What
remains is the publication order in that function:

net/llc/llc_conn.c:llc_release_incoming_sock() {
	if (atomic_xchg(&llc->incoming_state, LLC_INCOMING_NONE) ==
	    LLC_INCOMING_NONE)
		return;

	WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
	...
}

Can a drainer in the fallback branch observe incoming_state as
LLC_INCOMING_NONE while still seeing the child's pre-teardown non-zero
state, and then run the child's state machine concurrently with
llc_release_incoming_sock() on another CPU?  Both fields are read there
without the child's bh lock.  Reaching the branch at all also needs the
listener's own llc->state above 1, which seems possible only through a
listen() followed by connect() on the same socket, since llc_ui_listen()
leaves sock->state at SS_UNCONNECTED and llc_ui_connect() has no TCP_LISTEN
check.  I could not confirm that all of these hold at the same time, so this
may not be reachable in practice.
-- 
pw-bot: cr

Re: [PATCH net v6 2/2] llc: reject out-of-service state before state lookup

[zihan xi]

On Wed, Sep 2, 2026 at 9:00 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> llc: reject out-of-service state before state lookup
>
> This adds a lower-bound check on llc->state in llc_conn_service() so that a
> socket sitting in LLC_CONN_OUT_OF_SVC is rejected before llc_qualify_conn_ev()
> indexes llc_conn_state_table[state - 1].  A few questions about the new
> check below.
>
> > diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> > index 885a5c33024c3..4a34f240ad2cc 100644
> > --- a/net/llc/llc_conn.c
> > +++ b/net/llc/llc_conn.c
> > @@ -362,7 +362,8 @@ static int llc_conn_service(struct sock *sk, struct sk_buff *skb)
> >       struct llc_sock *llc = llc_sk(sk);
> >       int rc = 1;
> >
> > -     if (llc->state > NBR_CONN_STATES)
> > +     if (llc->state < LLC_CONN_STATE_ADM ||
> > +         llc->state > NBR_CONN_STATES)
> >               goto out;
>
> [Severity: High]
> Does this check actually protect the table access, given that the validated
> value is not latched?  llc_qualify_conn_ev() re-reads llc->state twice to do
> the indexing:
>
> net/llc/llc_conn.c:llc_qualify_conn_ev() {
>         struct llc_conn_state *curr_state =
>                                         &llc_conn_state_table[llc->state - 1];
>         ...
>         for (next_trans = curr_state->transitions +
>                 llc_find_offset(llc->state - 1, ev->type);
>              (*next_trans)->ev; next_trans++) {
>                 if (!((*next_trans)->ev)(sk, skb)) {
>         ...
> }
>
> The store that produces state 0 is done with no socket lock at all:
>
> net/llc/llc_conn.c:llc_sk_free() {
>         llc->state = LLC_CONN_OUT_OF_SVC;
>         /* Stop all (possibly) running timers */
>         llc_sk_stop_all_timers(sk, sync);
>         ...
> }
>
> and llc_ui_release() calls it after dropping the socket lock and before the
> timers are synchronized:
>
> net/llc/af_llc.c:llc_ui_release() {
>         ...
>                 llc_release_incoming_children(sk);
>                 release_sock(sk);
>         ...
>         llc_sk_free(sk, true);
> }
>
> So can this interleaving happen for a socket closed while an ack timer is
> still armed (for example a non-blocking connect() left in
> LLC_CONN_STATE_SETUP, where llc_send_disc() returns early because sk_state
> is not TCP_ESTABLISHED and stops nothing)?
>
> CPU0 (softirq, ack timer)
> llc_conn_ack_tmr_cb() -> llc_conn_tmr_common_cb()
>         bh_lock_sock(sk);          /* excludes nothing here */
>         llc_process_tmr_ev()       /* sees state 2, !sock_owned_by_user() */
>           llc_conn_state_process() -> llc_conn_service()
>                 reads llc->state == 2, passes the new bound check
>
> CPU1 (close(2))
> llc_ui_release() -> release_sock(sk) -> llc_sk_free()
>         llc->state = LLC_CONN_OUT_OF_SVC;     /* unlocked store of 0 */
>
> CPU0 continues into llc_qualify_conn_ev()
>         &llc_conn_state_table[llc->state - 1]   /* now index -1 */
>         llc_find_offset(llc->state - 1, ev->type)
>         (*next_trans)->ev(sk, skb)              /* indirect call */
>
> Since bh_lock_sock() in llc_conn_tmr_common_cb() does not exclude a writer
> that holds no lock, is the negative index and the indirect call through the
> pointer read from before the tables still reachable after this patch?
>
> Would latching the value once, for example
>
>         u8 state = READ_ONCE(llc->state);
>
> and passing that value down to llc_qualify_conn_ev() and llc_find_offset(),
> or moving the llc_sk_free() state store under the socket lock, be a better
> fit for the problem described in the changelog?
>
> >       rc = 0;
> >       trans = llc_qualify_conn_ev(sk, skb);
>
> [Severity: Medium]
> The goto out here leaves rc at its initial value of 1, which was previously
> unreachable for a state of 0.  Where does that 1 end up?
>
> net/llc/llc_conn.c:llc_conn_state_process() {
>         rc = llc_conn_service(skb->sk, skb);
>         if (unlikely(rc != 0)) {
>                 printk(KERN_ERR "%s: llc_conn_service failed\n", __func__);
>                 goto out_skb_put;
>         }
>         ...
> }
>
> net/llc/llc_if.c:llc_establish_connection() {
>         ...
>                 rc = llc_conn_state_process(sk, skb);
>         }
> out_put:
>         sock_put(sk);
>         return rc;
> }
>
> net/llc/af_llc.c:llc_ui_connect() {
>         rc = llc_establish_connection(sk, llc->dev->dev_addr,
>                                       addr->sllc_mac, addr->sllc_sap);
>         if (rc) {
>                 sock->state  = SS_UNCONNECTED;
>                 sk->sk_state = TCP_CLOSE;
>                 goto out;
>         }
>         ...
> out:
>         release_sock(sk);
>         return rc;
> }
>
> Can connect(2) then return +1 to userspace on a socket that was just reset
> to SS_UNCONNECTED and TCP_CLOSE?  Callers testing for a negative return
> would read that as success.
>
> Is the printk also a concern, since it is not ratelimited and now fires once
> per event for a state this patch treats as expected?
>
> The sequence I believe reaches it on a still-hashed socket:
>
>   1. socket(AF_LLC, SOCK_STREAM) + bind() leaves llc->state at
>      LLC_CONN_STATE_ADM
>   2. a failing connect() leaves state at ADM with llc->daddr set and the
>      socket still in the SAP list
>   3. any further frame from the peer matches the ADM catch-all transition
>
> net/llc/llc_c_st.c:
> static const struct llc_conn_state_trans llc_adm_state_trans_5 = {
>         .ev            = llc_conn_ev_rx_any_frame,
>         .next_state    = LLC_CONN_OUT_OF_SVC,
>         .ev_qualifiers = NONE,
>         .ev_actions    = llc_adm_actions_5,
> };
>
>      which sets llc->state to 0
>   4. connect(2) again (sock->state is SS_UNCONNECTED, so it is accepted)
>      reaches the new reject path
>
> Would it be more consistent to translate out-of-service into -ECONNABORTED
> at the llc_if.c entry points?  llc_send_disc() already does an explicit
> check:
>
> net/llc/llc_if.c:llc_send_disc() {
>         if (sk->sk_type != SOCK_STREAM || sk->sk_state != TCP_ESTABLISHED ||
>             llc_sk(sk)->state == LLC_CONN_STATE_ADM ||
>             llc_sk(sk)->state == LLC_CONN_OUT_OF_SVC)
>                 goto out;
>         ...
> }
>
> while llc_establish_connection() has no such check.  On a related note, the
> kernel-doc for llc_build_and_send_pkt() still documents only 0,
> -ECONNABORTED and -EBUSY, although state 0 is filtered there only
> incidentally by llc_data_accept_state().
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but while looking at the callers
> of llc_conn_service() there is an asymmetry in llc_backlog_rcv() worth
> mentioning.  The fallback branch tests the owner socket's state and then
> dispatches on it, while llc_conn_state_process() operates on skb->sk:
>
> net/llc/llc_conn.c:llc_backlog_rcv() {
>         ...
>         } else if (likely(llc->state > 1)) {
>                 rc = llc_conn_rcv(sk, skb);
>         } else {
>                 goto out_kfree_skb;
>         }
>         ...
> }
>
> The two branches above it take bh_lock_sock_nested(newsk) before touching a
> child, this one does not.  For a listener-owned backlog skb whose skb->sk is
> a child, the child's state machine runs without the child's lock, even
> though llc_conn_state_process() documents that the socket lock has to be
> held.
>
> Every child reachable through this branch appears to have already been
> released by llc_release_incoming_sock(), whose state store the new check now
> rejects, so this looks defused for the concretely reachable case.  What
> remains is the publication order in that function:
>
> net/llc/llc_conn.c:llc_release_incoming_sock() {
>         if (atomic_xchg(&llc->incoming_state, LLC_INCOMING_NONE) ==
>             LLC_INCOMING_NONE)
>                 return;
>
>         WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
>         ...
> }
>
> Can a drainer in the fallback branch observe incoming_state as
> LLC_INCOMING_NONE while still seeing the child's pre-teardown non-zero
> state, and then run the child's state machine concurrently with
> llc_release_incoming_sock() on another CPU?  Both fields are read there
> without the child's bh lock.  Reaching the branch at all also needs the
> listener's own llc->state above 1, which seems possible only through a
> listen() followed by connect() on the same socket, since llc_ui_listen()
> leaves sock->state at SS_UNCONNECTED and llc_ui_connect() has no TCP_LISTEN
> check.  I could not confirm that all of these hold at the same time, so this
> may not be reachable in practice.
> --
> pw-bot: cr

Thanks for the review. The points on 2/2 look valid, especially the
unlatched llc->state check versus llc_sk_free(), and the connect(2)
return of +1 / unratelimited printk path.

I'll drop 2/2. Kees's net-next series covers the same out-of-service
lookup more completely, including the state machine guard and the
-ECONNABORTED translation:

  https://lore.kernel.org/all/20260901210300.i.590-kees@kernel.org/

I'll leave that bug to his series.

1/2 is a separate listener child-socket leak and is not addressed
there. I'll reroll that alone as v7.

The remaining note about llc_backlog_rcv() / llc_release_incoming_sock()
belongs to 1/2; I'll look at the publication order there in the reroll.

Thanks,
Zihan