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

[zihan xi]

On Sun, Sep 6, 2026 at 8:48 PM Simon Horman <horms@xxxxxxxxxx> 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@xxxxxxxxxx?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