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

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
llc: fix listener child socket leaks before passive open completes

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

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

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

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

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

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

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

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

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

[ ... ]

[ ... ]

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

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

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

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

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

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

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

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

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

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

	if (!llc->incoming_pend)
		return;

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

	skb_queue_purge(&sk->sk_receive_queue);

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

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

[ ... ]

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

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

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

so another CPU can find it in __llc_lookup_established():

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

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

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

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

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

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

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

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

llc_sk_free() hardcodes the synchronous variant:

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

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

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

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

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

The existing BH-context helper uses the async variant:

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

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

[ ... ]

[ ... ]

-- 
pw-bot: cr