Re: [PATCH net v13 2/2] llc: create listener children only for SABME

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Critical] In llc_conn_rcv_existing_sabme() (net/llc/llc_conn.c), the
  patch calls bh_lock_sock_nested(newsk) on whatever socket…

--

llc: create listener children only for SABME

With this patch, an LLC listener creates a passive-open child only for a
SABME command. It answers DISC and other P=1 commands with DM and drops
all other frames. A duplicate SABME that matches an existing connection
now goes to that child through the new llc_conn_rcv_existing_sabme()
helper instead of being dropped, and llc_backlog_rcv() checks backlog
packets more strictly.

> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 349a7c5b9bb1a..79c29c57cd471 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c

[ ... ]

> @@ -801,7 +801,64 @@ static struct sock *llc_create_incoming_sock(struct sock *sk,
>  	return newsk;
>  }
>  
> -static int llc_conn_rcv_listener(struct sock *sk, struct sk_buff *skb,
> +/* The listener is locked and bottom halves are disabled. */
> +static int llc_conn_rcv_existing_sabme(struct sock *listener,
> +				       struct sock *newsk,
> +				       struct sk_buff *skb)
> +{
> +	struct llc_sock *llc = llc_sk(newsk);
> +	int incoming_state;
> +	int rc = 0;
> +
> +	bh_lock_sock_nested(newsk);

[ ... ]

> @@ -812,8 +869,9 @@ static int llc_conn_rcv_listener(struct sock *sk, struct sk_buff *skb,
>  	newsk = __llc_lookup_established(llc_sk(sk)->sap, saddr, daddr,
>  					 dev_net(skb->dev));
>  	if (newsk) {
> -		sock_put(newsk);
> -		goto drop;
> +		rc = llc_conn_rcv_existing_sabme(sk, newsk, skb);
> +		local_bh_enable();
> +		return rc;
>  	}

[Severity: Critical]

Can __llc_lookup_established() return the listener itself here?

Listeners are hashed in the same sap->sk_laddr_hash, and
llc_estab_match() does not check sk_state:

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);
}

A listener that was bound but never connected has an all-zero daddr.
llc_ui_bind() relies on that zero daddr in its clash check.

A socket whose connect() failed keeps llc->daddr set to the peer, because
llc_ui_connect() fills it in before the attempt. llc_ui_listen() only
requires SS_UNCONNECTED, so that socket can then become a listener that
still matches the peer.

Suppose a peer sends one SABME command to the listener's MAC and SAP,
using SA 00:00:00:00:00:00 and SSAP 0. The softirq path then looks like
this:

llc_conn_handler()
  __llc_lookup() returns the listener
  bh_lock_sock(sk)
  sk_state is TCP_LISTEN, !newsk, frame is SABME, not owned by user
  llc_conn_rcv_sabme(sk, skb, &saddr, &daddr)
    __llc_lookup_established() returns the same listener
    llc_conn_rcv_existing_sabme(sk, sk, skb)
      bh_lock_sock_nested(sk)

Wouldn't this CPU spin forever on sk->sk_lock.slock, which it already
holds? SINGLE_DEPTH_NESTING only changes the lockdep subclass. It does not
stop a real recursive acquisition.

Before this patch, a lookup hit here just called sock_put(newsk) and
dropped the skb without taking any child lock.

The deferred path does not deadlock, because __release_sock() has already
dropped the slock. In that case the skb is queued again on the listener as
LLC_PACKET, and llc_backlog_rcv() drops it because the listener's
llc->state is ADM.

Should llc_conn_rcv_sabme() drop the frame when newsk == sk (or when
newsk->sk_state == TCP_LISTEN) before taking the child lock? The
established match could also skip listening sockets instead.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790688018.git.zihanx%40nebusec.ai