mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net] llc: stop connection timers before dropping sap ref on release
@ 2026-09-16  7:41 Miguel García Román
  2026-09-16 10:47 ` [PATCH net v2] " Miguel Garcia
  2026-09-20  7:58 ` [PATCH net] " netdev-bot+sashiko
  0 siblings, 2 replies; 4+ messages in thread
From: Miguel García Román @ 2026-09-16  7:41 UTC (permalink / raw)
  To: netdev
  Cc: davem, edumazet, kuba, pabeni, horms, linux-kernel,
	syzbot+44efda9647c52be29d9c, Miguel García Román

syzbot reported a KASAN slab-use-after-free in
llc_conn_ac_send_sabme_cmd_p_set_x() when a connection timer fires
after the associated llc_sap has already been freed via RCU.

llc_ui_release() currently does:

  llc_sap_hold(sap);
  llc_sap_remove_socket(...);   /* may drop last socket's sap ref */
  release_sock(sk);
  llc_sap_put(sap);             /* last ref -> llc_sap_close -> kfree_rcu */
  ...
  llc_sk_free(sk);              /* timer_delete_sync() only here */

Connection timers (ack / P / REJ / busy) are embedded in llc_sock and
their callbacks run llc_process_tmr_ev() -> state machine actions that
dereference llc->sap (e.g. sap->laddr.lsap). Those timers are only
synchronously cancelled in llc_sk_free(), which runs *after* the sap
reference is dropped. If this was the last socket on the sap, the sap
can be RCU-freed while a timer is still pending or running, and the
softirq timer path UAFs the freed sap.

llc_ui_release() already keeps an extra sap reference across
release_sock() so backlog processing can still use the sap. Extend that
window: after release_sock(), synchronously stop all connection timers
while the sap is still held, then drop the sap reference.
timer_delete_sync() must not run under lock_sock() (timer callbacks take
bh_lock_sock()), so stopping after release_sock() is required.

llc_sk_free() still stops timers again; a second sync delete is
harmless.

Fixes: f7e43672683b ("llc: hold llc_sap before release_sock()")
Reported-by: syzbot+44efda9647c52be29d9c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=44efda9647c52be29d9c
Signed-off-by: Miguel García Román <miguelgarciaroman8@gmail.com>
---
 net/llc/af_llc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index b0447c33d..4fc397fad 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -215,6 +215,14 @@ static int llc_ui_release(struct socket *sock)
 		llc_sap_hold(sap);
 		llc_sap_remove_socket(llc->sap, sk);
 		release_sock(sk);
+		/*
+		 * Timers dereference llc->sap.  Cancel them while the sap is
+		 * still held; llc_sk_free() runs after the final sap put and
+		 * would otherwise race with kfree_rcu(sap).  Must run after
+		 * release_sock() to avoid deadlock with bh_lock_sock() in the
+		 * timer callbacks.
+		 */
+		llc_sk_stop_all_timers(sk, true);
 		llc_sap_put(sap);
 	} else {
 		release_sock(sk);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH net v2] llc: stop connection timers before dropping sap ref on release
  2026-09-16  7:41 [PATCH net] llc: stop connection timers before dropping sap ref on release Miguel García Román
@ 2026-09-16 10:47 ` Miguel Garcia
  2026-09-20 11:19   ` netdev-bot+sashiko
  2026-09-20  7:58 ` [PATCH net] " netdev-bot+sashiko
  1 sibling, 1 reply; 4+ messages in thread
From: Miguel Garcia @ 2026-09-16 10:47 UTC (permalink / raw)
  To: netdev
  Cc: davem, edumazet, kuba, pabeni, horms, linux-kernel,
	syzbot+44efda9647c52be29d9c, Miguel García Román

From: Miguel García Román <miguelgarciaroman8@gmail.com>

syzbot reported a KASAN slab-use-after-free in
llc_conn_ac_send_sabme_cmd_p_set_x() when a connection timer fires
after the associated llc_sap has already been freed via RCU.

llc_ui_release() currently does:

  llc_sap_hold(sap);
  llc_sap_remove_socket(...);   /* may drop last socket's sap ref */
  release_sock(sk);
  llc_sap_put(sap);             /* last ref -> llc_sap_close -> kfree_rcu */
  ...
  llc_sk_free(sk);              /* timer_delete_sync() only here */

Connection timers (ack / P / REJ / busy) are embedded in llc_sock and
their callbacks run llc_process_tmr_ev() -> state machine actions that
dereference llc->sap (e.g. sap->laddr.lsap). Those timers are only
synchronously cancelled in llc_sk_free(), which runs *after* the sap
reference is dropped. If this was the last socket on the sap, the sap
can be RCU-freed while a timer is still pending or running, and the
softirq timer path UAFs the freed sap.

llc_ui_release() already keeps an extra sap reference across
release_sock() so backlog processing can still use the sap. Extend that
window: after release_sock(), synchronously stop all connection timers
while the sap is still held, then drop the sap reference.
timer_delete_sync() must not run under lock_sock() (timer callbacks take
bh_lock_sock()), so stopping after release_sock() is required.

llc_sk_free() still stops timers again; a second sync delete is
harmless.

Fixes: 9708d2b5b7c6 ("llc: avoid blocking in llc_sap_close()")
Reported-by: syzbot+44efda9647c52be29d9c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=44efda9647c52be29d9c
Signed-off-by: Miguel García Román <miguelgarciaroman8@gmail.com>
---
v2: Correct Fixes: tag to 9708d2b5b7c6 ("llc: avoid blocking in
    llc_sap_close()"), which introduced kfree_rcu() for sap teardown
    and opened the timer vs RCU-free race. v1 used f7e43672683b.
    No code change.

 net/llc/af_llc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index b0447c33dbf0..4fc397fad4d2 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -215,6 +215,14 @@ static int llc_ui_release(struct socket *sock)
 		llc_sap_hold(sap);
 		llc_sap_remove_socket(llc->sap, sk);
 		release_sock(sk);
+		/*
+		 * Timers dereference llc->sap.  Cancel them while the sap is
+		 * still held; llc_sk_free() runs after the final sap put and
+		 * would otherwise race with kfree_rcu(sap).  Must run after
+		 * release_sock() to avoid deadlock with bh_lock_sock() in the
+		 * timer callbacks.
+		 */
+		llc_sk_stop_all_timers(sk, true);
 		llc_sap_put(sap);
 	} else {
 		release_sock(sk);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] llc: stop connection timers before dropping sap ref on release
  2026-09-16  7:41 [PATCH net] llc: stop connection timers before dropping sap ref on release Miguel García Román
  2026-09-16 10:47 ` [PATCH net v2] " Miguel Garcia
@ 2026-09-20  7:58 ` netdev-bot+sashiko
  1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-20  7:58 UTC (permalink / raw)
  To: miguelgarciaroman8
  Cc: netdev, davem, edumazet, kuba, pabeni, horms, linux-kernel,
	syzbot+44efda9647c52be29d9c

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the new `llc_sk_stop_all_timers(sk, true)` in
  `llc_ui_release()` (net/llc/af_llc.c:225) cannot guarantee that no…
- [Low] The comment added by this patch (net/llc/af_llc.c:221-223) and
  the corresponding commit-message sentence ("timer_delete_sync() must…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is in the
  code the patch touches and is a hard out-of-bounds access:…

--

llc: stop connection timers before dropping sap ref on release

The patch adds a synchronous connection-timer cancel in llc_ui_release()
after release_sock() and before the final llc_sap_put(), so that the ack /
P / REJ / busy timers are stopped while the sap reference is still held.
It targets a syzbot slab-use-after-free of the RCU-freed llc_sap from the
timer softirq path.

Closes: https://syzkaller.appspot.com/bug?extid=44efda9647c52be29d9c

> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> index b0447c33dbf09..4fc397fad4d2e 100644
> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
> @@ -215,6 +215,14 @@ static int llc_ui_release(struct socket *sock)
>  		llc_sap_hold(sap);
>  		llc_sap_remove_socket(llc->sap, sk);
>  		release_sock(sk);
> +		/*
> +		 * Timers dereference llc->sap.  Cancel them while the sap is
> +		 * still held; llc_sk_free() runs after the final sap put and
> +		 * would otherwise race with kfree_rcu(sap).  Must run after
> +		 * release_sock() to avoid deadlock with bh_lock_sock() in the
> +		 * timer callbacks.
> +		 */

[Severity: Low]
This isn't a bug, but is the stated deadlock rationale accurate?  The same
claim appears in the changelog:

    timer_delete_sync() must not run under lock_sock() (timer callbacks take
    bh_lock_sock()), so stopping after release_sock() is required.

lock_sock() does not hold sk->sk_lock.slock across its critical section.
lock_sock_nested() in net/core/sock.c takes the slock only to set the
ownership flag and drops it again:

	spin_lock_bh(&sk->sk_lock.slock);
	if (unlikely(sock_owned_by_user_nocheck(sk)))
		__lock_sock(sk);
	sk->sk_lock.owned = 1;
	spin_unlock_bh(&sk->sk_lock.slock);

Since bh_lock_sock() is just spin_lock(&((__sk)->sk_lock.slock)), a timer
callback can take it while another thread owns the socket lock, and
llc_process_tmr_ev() then defers rather than blocking:

net/llc/llc_c_ac.c:llc_process_tmr_ev() {
	...
		if (!sock_owned_by_user(sk))
			llc_conn_state_process(sk, skb);
		else {
			llc_set_backlog_type(skb, LLC_EVENT);
			__sk_add_backlog(sk, skb);
		}
	...
}

So timer_delete_sync() under lock_sock() would not deadlock on
bh_lock_sock().  Placing the cancel after release_sock() still looks
desirable for a different reason (a callback firing while the socket is
owned pushes an LLC_EVENT onto the backlog, and release_sock() processing
of that event can re-arm timers).  Could the comment and changelog be
reworded to describe that instead?

> +		llc_sk_stop_all_timers(sk, true);
>  		llc_sap_put(sap);

[Severity: High]
Can a timer still be pending here when llc_sap_put() drops the last
reference and queues kfree_rcu(sap)?

Two things differ from the other caller of this helper.  llc_sk_free()
marks the connection dead first:

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, true);
	...
}

and LLC_CONN_OUT_OF_SVC is the only thing that stops an expiring timer from
re-entering the state machine:

net/llc/llc_c_ac.c:llc_process_tmr_ev() {
	if (llc_sk(sk)->state == LLC_CONN_OUT_OF_SVC) {
	...
}

The new call site does not set that state, and the helper uses
timer_delete_sync() rather than timer_shutdown_sync(), so a mod_timer()
from a callback is not neutralised.

The first re-arm path is inside the helper itself, which deletes in a fixed
order:

net/llc/llc_conn.c:llc_sk_stop_all_timers() {
	if (sync) {
		timer_delete_sync(&llc->pf_cycle_timer.timer);
		timer_delete_sync(&llc->ack_timer.timer);
		timer_delete_sync(&llc->rej_sent_timer.timer);
		timer_delete_sync(&llc->busy_state_timer.timer);
	...
}

An ack-timer callback running concurrently takes bh_lock_sock() (free,
because release_sock() already ran), sees state != LLC_CONN_OUT_OF_SVC, and
runs the NORMAL state transition whose action list contains:

net/llc/llc_c_st.c:llc_normal_actions_20a[] {
	[3] = llc_conn_ac_start_p_timer,
}

net/llc/llc_c_ac.c:llc_conn_ac_start_p_timer() {
	mod_timer(&llc->pf_cycle_timer.timer,
		  jiffies + llc->pf_cycle_timer.expire);
}

pf_cycle was already deleted first, so timer_delete_sync(&ack) only waits
for that callback and returns with pf_cycle armed again.  The symmetric case
exists in the other direction, where busy-timer expiry (deleted last)
re-arms the ack timer (deleted second):

net/llc/llc_c_st.c:llc_common_actions_11d[] {
	[1] = llc_conn_ac_start_ack_timer,
}

The second re-arm path is an in-flight receive softirq.
llc_sap_remove_socket() only unhashes the socket; a lookup that already
succeeded holds a socket reference via refcount_inc_not_zero() under
rcu_read_lock(), and after release_sock() the socket is no longer owned:

net/llc/llc_conn.c:llc_conn_handler() {
	...
	if (!sock_owned_by_user(sk))
		llc_conn_rcv(sk, skb);
	...
}

llc_conn_rcv() -> llc_conn_state_process() can mod_timer() any of the four
timers after the new cancel pass has completed.

Both look reachable when the connection is still active at close time, for
example when llc_send_disc() returns non-zero:

net/llc/llc_if.c:llc_send_disc() {
	...
	skb = alloc_skb(0, GFP_ATOMIC);
	if (!skb)
		goto out;
	...
}

or when sk->sk_state != TCP_ESTABLISHED, e.g. a connect that timed out and
left the socket in setup state with the ack timer armed.  The socket is not
SOCK_ZAPPED in that case, so this branch is taken with live timers and a
live state machine.

The changelog says:

    llc_sk_free() still stops timers again; a second sync delete is
    harmless.

Can that second pass restore the guarantee?  It runs after the final sap
put, and its llc->state = LLC_CONN_OUT_OF_SVC is a plain store that a
concurrent state machine can overwrite via llc->state = trans->next_state
in llc_conn_service().  If so, the original UAF (a timer action touching
sap->laddr.lsap in llc_conn_ac_send_sabme_cmd_p_set_x() /
llc_conn_ac_send_rr_cmd_p_set_1() after the sap was RCU-freed) still looks
reachable.

Would it work better to make re-arming impossible before the sap reference
is dropped, by marking the connection out of service under the socket lock
and/or using timer_shutdown_sync(), or alternatively to keep the extra sap
reference until after llc_sk_free()?

As a smaller point, the trailing plain stores in the helper:

	llc->ack_must_be_send = 0;
	llc->ack_pf = 0;

now execute with the socket unlocked while it is still reachable from an
in-flight softirq.  Is that a new data race on connection state?

[Severity: High]
This isn't a bug introduced by this patch, but it is in the teardown code
the patch touches and interacts with any fix for the above, so it seems
worth raising.

llc_conn_service() validates only the upper bound of llc->state:

net/llc/llc_conn.c:llc_conn_service() {
	if (llc->state > NBR_CONN_STATES)
		goto out;
	...
}

and llc_qualify_conn_ev() then indexes with state - 1:

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++) {
	...
}

With llc->state == LLC_CONN_OUT_OF_SVC (which is 0) both become index -1,
reading the memory before llc_conn_state_table[] and before
llc_offset_table[][].  The resulting transitions pointer is then walked and
its members are invoked as function pointers (ev, ev_qualifiers,
ev_actions).

Is state 0 reachable from received frames alone?  llc_conn_handler()
creates and hashes an incoming socket for a frame that reaches a listener,
then runs the state machine on it.  In ADM state any frame that is not
SABME/DISC/xxx_cmd_Pbit_1 matches:

net/llc/llc_c_ev.c:llc_conn_ev_rx_any_frame() {
	return 0;
}

which selects the transition whose next state is 0:

net/llc/llc_c_st.c:llc_adm_state_trans_5 {
	.ev	       = llc_conn_ev_rx_any_frame,
	.next_state    = LLC_CONN_OUT_OF_SVC,
}

llc_conn_service() writes that into llc->state.  The socket stays hashed,
and the established lookup filters only on netns and addresses:

net/llc/llc_conn.c:llc_estab_match() {
	return net_eq(sock_net(sk), net) &&
		llc->laddr.lsap == laddr->lsap &&
		llc->daddr.lsap == daddr->lsap &&
		ether_addr_equal(llc->laddr.mac, laddr->mac) &&
		ether_addr_equal(llc->daddr.mac, daddr->mac);
}

so a second frame from the same source MAC/SSAP reaches
__llc_lookup_established() -> llc_conn_rcv() -> llc_conn_state_process() ->
llc_conn_service() with state 0.  Unlike llc_backlog_rcv() (which checks
llc->state > 1) and llc_process_tmr_ev() (explicit OUT_OF_SVC check), the
direct receive path has no state guard.

llc_sk_free() is the second producer of state 0, and it runs while an
in-flight receive softirq holding a pre-removal socket reference can still
drive that unguarded path.  Should llc_conn_service() also reject state 0,
particularly if the timer fix ends up marking the connection out of service
earlier while it is still hashed?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916074158.3375909-1-miguelgarciaroman8%40gmail.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v2] llc: stop connection timers before dropping sap ref on release
  2026-09-16 10:47 ` [PATCH net v2] " Miguel Garcia
@ 2026-09-20 11:19   ` netdev-bot+sashiko
  0 siblings, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-20 11:19 UTC (permalink / raw)
  To: miguelgarciaroman8
  Cc: netdev, davem, edumazet, kuba, pabeni, horms, linux-kernel,
	syzbot+44efda9647c52be29d9c

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the new `llc_sk_stop_all_timers(sk, true)` in
  `llc_ui_release()` (net/llc/af_llc.c:225) does not establish the…
- [Low] The new comment in `llc_ui_release()` (net/llc/af_llc.c:218-224)
  and the commit message both state that `timer_delete_sync()` "must not…
- [Low] `llc_sk_stop_all_timers()` ends with the plain stores
  `llc->ack_must_be_send = 0; llc->ack_pf = 0;`…

--

llc: stop connection timers before dropping sap ref on release

This adds a synchronous cancellation of the four llc_sock connection timers
in llc_ui_release(), placed after release_sock() and before the extra sap
reference is dropped, so that the timer callbacks cannot dereference an
already RCU-freed llc_sap.  A few questions about the new window and the
new comment below.

Closes: https://syzkaller.appspot.com/bug?extid=44efda9647c52be29d9c

> diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
> index b0447c33dbf09..4fc397fad4d2e 100644
> --- a/net/llc/af_llc.c
> +++ b/net/llc/af_llc.c
> @@ -215,6 +215,14 @@ static int llc_ui_release(struct socket *sock)
>  		llc_sap_hold(sap);
>  		llc_sap_remove_socket(llc->sap, sk);
>  		release_sock(sk);
> +		/*
> +		 * Timers dereference llc->sap.  Cancel them while the sap is
> +		 * still held; llc_sk_free() runs after the final sap put and
> +		 * would otherwise race with kfree_rcu(sap).  Must run after
> +		 * release_sock() to avoid deadlock with bh_lock_sock() in the
> +		 * timer callbacks.
> +		 */

[Severity: Low]
Is the deadlock rationale in this comment accurate?

llc_conn_tmr_common_cb() in net/llc/llc_c_ac.c takes only bh_lock_sock():

	bh_lock_sock(sk);
	if (skb) {
		...
		llc_process_tmr_ev(sk, skb);
	}
	bh_unlock_sock(sk);

and bh_lock_sock() is spin_lock(&sk->sk_lock.slock), while
lock_sock_nested() releases slock before it returns and leaves only
sk->sk_lock.owned set:

	spin_lock_bh(&sk->sk_lock.slock);
	if (unlikely(sock_owned_by_user_nocheck(sk)))
		__lock_sock(sk);
	sk->sk_lock.owned = 1;
	spin_unlock_bh(&sk->sk_lock.slock);

Since a process-context owner of the socket does not hold slock, wouldn't a
timer_delete_sync() called under lock_sock() let the callback acquire slock,
run to completion and return?

What does change across release_sock() is sock_owned_by_user(): under
lock_sock() llc_process_tmr_ev() queues the event via __sk_add_backlog()
instead of running llc_conn_state_process() inline.

Could this comment and the matching commit message sentence,
"timer_delete_sync() must not run under lock_sock() (timer callbacks take
bh_lock_sock()), so stopping after release_sock() is required", be reworded
to describe that difference instead?  As written they suggest that stopping
timers or marking state while still holding the socket lock is forbidden.

> +		llc_sk_stop_all_timers(sk, true);
>  		llc_sap_put(sap);

[Severity: High]
Does this establish the invariant that the following llc_sap_put() needs,
namely that no timer can be pending any more?

llc_sk_stop_all_timers() makes a single fixed-order pass:

net/llc/llc_conn.c:llc_sk_stop_all_timers() {
	if (sync) {
		timer_delete_sync(&llc->pf_cycle_timer.timer);
		timer_delete_sync(&llc->ack_timer.timer);
		timer_delete_sync(&llc->rej_sent_timer.timer);
		timer_delete_sync(&llc->busy_state_timer.timer);
	...
}

timer_delete_sync() waits only for that one timer's callback and has no
shutdown semantics.  Can a later-deleted timer's callback re-arm an
earlier-deleted one?

llc_conn_rej_tmr_cb() -> llc_conn_tmr_common_cb() -> llc_process_tmr_ev()
-> llc_conn_state_process()

and llc_common_actions_11c / llc_common_actions_11d in net/llc/llc_c_st.c
contain:

	[1] = llc_conn_ac_start_ack_timer,

which is an unconditional re-arm in net/llc/llc_c_ac.c:

	mod_timer(&llc->ack_timer.timer, jiffies + llc->ack_timer.expire);

The same holds for llc_normal_actions_20a / llc_normal_actions_20b, whose

	[3] = llc_conn_ac_start_p_timer,

re-arms pf_cycle_timer, the timer deleted first in the pass.

There is a second way into the same window.  llc_conn_handler() takes the sk
reference under RCU, and its only re-validation is:

	if (llc_sk(rc)->sap != sap)
		...

llc->sap is never cleared during release, so a softirq whose lookup predates
llc_sap_remove_socket() still passes that check.  release_sock() has already
run, so sock_owned_by_user(sk) is false and the frame goes straight into
llc_conn_rcv() -> llc_conn_state_process(), whose actions mod_timer() as
well.  Can that land after the last timer_delete_sync() returns and before
llc_sap_put()?

Is anything suppressing the state machine in this window?
llc_process_tmr_ev() only discards events when the connection is already out
of service:

net/llc/llc_c_ac.c:llc_process_tmr_ev() {
	if (llc_sk(sk)->state == LLC_CONN_OUT_OF_SVC) {
		...
		kfree_skb(skb);
	} else {
		if (!sock_owned_by_user(sk))
			llc_conn_state_process(sk, skb);
	...
}

and that state is set only later, after llc_sap_put() has run:

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, true);
	...
}

If llc_sap_put() here drops the last reference, llc_sap_close() does
list_del_rcu() + kfree_rcu(sap, rcu).  A pending timer holds no RCU
read-side protection, so if the releasing task is delayed between
llc_sap_put() and llc_sk_free(), can the re-armed timer still dereference
the freed sap (sap->laddr.lsap in llc_conn_ac_send_sabme_cmd_p_set_x()),
which is the reported signature?

Reaching release with an armed state machine also looks easy:
llc_send_disc() in net/llc/llc_if.c returns without any state transition
when the atomic allocation fails:

	skb = alloc_skb(0, GFP_ATOMIC);
	if (!skb)
		goto out;

Would using timer_shutdown_sync() for all four timers on this teardown path,
setting llc->state = LLC_CONN_OUT_OF_SVC while the socket lock is still
held, and/or keeping the extra sap reference until after llc_sk_free(sk)
close the window instead of narrowing it?

[Severity: Low]
Can this call race on llc->ack_must_be_send and llc->ack_pf?
llc_sk_stop_all_timers() ends with two plain stores:

	llc->ack_must_be_send = 0;
	llc->ack_pf = 0;

The existing callers are serialized: llc_sk_reset() runs from state-machine
actions with the socket owned or BH-locked, and llc_sk_free() runs after
sock_orphan() and sock->sk = NULL, when nothing can be dispatched to the
socket.  This new call site holds neither lock_sock() nor bh_lock_sock().

Meanwhile a softirq that took the sk reference in llc_conn_handler() before
the unhash can run llc_conn_state_process() under bh_lock_sock() and
read/write the same two fields, for example through
llc_conn_ac_rst_sendack_flag() and llc_conn_ac_send_ack_if_needed().

Would KCSAN flag this pair of accesses, and can the concurrent state machine
set the flags again right after they are cleared here?

>  	} else {
>  		release_sock(sk);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916104735.3565609-1-miguelgarciaroman8%40gmail.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-09-20 11:20 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-16  7:41 [PATCH net] llc: stop connection timers before dropping sap ref on release Miguel García Román
2026-09-16 10:47 ` [PATCH net v2] " Miguel Garcia
2026-09-20 11:19   ` netdev-bot+sashiko
2026-09-20  7:58 ` [PATCH net] " netdev-bot+sashiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®