Re: [PATCH net v6 2/2] llc: reject out-of-service state before state lookup

[zihan xi]

On Wed, Sep 2, 2026 at 9:00 AM Jakub Kicinski <kuba@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.
> ---
> llc: reject out-of-service state before state lookup
>
> This adds a lower-bound check on llc->state in llc_conn_service() so that a
> socket sitting in LLC_CONN_OUT_OF_SVC is rejected before llc_qualify_conn_ev()
> indexes llc_conn_state_table[state - 1].  A few questions about the new
> check below.
>
> > diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> > index 885a5c33024c3..4a34f240ad2cc 100644
> > --- a/net/llc/llc_conn.c
> > +++ b/net/llc/llc_conn.c
> > @@ -362,7 +362,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: High]
> Does this check actually protect the table access, given that the validated
> value is not latched?  llc_qualify_conn_ev() re-reads llc->state twice to do
> the indexing:
>
> net/llc/llc_conn.c:llc_qualify_conn_ev() {
>         struct llc_conn_state *curr_state =
>                                         &llc_conn_state_table[llc->state - 1];
>         ...
>         for (next_trans = curr_state->transitions +
>                 llc_find_offset(llc->state - 1, ev->type);
>              (*next_trans)->ev; next_trans++) {
>                 if (!((*next_trans)->ev)(sk, skb)) {
>         ...
> }
>
> The store that produces state 0 is done with no socket lock at all:
>
> net/llc/llc_conn.c:llc_sk_free() {
>         llc->state = LLC_CONN_OUT_OF_SVC;
>         /* Stop all (possibly) running timers */
>         llc_sk_stop_all_timers(sk, sync);
>         ...
> }
>
> and llc_ui_release() calls it after dropping the socket lock and before the
> timers are synchronized:
>
> net/llc/af_llc.c:llc_ui_release() {
>         ...
>                 llc_release_incoming_children(sk);
>                 release_sock(sk);
>         ...
>         llc_sk_free(sk, true);
> }
>
> So can this interleaving happen for a socket closed while an ack timer is
> still armed (for example a non-blocking connect() left in
> LLC_CONN_STATE_SETUP, where llc_send_disc() returns early because sk_state
> is not TCP_ESTABLISHED and stops nothing)?
>
> CPU0 (softirq, ack timer)
> llc_conn_ack_tmr_cb() -> llc_conn_tmr_common_cb()
>         bh_lock_sock(sk);          /* excludes nothing here */
>         llc_process_tmr_ev()       /* sees state 2, !sock_owned_by_user() */
>           llc_conn_state_process() -> llc_conn_service()
>                 reads llc->state == 2, passes the new bound check
>
> CPU1 (close(2))
> llc_ui_release() -> release_sock(sk) -> llc_sk_free()
>         llc->state = LLC_CONN_OUT_OF_SVC;     /* unlocked store of 0 */
>
> CPU0 continues into llc_qualify_conn_ev()
>         &llc_conn_state_table[llc->state - 1]   /* now index -1 */
>         llc_find_offset(llc->state - 1, ev->type)
>         (*next_trans)->ev(sk, skb)              /* indirect call */
>
> Since bh_lock_sock() in llc_conn_tmr_common_cb() does not exclude a writer
> that holds no lock, is the negative index and the indirect call through the
> pointer read from before the tables still reachable after this patch?
>
> Would latching the value once, for example
>
>         u8 state = READ_ONCE(llc->state);
>
> and passing that value down to llc_qualify_conn_ev() and llc_find_offset(),
> or moving the llc_sk_free() state store under the socket lock, be a better
> fit for the problem described in the changelog?
>
> >       rc = 0;
> >       trans = llc_qualify_conn_ev(sk, skb);
>
> [Severity: Medium]
> The goto out here leaves rc at its initial value of 1, which was previously
> unreachable for a state of 0.  Where does that 1 end up?
>
> 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;
>         }
>         ...
> }
>
> net/llc/llc_if.c:llc_establish_connection() {
>         ...
>                 rc = llc_conn_state_process(sk, skb);
>         }
> out_put:
>         sock_put(sk);
>         return rc;
> }
>
> net/llc/af_llc.c:llc_ui_connect() {
>         rc = llc_establish_connection(sk, llc->dev->dev_addr,
>                                       addr->sllc_mac, addr->sllc_sap);
>         if (rc) {
>                 sock->state  = SS_UNCONNECTED;
>                 sk->sk_state = TCP_CLOSE;
>                 goto out;
>         }
>         ...
> out:
>         release_sock(sk);
>         return rc;
> }
>
> Can connect(2) then return +1 to userspace on a socket that was just reset
> to SS_UNCONNECTED and TCP_CLOSE?  Callers testing for a negative return
> would read that as success.
>
> Is the printk also a concern, since it is not ratelimited and now fires once
> per event for a state this patch treats as expected?
>
> The sequence I believe reaches it on a still-hashed socket:
>
>   1. socket(AF_LLC, SOCK_STREAM) + bind() leaves llc->state at
>      LLC_CONN_STATE_ADM
>   2. a failing connect() leaves state at ADM with llc->daddr set and the
>      socket still in the SAP list
>   3. any further frame from the peer matches the ADM catch-all transition
>
> 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,
>         .ev_qualifiers = NONE,
>         .ev_actions    = llc_adm_actions_5,
> };
>
>      which sets llc->state to 0
>   4. connect(2) again (sock->state is SS_UNCONNECTED, so it is accepted)
>      reaches the new reject path
>
> Would it be more consistent to translate out-of-service into -ECONNABORTED
> at the llc_if.c entry points?  llc_send_disc() already does an explicit
> check:
>
> net/llc/llc_if.c:llc_send_disc() {
>         if (sk->sk_type != SOCK_STREAM || sk->sk_state != TCP_ESTABLISHED ||
>             llc_sk(sk)->state == LLC_CONN_STATE_ADM ||
>             llc_sk(sk)->state == LLC_CONN_OUT_OF_SVC)
>                 goto out;
>         ...
> }
>
> while llc_establish_connection() has no such check.  On a related note, the
> kernel-doc for llc_build_and_send_pkt() still documents only 0,
> -ECONNABORTED and -EBUSY, although state 0 is filtered there only
> incidentally by llc_data_accept_state().
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but while looking at the callers
> of llc_conn_service() there is an asymmetry in llc_backlog_rcv() worth
> mentioning.  The fallback branch tests the owner socket's state and then
> dispatches on it, while llc_conn_state_process() operates on skb->sk:
>
> net/llc/llc_conn.c:llc_backlog_rcv() {
>         ...
>         } else if (likely(llc->state > 1)) {
>                 rc = llc_conn_rcv(sk, skb);
>         } else {
>                 goto out_kfree_skb;
>         }
>         ...
> }
>
> The two branches above it take bh_lock_sock_nested(newsk) before touching a
> child, this one does not.  For a listener-owned backlog skb whose skb->sk is
> a child, the child's state machine runs without the child's lock, even
> though llc_conn_state_process() documents that the socket lock has to be
> held.
>
> Every child reachable through this branch appears to have already been
> released by llc_release_incoming_sock(), whose state store the new check now
> rejects, so this looks defused for the concretely reachable case.  What
> remains is the publication order in that function:
>
> net/llc/llc_conn.c:llc_release_incoming_sock() {
>         if (atomic_xchg(&llc->incoming_state, LLC_INCOMING_NONE) ==
>             LLC_INCOMING_NONE)
>                 return;
>
>         WRITE_ONCE(llc->state, LLC_CONN_OUT_OF_SVC);
>         ...
> }
>
> Can a drainer in the fallback branch observe incoming_state as
> LLC_INCOMING_NONE while still seeing the child's pre-teardown non-zero
> state, and then run the child's state machine concurrently with
> llc_release_incoming_sock() on another CPU?  Both fields are read there
> without the child's bh lock.  Reaching the branch at all also needs the
> listener's own llc->state above 1, which seems possible only through a
> listen() followed by connect() on the same socket, since llc_ui_listen()
> leaves sock->state at SS_UNCONNECTED and llc_ui_connect() has no TCP_LISTEN
> check.  I could not confirm that all of these hold at the same time, so this
> may not be reachable in practice.
> --
> pw-bot: cr

Thanks for the review. The points on 2/2 look valid, especially the
unlatched llc->state check versus llc_sk_free(), and the connect(2)
return of +1 / unratelimited printk path.

I'll drop 2/2. Kees's net-next series covers the same out-of-service
lookup more completely, including the state machine guard and the
-ECONNABORTED translation:

  https://lore.kernel.org/all/20260901210300.i.590-kees@xxxxxxxxxx/

I'll leave that bug to his series.

1/2 is a separate listener child-socket leak and is not addressed
there. I'll reroll that alone as v7.

The remaining note about llc_backlog_rcv() / llc_release_incoming_sock()
belongs to 1/2; I'll look at the publication order there in the reroll.

Thanks,
Zihan