Re: [PATCH net v9 1/1] llc: fix listener child socket leak on non-SABME frames

[Jakub Kicinski <kuba@kernel.org>]

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 leak on non-SABME frames

This restricts llc_conn_handler() to creating a passive-open child socket
only for SABME commands, answers DISC and other P=1 commands with a DM
response built from the address decoded out of the received frame, and
drops the remaining non-SABME frames without running them through the
listener's ADM state machine.

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

[ ... ]

> @@ -795,11 +825,26 @@ 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)
> +		struct sock *newsk;
> +
> +		if (!llc_conn_ev_rx_sabme_cmd_pbit_set_x(sk, skb)) {
> +			newsk = llc_create_incoming_sock(sk, skb->dev, &saddr,
> +							 &daddr);
> +			if (!newsk)
> +				goto drop_unlock;
> +			skb_set_owner_r(skb, newsk);

[Severity: High]

Does the retained SABME path still leak the child struct sock and the
device reference that the changelog describes?

llc_create_incoming_sock() publishes the child and pins the device before
anything has decided the child will ever be handed to accept():

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() also takes llc_sap_hold() and bumps sap->sk_count, so
each unaccepted child holds a struct sock, a SAP reference and a netdev
reference.

There appear to be three paths where none of that is undone.

First, closing the listener.  llc_ui_release() removes only the listener
from the SAP and netdev_put()s only the listener's device, then calls
llc_sk_free(), which only purges the queues:

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

skb_set_owner_r(skb, newsk) above takes no socket reference, so purging the
listener's receive queue only runs sock_rfree() on the queued indication.
Doesn't the child then stay in the SAP hash with its refcount and its
dev_hold() forever, which is the same leftover /proc/net/llc/socket state
the changelog cites?

Second, the backlog path in this same function.  Once skb_set_owner_r() has
run, the child is already published, and:

	if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf)))
		goto drop_unlock;

drop_unlock only does kfree_skb() plus sock_put() on the listener.  Should
the freshly published child be unhashed and released here as well?

Third, llc_conn_state_process() bails out before the only handoff to
accept() if the state machine fails:

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

That skips the LLC_CONN_PRIM case that queues the indication on the
listener, and llc_conn_ac_send_ua_rsp_f_set_p() can return -ENOBUFS under
memory pressure, which is exactly the condition a flood creates.  Is the
child reclaimed on that path?

Related question on admission control: is there anything bounding how many
children a flood can create?  llc_ui_listen() stores the limit:

net/llc/af_llc.c:llc_ui_listen() {
	sk->sk_max_ack_backlog = backlog;

but the receive path never consults sk_max_ack_backlog and there is no
sk_acceptq_added() anywhere in net/llc.  The indication skb is charged to
the child by skb_set_owner_r(), not to the listener, so the listener's
sk_rcvbuf does not bound it either.

Given the above, could the following changelog statements be revisited?

    Repeating such frames, for example DISC commands with distinct source
    addresses, can leak struct sock objects and device references and
    eventually exhaust memory.

    The existing SABME child lifecycle is unchanged; this patch addresses
    only the non-SABME path.

If an L2-adjacent sender emits SABME commands rather than DISC commands,
with distinct source MAC/SSAP tuples, doesn't the same unbounded
accumulation of children, queued indications and pinned netdev references
remain reachable, including the pinned device making unregister_netdevice()
wait?  Would a fix that gives the child an owner and a teardown path (a
listener child list, or release-on-drop) plus an accept-queue bound be
needed to close the exhaustion the changelog and the Cc: stable tag refer
to, rather than filtering on frame type alone?

> +		} else {
> +			/* Not a passive-open request. */
> +			if (!llc_conn_ev_rx_disc_cmd_pbit_set_x(sk, skb)) {
> +				u8 f_bit;
> +
> +				llc_pdu_decode_pf_bit(skb, &f_bit);
> +				llc_listener_send_dm(sap, skb, &saddr, f_bit);
> +			} else if (!llc_conn_ev_rx_xxx_cmd_pbit_set_1(sk, skb)) {
> +				llc_listener_send_dm(sap, skb, &saddr, 1);
> +			}
>  			goto drop_unlock;
> -		skb_set_owner_r(skb, newsk);
> +		}
>  	} else {

[ ... ]