Re: [PATCH net 1/1] llc: conn: drop out-of-service state in llc_conn_service

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

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://sashiko.dev
---
llc: conn: drop out-of-service state in llc_conn_service

The commit adds a check for LLC_CONN_OUT_OF_SVC in llc_conn_service() to
avoid indexing state tables with an invalid state. It returns success for
this state to ignore repeated packets to a stale reachable socket.

> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 5c0ac243b248f..715d5a3140716 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -360,6 +360,10 @@ 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 == LLC_CONN_OUT_OF_SVC) {
> +		rc = 0;
> +		goto out;
> +	}

Can this check still race with llc_sk_free() setting llc->state locklessly?

llc_ui_release() drops the socket lock via release_sock(sk) and calls
llc_sk_free(sk), which sets llc->state = LLC_CONN_OUT_OF_SVC (0) without
holding a lock.

Concurrently, a packet processed by llc_conn_handler() can acquire
bh_lock_sock(sk) and call llc_conn_service(). Since llc->state is checked
without READ_ONCE(), could the compiler emit multiple loads?

If llc->state is > 0 during this check, it passes. If CPU 1 then locklessly
changes llc->state to 0, the subsequent call to llc_qualify_conn_ev() could
read llc->state again from memory as 0.

This would evaluate llc->state - 1 as 255, causing an out-of-bounds array
access on llc_conn_state_table[255]. The garbage data could be interpreted
as curr_state->transitions, leading to unconditional dereferences and a
kernel panic. Does this lockless concurrency need to be addressed to
properly fix the issue?

Additionally, this highlights a potential timer race during socket teardown.
llc_sk_free() stops timers via llc_sk_stop_all_timers(sk, true).

CPU 1 starts socket destruction in llc_ui_release(), calling release_sock(sk)
followed by llc_sk_free(sk).

Concurrently, CPU 2 (finding the socket under RCU via __llc_lookup) receives
a packet in llc_conn_handler(), acquires bh_lock_sock(sk), and since
!sock_owned_by_user(sk) is now true, directly processes the packet.

If CPU 2 processes state transitions and triggers an action (like
llc_conn_ac_start_ack_timer) that calls mod_timer(), it could re-arm the
timer AFTER CPU 1 has stopped it, because CPU 1's timer_delete_sync() does
not wait for the packet handler.

When CPU 1 drops its reference, and CPU 2 finishes processing and calls
sock_put(sk), the socket memory is scheduled to be freed. Will the timer
re-armed by CPU 2 fire later on the freed socket memory, causing a
use-after-free regression?

>  	if (llc->state > NBR_CONN_STATES)
>  		goto out;
>  	rc = 0;