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

On Fri, Aug 21, 2026 at 4:32 AM Jakub Kicinski <[email protected]> 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.
> ---
> llc: fix listener child socket leaks before passive open completes
>
> This changes llc_conn_handler() so that a child socket is only created for
> SABME commands, answers the other listener-side cases with a directly built
> DM response, and adds an atomic incoming-child lifecycle so unaccepted
> children are released on direct, backlog and listener-close failures.  The
> final child teardown is deferred to a work item so timers can be stopped
> synchronously in process context.
>
> A few questions below.
>
> > diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> > index b0447c33dbf09..837b8e0348637 100644
> > --- a/net/llc/af_llc.c
> > +++ b/net/llc/af_llc.c
> > @@ -206,6 +207,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);
>
> [Severity: High]
> This isn't a leak introduced by this patch, since unaccepted children leaked
> in every case before it, but does this branch leave one reachable case
> unfixed?
>
> The cleanup is gated on the socket's current sk_state, so a socket that was
> a listener but has since left TCP_LISTEN skips both llc_release_incoming_children()
> calls.
>
> llc_ui_listen() leaves sock->state as SS_UNCONNECTED, and llc_ui_connect()
> rejects only addrlen, family, sk_type and sock->state == SS_CONNECTING:
>
> net/llc/af_llc.c:llc_ui_connect() {
>         ...
>         rc = -EALREADY;
>         if (unlikely(sock->state == SS_CONNECTING))
>                 goto out;
>         ...
>         sock->state = SS_CONNECTING;
>         sk->sk_state   = TCP_SYN_SENT;
> }
>
> So connect() on a listening socket that already has an LLC_INCOMING_QUEUED
> indication on its receive queue is allowed and moves sk_state away from
> TCP_LISTEN.  llc_ui_accept() then refuses to consume the indication because
> it requires sk_state == TCP_LISTEN, and at close time listener is false.
>
> llc_sk_free() only does skb_queue_purge(&sk->sk_receive_queue), which frees
> the indication skb but never calls llc_release_incoming_sock() for the child
> it referenced.  Does the child then stay in the SAP tables forever, holding
> the dev_hold() reference taken in llc_create_incoming_sock() (blocking
> netdev unregistration) and the new sock_hold() on the closed listener?
>
> >       if (!sock_flag(sk, SOCK_ZAPPED)) {
> >               struct llc_sap *sap = llc->sap;
> >
> > @@ -214,16 +218,20 @@ static int llc_ui_release(struct socket *sock)
> >                */
> >               llc_sap_hold(sap);
> >               llc_sap_remove_socket(llc->sap, sk);
> > +             if (listener)
> > +                     llc_release_incoming_children(sk);
> >               release_sock(sk);
> >               llc_sap_put(sap);
> >       } else {
> > +             if (listener)
> > +                     llc_release_incoming_children(sk);
> >               release_sock(sk);
> >       }
>
> [Severity: High]
> Is it safe to call llc_release_incoming_children() here with softirqs
> enabled?  This path holds only lock_sock(sk), and lock_sock_nested() ends
> with spin_unlock_bh(), so softirqs are on.  llc_release_incoming_children()
> then takes the child's slock with a plain spin_lock():
>
> net/llc/llc_conn.c:llc_release_incoming_children() {
>         ...
>                 if (newsk != sk)
>                         bh_lock_sock_nested(newsk);
>         ...
> }
>
> The same slock is taken from softirq context by the receive path and by the
> LLC timer callbacks:
>
> net/llc/llc_c_ac.c:llc_conn_tmr_common_cb() {
>         ...
>         bh_lock_sock(sk);
>         ...
> }
>
> While that child lock is held the child is still published in the SAP hash
> (llc_sap_remove_socket() only runs later inside llc_release_incoming_sock())
> and llc_estab_match() still matches it, so can a NET_RX softirq on the same
> CPU look up that child and spin on the held lock?
>
> The window is even opened from inside the critical section:
> llc_release_incoming_sock() -> llc_sap_remove_socket() does
> spin_unlock_bh(&sap->sk_lock), whose local_bh_enable() runs pending softirqs
> while the child's plain spin_lock is still held:
>
> llc_ui_release()
>   llc_release_incoming_children()
>     bh_lock_sock_nested(child)
>       llc_release_incoming_sock(child)
>         llc_sap_remove_socket()
>           spin_unlock_bh(&sap->sk_lock)   /* runs softirqs */
>             llc_rcv() -> llc_conn_handler() -> bh_lock_sock(child)
>
> Elsewhere LLC wraps such acquisitions in local_bh_disable(), for example
> llc_lookup_established().  Would a lockdep inconsistent softirq-on /
> in-softirq usage report also fire here on CONFIG_PROVE_LOCKING?
>
> [ ... ]
>
> > diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> > index 260460d50f54c..bc05a5f66f95f 100644
> > --- a/net/llc/llc_conn.c
> > +++ b/net/llc/llc_conn.c
> > @@ -354,7 +360,8 @@ static int llc_conn_service(struct sock *sk, struct sk_buff *skb)
> >       struct llc_sock *llc = llc_sk(sk);
> >       int rc = 1;
> >
> > -     if (llc->state > NBR_CONN_STATES)
> > +     if (llc->state < LLC_CONN_STATE_ADM ||
> > +         llc->state > NBR_CONN_STATES)
> >               goto out;
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but could this hunk be split out
> with its own changelog?  The added lower bound looks like it fixes a
> distinct, remotely reachable out-of-bounds access that the commit message
> does not mention.
>
> Without it, llc_qualify_conn_ev() indexes element -1 when llc->state is
> LLC_CONN_OUT_OF_SVC (0) and then calls function pointers read from there:
>
> net/llc/llc_conn.c:llc_qualify_conn_ev() {
>         struct llc_conn_state *curr_state =
>                                         &llc_conn_state_table[llc->state - 1];
>         ...
> }
>
> State 0 is reachable from a received frame, since llc_adm_state_trans_5 has:
>
> net/llc/llc_c_st.c
> static const struct llc_conn_state_trans llc_adm_state_trans_5 = {
>         .ev            = llc_conn_ev_rx_any_frame,
>         .next_state    = LLC_CONN_OUT_OF_SVC,
>         ...
> };
>
> and llc_estab_match() does not filter on llc->state, so the socket keeps
> being returned by __llc_lookup() for subsequent frames.  As written, a
> stable backporter has no indication that a memory-safety fix is included
> here.
>
> [ ... ]
>
> > +void llc_release_incoming_children(struct sock *sk)
> > +{
> > +     struct sk_buff *skb;
> > +
> > +     while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
> > +             struct sock *newsk = skb->sk;
> > +
> > +             if (newsk != sk)
> > +                     bh_lock_sock_nested(newsk);
> > +             if (newsk != sk) {
> > +                     llc_release_incoming_sock(newsk);
> > +                     kfree_skb(skb);
> > +                     bh_unlock_sock(newsk);
> > +             } else {
> > +                     kfree_skb(skb);
> > +             }
> > +     }
> > +}
>
> [ ... ]
>
> > @@ -795,10 +921,25 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
> >       bh_lock_sock(sk);
> > +     if (unlikely(sock_flag(sk, SOCK_DEAD)))
> > +             goto drop_unlock;
>
> [Severity: Medium]
> Is this test too broad?  llc_ui_release() only sets SOCK_DEAD when the
> socket is in TCP_LISTEN, but this check runs right after __llc_lookup() and
> so applies to every socket the lookup returns.
>
> Existing code already sets SOCK_DEAD on ordinary established stream sockets
> when the peer disconnects:
>
> net/llc/llc_conn.c:llc_conn_state_process() {
>         case LLC_DISC_PRIM:
>                 ...
>                 if (sk->sk_type == SOCK_STREAM &&
>                     sk->sk_state == TCP_ESTABLISHED) {
>                         ...
>                         if (!sock_flag(sk, SOCK_DEAD)) {
>                                 sock_set_flag(sk, SOCK_DEAD);
>                                 sk->sk_state_change(sk);
>                         }
>                 }
> }
>
> At that point the socket is still published in the SAP tables, because
> llc_sap_remove_socket() only runs from llc_ui_release(), and
> llc_estab_match() still matches it.  After such a peer-initiated disconnect,
> are all further frames for that tuple now silently dropped instead of
> running the ADM-state transitions that answer retransmitted DISC and other
> commands with DM responses (llc_adm_state_trans_3 and
> llc_adm_state_trans_4)?  That is the same protocol behaviour this patch
> open-codes for listeners in llc_conn_send_dm_rsp().
>
> Would distinguishing the listener case, for instance testing
> sk->sk_state == TCP_LISTEN together with SOCK_DEAD, or using a dedicated
> flag, be closer to the intent?
>
> > +     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;
> > +     }
> >       /*
> >        * This has to be done here and not at the upper layer ->accept
> >        * method because of the way the PROCOM state machine works:
> > @@ -795,10 +921,25 @@ 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)) {
>
> [Severity: Medium]
> When the lookup returned an LLC_INCOMING_PENDING child, sk was re-pointed at
> the child's incoming_listener and only SOCK_DEAD was validated afterwards.
> What happens if that socket is no longer TCP_LISTEN?
>
> This condition is then false and control falls into the non-listener else
> branch:
>
>         } else {
>                 skb_orphan(skb);
>                 sock_hold(sk);
>                 skb->sk = sk;
>                 skb->destructor = sock_efree;
>         }
>
> so the child's PDU is fed into the ex-listener's own connection state
> machine, since llc_conn_state_process() runs llc_conn_service(skb->sk, skb).
> The backlog path guards exactly this case:
>
> net/llc/llc_conn.c:llc_backlog_rcv() {
>         ...
>                         if (sock_flag(sk, SOCK_DEAD) ||
>                             sk->sk_state != TCP_LISTEN)
>                                 goto out_unlock_newsk;
>         ...
> }
>
> Reachability looks similar to the llc_ui_release() case above: connect() on
> a listening socket is permitted and sets sk_state to TCP_SYN_SENT under
> lock_sock(), a SABME that arrived while the lock was owned leaves a PENDING
> child, and a second frame for that tuple arriving before release_sock()
> drains the backlog takes the redirect path with sk_state == TCP_SYN_SENT.
> Can a frame from peer A then drive the state machine of a socket that is
> connecting to peer B, while the PENDING child is torn down by
> llc_release_incoming_sock() without its frame ever being processed for it?
>
> [ ... ]
>
> > @@ -852,12 +1013,38 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> >  {
> >       int rc = 0;
> >       struct llc_sock *llc = llc_sk(sk);
> > +     struct sock *newsk = NULL;
> >
> >       if (likely(llc_backlog_type(skb) == LLC_PACKET)) {
> > -             if (likely(llc->state > 1)) /* not closed */
> > +             newsk = skb->sk;
> > +             if (newsk &&
> > +                 atomic_read(&llc_sk(newsk)->incoming_state) ==
> > +                 LLC_INCOMING_PENDING) {
> > +                     bh_lock_sock_nested(newsk);
>
> [Severity: High]
> Same locking question as in llc_release_incoming_children(): this handler
> runs from release_sock() -> __release_sock(), which drops the parent slock
> with spin_unlock_bh() and even calls cond_resched() in that loop:
>
> net/core/sock.c:__release_sock() {
>                 spin_unlock_bh(&sk->sk_lock.slock);
>                 while (1) {
>                         ...
>                         sk_backlog_rcv(sk, skb);
>         ...
> }
>
> so softirqs are enabled here too.  Can these bh_lock_sock_nested() and
> bh_lock_sock() acquisitions on the child (also in the retry,
> out_unlock_newsk and out_kfree_skb paths) deadlock against the same lock
> taken from llc_rcv() -> llc_conn_handler() or from the LLC timer callbacks
> on the same CPU?  The child slock is additionally held across a full
> llc_conn_rcv() state-machine run, including dev_queue_xmit().
>
> > +                     if (atomic_read(&llc_sk(newsk)->incoming_state) !=
> > +                         LLC_INCOMING_PENDING) {
> > +                             bh_unlock_sock(newsk);
> > +                             goto retry;
> > +                     }
> > +                     if (sock_flag(sk, SOCK_DEAD) ||
> > +                         sk->sk_state != TCP_LISTEN)
> > +                             goto out_unlock_newsk;
> >                       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);
> > +             } else if (newsk &&
> > +                        atomic_read(&llc_sk(newsk)->incoming_state) ==
> > +                        LLC_INCOMING_QUEUED) {
> > +                     bh_lock_sock(newsk);
> > +                     rc = llc_conn_rcv(newsk, skb);
> > +                     bh_unlock_sock(newsk);
> > +             } else if (likely(llc->state > 1)) {
> > +                     rc = llc_conn_rcv(sk, skb);
> > +             } else {
> >                       goto out_kfree_skb;
> > +             }
>
> [Severity: Medium]
> The new QUEUED branch dispatches into llc_conn_rcv(newsk, skb) with no
> closed-state filter, while the surviving else-if branch still keeps the
> llc->state > 1 test.  Combined with the new lower bound in
> llc_conn_service(), does a frame for a child left in LLC_CONN_OUT_OF_SVC now
> produce one unratelimited log line per frame?
>
> llc_conn_service() returns 1 for state below LLC_CONN_STATE_ADM, and
> llc_conn_state_process() prints unconditionally:
>
> 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;
>         }
>         ...
> }
>
> State 0 looks remotely reachable: a child back in ADM (NORMAL plus rx DISC)
> that then receives any frame that is neither SABME, DISC nor a command with
> P=1 hits llc_adm_state_trans_5 with .next_state = LLC_CONN_OUT_OF_SVC and a
> no-op action, while it is still hashed in the SAP table and matched by
> llc_estab_match().  Every further frame for that tuple would then reach this
> branch, and the direct llc_conn_rcv() path in llc_conn_handler(), and emit
> another KERN_ERR line.  Would filtering the closed state in these new
> branches be preferable to letting llc_conn_service() fail?
>
> [ ... ]
> --
> pw-bot: cr
Hi Jakub,

Thanks for forwarding the review.

I agree there are enough concerns here to warrant a v2. I’ll rework the
patch and recheck the listener-child cleanup path, the child socket
locking, and the closed-state handling. I’ll also split out the
llc->state lower-bound change if it is indeed a separate fix.

Thanks,
Zihan