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

[zihan xi]

On Wed, Aug 26, 2026 at 4:34 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/20260822082354.3109-2-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 directly generated DM response, and adds
> an incoming_state lifecycle (PENDING/QUEUED/NONE) so that children which
> never reach accept() are unhashed and torn down from a work item.
>
> > diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> > index b0447c33dbf0..7d246d4d281b 100644
> > --- a/net/llc/af_llc.c
> > +++ b/net/llc/af_llc.c
>
> [ ... ]
>
> > @@ -722,6 +728,7 @@ static int llc_ui_accept(struct socket *sock, struct socket *newsock,
> >               goto frees;
> >       rc = 0;
> >       newsk = skb->sk;
> > +     llc_accept_incoming_sock(newsk);
> >       /* attach connection to a new socket. */
> >       llc_ui_sk_init(newsock, newsk);
> >       sock_reset_flag(newsk, SOCK_ZAPPED);
>
> [Severity: High]
>
> llc_accept_incoming_sock() performs the QUEUED to LLC_INCOMING_NONE
> transition, and the paired sock_put(llc->incoming_listener), with only the
> listener's owner lock held (the lock_sock(sk) at the top of
> llc_ui_accept()).
>
> Every other transition site holds the child's slock instead:
> bh_lock_sock()/bh_lock_sock_nested(newsk) in llc_conn_handler(),
> local_bh_disable() plus bh_lock_sock_nested(newsk) in llc_backlog_rcv(),
> and bh_lock_sock_nested(newsk) in llc_release_incoming_children().
>
> Since those two lock sets are disjoint, can a softirq-side release win the
> atomic_xchg while accept() sits between skb_dequeue() and the graft?
>
> CPU0 (softirq) llc_conn_handler() or llc_backlog_rcv()
>         bh_lock_sock_nested(newsk);
>         llc_release_incoming_sock(newsk);  /* xchg -> NONE, work queued */
>
> CPU1 (process) llc_ui_accept()
>         newsk = skb->sk;
>         llc_accept_incoming_sock(newsk);   /* xchg returns NONE, no fixup */
>         llc_ui_sk_init(newsock, newsk);
>         newsk->sk_state         = TCP_ESTABLISHED;
>
> llc_accept_incoming_sock() returns void, so llc_ui_accept() has no way to
> notice that it lost the transition, and it takes no reference on newsk and
> never holds the child's lock.
>
> llc_incoming_sock_work() only waits on lock_sock(listener), which accept()
> drops on return, so the final sock_put() pair can land immediately after
> newsock->sk has been published to userspace.
>
> Would it work for llc_accept_incoming_sock() to return whether it won the
> transition, taken under the child's lock, so accept() can fail instead of
> grafting a socket whose teardown is already scheduled?
>
> > diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> > index 260460d50f54..8120ecd46ace 100644
> > --- a/net/llc/llc_conn.c
> > +++ b/net/llc/llc_conn.c
>
> [ ... ]
>
> > @@ -88,6 +89,11 @@ 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)
> > +                     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 +771,153 @@ 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 *sk = &llc->sk;
> > +     struct sock *listener = llc->incoming_listener;
> > +
> > +     lock_sock(listener);
> > +     lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> > +     llc_sk_free(sk, false);
> > +     sock_orphan(sk);
> > +     release_sock(sk);
> > +     llc_sk_stop_all_timers(sk, true);
> > +     release_sock(listener);
> > +     dev_put(llc->dev);
> > +     llc->dev = NULL;
> > +     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);
> > +     sock_hold(sk);
> > +     llc_sap_remove_socket(llc->sap, sk);
> > +     schedule_work(&llc->incoming_work);
> > +}
>
> [ ... ]
>
> > @@ -813,18 +960,46 @@ 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)
> > +                     llc_release_incoming_sock(newsk);
> > +             else if (atomic_read(&llc_sk(sk)->incoming_state) !=
> > +                      LLC_INCOMING_NONE)
> > +                     llc_release_incoming_sock(sk);
> > +             goto drop_unlock;
> > +     }
>
> [Severity: Critical]
>
> Can this free a child that is already sitting on the listener's accept
> queue?
>
> Once llc_conn_state_process() has flipped the child to
> LLC_INCOMING_QUEUED, the connection indication skb is on the listener's
> sk_receive_queue holding a raw skb->sk pointer to the child;
> skb_set_owner_r() takes no socket reference. This gate releases the child
> found directly by established lookup whenever incoming_state is not
> LLC_INCOMING_NONE, which includes QUEUED children.
>
> A purely remote frame sequence against a listening socket reaches it:
>
>   SABME  -> llc_create_incoming_sock() publishes the child, PENDING
>          -> llc_conn_state_process() LLC_CONN_PRIM: QUEUED, skb queued
>   DISC   -> __llc_lookup() selects the child, and llc_common_state_trans_4
>             in net/llc/llc_c_st.c moves it from NORMAL to
>             LLC_CONN_STATE_ADM
>   command frame -> llc_adm_state_trans_5 sets LLC_CONN_OUT_OF_SVC
>   one more frame -> this gate sees state < LLC_CONN_STATE_ADM and
>             incoming_state == LLC_INCOMING_QUEUED
>
> llc_release_incoming_sock() then drops the SAP hash reference through
> llc_sap_remove_socket() and schedules llc_incoming_sock_work(), which drops
> the llc_sk_free() sock_put plus the explicit sock_put, so the child is
> freed. Nothing dequeues or invalidates the indication skb.
>
> Does a later accept() then work on freed memory?
>
> net/llc/af_llc.c:llc_ui_accept() {
>         newsk = skb->sk;
>         llc_accept_incoming_sock(newsk);
>         llc_ui_sk_init(newsock, newsk);
>         sock_reset_flag(newsk, SOCK_ZAPPED);
>         newsk->sk_state         = TCP_ESTABLISHED;
>         ...
> frees:
>         kfree_skb(skb);
> }
>
> The trailing kfree_skb() also runs sock_rfree() on that sock, and listener
> close in llc_release_incoming_children() reads
> llc_sk(newsk)->incoming_state and takes bh_lock_sock_nested(newsk) on the
> same pointer.
>
> The QUEUED branch and the retry: block of llc_backlog_rcv() call
> llc_release_incoming_sock() on an already-indicated child in the same way.
>
> Should the release paths be limited to LLC_INCOMING_PENDING children, or
> should the queued indication skb be removed from the listener's receive
> queue as part of the release?
>
> > +     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 {
>
> [ ... ]
>
> > @@ -852,12 +1027,53 @@ static int llc_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>
> [ ... ]
>
> > +                     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;
> > +                     }
>
> [ ... ]
>
> > +             } 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) {
> > +                             llc_release_incoming_sock(newsk);
> > +                             bh_unlock_sock(newsk);
> > +                             local_bh_enable();
> > +                             goto out_kfree_skb;
> > +                     }
>                                             ^^
>
> [Severity: Medium]
>
> Is the <= bound intended here? The PENDING branch just above and the gate
> in llc_conn_handler() both use state < LLC_CONN_STATE_ADM, and the v5
> changelog describes the new check as an LLC_CONN_OUT_OF_SVC lower bound.
>
> LLC_CONN_STATE_ADM (1) is a live state with its own transition table
> (llc_adm_state_transitions), and a queued child legitimately returns to it
> after a remote DISC:
>
> 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,
>
> So the same frame is dispatched when it arrives on the direct path in
> llc_conn_handler(), but here it is dropped and the child is destroyed via
> llc_release_incoming_sock(). The retry: block below applies the same
> asymmetric bound with state > LLC_CONN_STATE_ADM.
>
> Since llc_qualify_conn_ev() indexes llc_conn_state_table[llc->state - 1],
> would excluding only LLC_CONN_OUT_OF_SVC (0) be enough?
>
> [ ... ]

Hi Simon,

Thanks for forwarding the review. I checked each point manually.

[High]
Confirmed. v5 lets accept() consume the incoming_state transition without
holding the child lock or knowing whether the concurrent release path already
won it. The indication skb also does not keep a child socket reference.

[Critical]
Confirmed. A QUEUED child can be released by the out-of-service handling while
its connection indication remains on the listener receive queue. That leaves
the skb->sk pointer stale for a later accept() or listener teardown.

[Medium]
Confirmed. LLC_CONN_STATE_ADM is a valid state, including after a DISC
transition. The queued-child path should reject only states below ADM, not ADM
itself.

For v6, the queued indication will hold a child socket reference until it is
consumed; accept() will serialize with the child lock and verify that it won
the QUEUED-to-NONE transition; packet and backlog paths will only release
PENDING children; and the queued-child state bound will be made consistent
with the lower-bound state-table check.

Thanks,
Zihan