Re: [PATCH] net: llc: clean up unaccepted child sockets in llc_ui_release()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 1 · High: 3 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] Type confusion in the new TCP_LISTEN drain loop in
  llc_ui_release(): the loop assumes every skb on sk->sk_receive_queue…
- [High] The drain of the listener's sk_receive_queue is placed at the
  very start of llc_ui_release() (net/llc/af_llc.c:207-225), before…
- [High] The new child teardown in llc_ui_release()
  (net/llc/af_llc.c:213-222) removes the child from the SAP hash, drops…
- [High] Incomplete fix: the new release-time drain can only reclaim
  children whose skb reached the listener's sk_receive_queue, i.e.…
- [Medium] Reference-tracker acquire/release mismatch: the new code
  releases the child's netdev reference with netdev_put(child_llc->dev,…
- [Low] The commit message cites a non-existent symbol: "llc_ui_release()
  calls skb_queue_purge(&sk->sk_receive_queue) in llc_sk_Component /…

--

net: llc: clean up unaccepted child sockets in llc_ui_release()

The patch adds a drain of the listener's sk_receive_queue at the top of
llc_ui_release() when sk_state is TCP_LISTEN.  For every queued skb it
takes skb->sk as an unaccepted child, removes it from its SAP, drops the
device reference and frees it with llc_sk_free().  A few questions about
the new block and the changelog below.

Commit message:

> If the listening socket is closed without calling `accept()`,
> `llc_ui_release()` calls `skb_queue_purge(&sk->sk_receive_queue)` in
> `llc_sk_Component` / `llc_sk_free()`, which frees the queued `skb`s

[Severity: Low]
Where does llc_sk_Component come from?  A grep for that string over the
tree at this commit returns nothing, and the only skb_queue_purge() of
sk->sk_receive_queue in net/llc is the one in llc_sk_free() in
net/llc/llc_conn.c.  Could the changelog name only the function that
actually exists?

> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> index b0447c33dbf0..4bc7839127aa 100644
> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
> @@ -204,6 +204,25 @@ static int llc_ui_release(struct socket *sock)
>  	llc = llc_sk(sk);
>  	dprintk("%s: closing local(%02X) remote(%02X)\n", __func__,
>  		llc->laddr.lsap, llc->daddr.lsap);
> +	if (sk->sk_state == TCP_LISTEN) {
> +		struct sk_buff *skb;
> +
> +		while ((skb = skb_dequeue(&sk->sk_receive_queue)) != NULL) {
> +			struct sock *child = skb->sk;
> +
> +			if (child) {

[Severity: Critical]
Is every skb on a TCP_LISTEN socket's receive queue really a connection
indication carrying a child sock?  In llc_conn_state_process() the data
path queues the skb on the receiving socket itself:

net/llc/llc_conn.c:llc_conn_state_process() {
	case LLC_DATA_PRIM:
		skb_get(skb);
		llc_save_primitive(sk, skb, LLC_DATA_PRIM);
		if (unlikely(sock_queue_rcv_skb(sk, skb))) {
	...
}

so skb_set_owner_r() leaves skb->sk == sk.  Those skbs can still be on the
queue when the socket goes back to listening, because the DISC path only
does:

net/llc/llc_conn.c:llc_conn_state_process() {
	...
			sk->sk_socket->state  = SS_UNCONNECTED;
			sk->sk_state          = TCP_CLOSE;
	...
}

without purging the queue, and llc_ui_listen() accepts that state:

net/llc/af_llc.c:llc_ui_listen() {
	if (unlikely(sock->state != SS_UNCONNECTED))
		goto out;
	...
	if (sk->sk_state != TCP_LISTEN) {
		sk->sk_ack_backlog = 0;
		sk->sk_state	   = TCP_LISTEN;
	}
}

With child == sk, does this loop then tear down the listener itself, after
which the unchanged tail of llc_ui_release() repeats
llc_sap_remove_socket(llc->sap, sk), netdev_put(llc->dev,
&llc->dev_tracker), sock_put(sk) and llc_sk_free(sk) on the same socket?
The second llc_sap_remove_socket() reaches an unconditional hlist_del():

net/llc/llc_conn.c:llc_sap_remove_socket() {
	spin_lock_bh(&sap->sk_lock);
	sk_nulls_del_node_init_rcu(sk);
	hlist_del(&llc->dev_hash_node);
	sap->sk_count--;
	...
}

sk_nulls_del_node_init_rcu() is idempotent, but hlist_del() is not, so the
second pass writes through an already poisoned pprev.  Should the loop
skip skbs whose skb->sk == sk (or compare against the listener) before
doing any of this?

[Severity: High]
Can children that never reach this queue still leak?  llc_conn_handler()
creates the child before the frame is classified:

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

A fresh child is in LLC_CONN_STATE_ADM, and only the SABME transition runs
llc_conn_ac_conn_ind().  The DISC and catch-all transitions use
llc_conn_disc():

net/llc/llc_c_ac.c:llc_conn_disc() {
	/* FIXME: this thing seems to want to die */
	return 0;
}

so ev->ind_prim stays 0 and llc_conn_state_process() ends at:

net/llc/llc_conn.c:llc_conn_state_process() {
out_skb_put:
	kfree_skb(skb);
	return rc;
}

Since skb_set_owner_r() holds no sock reference, does the child stay in
sap->sk_laddr_hash with its dev_hold() forever, i.e. the same leak the
changelog describes, reachable from an inbound non-SABME frame without
ever closing the listener?

> +				struct llc_sock *child_llc = llc_sk(child);
> +
> +				skb_orphan(skb);
> +				if (child_llc->sap)
> +					llc_sap_remove_socket(child_llc->sap, child);
> +				netdev_put(child_llc->dev, &child_llc->dev_tracker);

[Severity: Medium]
Does the tracked release here match the acquire side?  The child's device
reference is taken with plain dev_hold() in llc_create_incoming_sock(),
and dev_hold() is netdev_hold(dev, NULL, GFP_ATOMIC), so
child_llc->dev_tracker is never populated and only dir->no_tracker is
incremented.  netdev_put() with a non-NULL trackerp whose value is NULL
lands here:

lib/ref_tracker.c:ref_tracker_free() {
	if (!trackerp) {
		refcount_dec(&dir->no_tracker);
		return 0;
	}
	tracker = *trackerp;
	if (!tracker) {
		refcount_dec(&dir->untracked);
		return -EEXIST;
	}
	...
}

With CONFIG_NET_DEV_REFCNT_TRACKER=y, does that decrement of dir->untracked
(which carries a bias of 1) produce a refcount_t underflow warning, and
leave no_tracker elevated so ref_tracker_dir_exit() warns at device
teardown?  llc_ui_autobind() shows the other convention:

net/llc/af_llc.c:llc_ui_autobind() {
	llc->dev = dev;
	netdev_tracker_alloc(llc->dev, &llc->dev_tracker, GFP_KERNEL);
	...
}

Would either using dev_put() here, or converting
llc_create_incoming_sock() to netdev_hold(dev, &newllc->dev_tracker,
GFP_ATOMIC), be the better pairing?

> +				sock_orphan(child);
> +				llc_sk_free(child);

[Severity: High]
Is anything serialising this teardown against the child's own receive
path?  The child is fully hashed by llc_sap_add_socket(), so a softirq can
find it and run its state machine under its own lock:

net/llc/llc_conn.c:llc_conn_handler() {
	sk = __llc_lookup(sap, &saddr, &daddr, dev_net(skb->dev));
	if (!sk)
		goto drop;

	bh_lock_sock(sk);
	...
	if (!sock_owned_by_user(sk))
		llc_conn_rcv(sk, skb);
	...
}

The listener's lock_sock() does not exclude that, and actions such as:

net/llc/llc_c_ac.c:llc_conn_ac_start_ack_timer() {
	mod_timer(&llc_sk(sk)->ack_timer.timer,
		  jiffies + llc_sk(sk)->ack_timer.expire);
	return 0;
}

can re-arm an embedded timer after llc_sk_free() -> llc_sk_stop_all_timers(sk,
true) has already done timer_delete_sync().  Can that leave an armed timer
on a sock whose last reference is dropped by the sock_put() at the end of
llc_sk_free(), and can llc_conn_send_pdus() use child_llc->dev after the
netdev_put() above?  TCP takes bh_lock_sock() plus a sock_hold() on each
child in inet_csk_listen_stop(); should something equivalent be done here?

> +			}
> +			kfree_skb(skb);
> +		}
> +	}
>  	if (!llc_send_disc(sk))
>  		llc_ui_wait_for_disc(sk, READ_ONCE(sk->sk_rcvtimeo));
>  	if (!sock_flag(sk, SOCK_ZAPPED)) {

[Severity: High]
Does draining here, before the listener is unhashed and before the backlog
is flushed, still leave the leak open?  llc_sap_remove_socket(llc->sap, sk)
and release_sock(sk) run only after this point, so while the socket lock is
held llc_conn_handler() can still find the listener, create a child and
then push the skb to the backlog:

net/llc/llc_conn.c:llc_conn_handler() {
	if (unlikely(sk->sk_state == TCP_LISTEN)) {
		struct sock *newsk = llc_create_incoming_sock(sk, skb->dev,
							      &saddr, &daddr);
		if (!newsk)
			goto drop_unlock;
		skb_set_owner_r(skb, newsk);
	}
	...
	if (!sock_owned_by_user(sk))
		llc_conn_rcv(sk, skb);
	else {
		llc_set_backlog_type(skb, LLC_PACKET);
		if (sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf)))
			goto drop_unlock;
	}
}

release_sock() then runs llc_backlog_rcv() on a listener whose llc->state is
LLC_CONN_STATE_ADM:

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

so the skb is just freed and the freshly created child, its SAP reference
and its device reference are leaked again.  There also seems to be a second
window after release_sock(): a softirq that looked up the listener before it
was unhashed can queue another child-bearing skb on sk_receive_queue while
sk_state is still TCP_LISTEN, and llc_sk_free(sk) then purges it.  Would
performing this cleanup after the listener is unhashed and the backlog
flushed (or directly at the purge site) close both windows?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919213635.3314344-1-benquike%40gmail.com