Re: [PATCH net] net/x25: don't call lock_sock() from softirq in x25_kill_by_neigh()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 (1) · 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).

- [Medium] The patch adds a deferred-kill state.
- [Low] x25_make_new() copies the parent's whole flags word into the
  child (`x25->flags = ox25->flags;`) and clears only X25_INTERRUPT_FLAG.

Pre-existing issues:
- [High] x25_sendmsg() checks `if (!x25->neighbour)` under lock_sock().
- [Medium] On NETDEV_DOWN, x25_device_event() runs under RTNL and calls
  the new x25_kill_by_neigh_sync().

--

net/x25: don't call lock_sock() from softirq in x25_kill_by_neigh()

x25_kill_by_neigh() can be reached from softirq. This patch replaces its
lock_sock() with bh_lock_sock(). Sockets not owned by user are
disconnected right away. Owned sockets are marked with X25_KILL_FLAG, and
a new x25_release_cb() disconnects them on release_sock(). NETDEV_DOWN
also runs a sleeping x25_kill_by_neigh_sync() sweep.

> diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
> index 033e7d059f58c..4f9ca22231ac7 100644
> --- a/net/x25/af_x25.c
> +++ b/net/x25/af_x25.c
> @@ -200,6 +200,32 @@ static void x25_remove_socket(struct sock *sk)
>  	write_unlock_bh(&x25_list_lock);
>  }
>  
> +/*
> + * Process context only: wait for owners that x25_kill_by_neigh() deferred,
> + * so no socket still uses nb once the device goes away.
> + */
> +static void x25_kill_by_neigh_sync(struct x25_neigh *nb)
> +{
> +	struct sock *s;
> +
> +again:
> +	write_lock_bh(&x25_list_lock);
> +
> +	sk_for_each(s, &x25_list) {
> +		if (x25_sk(s)->neighbour == nb) {
> +			sock_hold(s);
> +			write_unlock_bh(&x25_list_lock);
> +			lock_sock(s);

[Severity: Medium]
This isn't a bug introduced by this patch, but can this sweep hold RTNL
indefinitely?

x25_device_event() handles NETDEV_DOWN with RTNL held and calls
x25_kill_by_neigh_sync(). That function sleeps in lock_sock() for each
socket on nb. Meanwhile, x25_sendmsg() holds the socket lock across the
user copy:

net/x25/af_x25.c:x25_sendmsg() {
    ...
	rc = memcpy_from_msg(skb_transport_header(skb), msg, len);
    ...
}

If the user buffer is backed by userfaultfd or FUSE, the user can stall
that copy for as long as they like. A later "ip link set <dev> down"
would then block here with RTNL held.

The old x25_kill_by_neigh() made the same lock_sock() call from the same
notifier. The commit message keeps this on purpose ("keep waiting for
socket owners there as before"), so the new helper carries it forward.

> +			if (x25_sk(s)->neighbour == nb)
> +				x25_disconnect(s, ENETUNREACH, 0, 0);

[ ... ]

> @@ -495,10 +522,20 @@ static int x25_listen(struct socket *sock, int backlog)
>  	return rc;
>  }
>  
> +/* Finish a link teardown that hit while the socket was owned by user. */
> +static void x25_release_cb(struct sock *sk)
> +{
> +	struct x25_sock *x25 = x25_sk(sk);
> +
> +	if (test_and_clear_bit(X25_KILL_FLAG, &x25->flags) && x25->neighbour)
> +		x25_disconnect(sk, ENETUNREACH, 0, 0);

[Severity: Low]
Can X25_KILL_FLAG be inherited by a child socket?

x25_make_new() copies the whole flags word from the listener and clears
only X25_INTERRUPT_FLAG:

net/x25/af_x25.c:x25_make_new() {
    ...
	x25->flags	= ox25->flags;
    ...
	clear_bit(X25_INTERRUPT_FLAG, &x25->flags);
    ...
}

x25_connect() does not reject TCP_LISTEN sockets, and it assigns
x25->neighbour while it owns the socket. A link kill during connect()
can therefore set X25_KILL_FLAG on a socket that was listening.

x25_rx_call_request() clones the listener without taking its socket
lock. A child created in that window would inherit the bit, even though
its own neighbour is healthy.

x25_release_cb() does not check which neighbour caused the kill. The
child's first release_sock() after accept() would then disconnect its
call with ENETUNREACH, and no CLEAR is sent to the peer.

The sequence is contrived. Should x25_make_new() also clear
X25_KILL_FLAG?

> +}
> +

[ ... ]

> @@ -1764,6 +1801,23 @@ static struct notifier_block x25_dev_notifier = {
>  	.notifier_call = x25_device_event,
>  };
>  
> +/* May run in softirq, so lock_sock() is not an option. */
> +static void x25_kill_sock(struct sock *sk, struct x25_neigh *nb)
> +{
> +	struct x25_sock *x25 = x25_sk(sk);
> +
> +	local_bh_disable();
> +	bh_lock_sock(sk);
> +	if (x25->neighbour == nb) {
> +		if (sock_owned_by_user(sk))
> +			set_bit(X25_KILL_FLAG, &x25->flags);

[Severity: Medium]
While the flag is set, x25->lci and x25->neighbour stay unchanged until
x25_release_cb() runs. __x25_find_socket() matches on only those two
fields:

	if (x25_sk(s)->lci == lci && x25_sk(s)->neighbour == nb) {

Could a new incoming call on the same LCI end up on the stale socket's
backlog?

In X25_LINK_STATE_3, x25_link_control() acknowledges a restart as soon
as x25_kill_by_neigh() returns:

		case X25_LINK_STATE_3:
			/* clear existing virtual calls */
			x25_kill_by_neigh(nb);

			x25_transmit_restart_confirmation(nb);

>From then on the peer treats every LCI as free. Suppose it sends a
CALL_REQUEST on an LCI still held by a flagged, user-owned socket.
x25_receive_data() finds that socket before it checks the frame type:

	if ((sk = x25_find_socket(lci, nb)) != NULL) {
		...
			queued = !sk_add_backlog(sk, skb, READ_ONCE(sk->sk_rcvbuf));
	...
	if (frametype == X25_CALL_REQUEST)
		return x25_rx_call_request(skb, nb, lci);

The call therefore never reaches x25_rx_call_request().

release_sock() replays the backlog before it calls release_cb, so the
old connection's state machine handles the CALL_REQUEST. In
X25_STATE_1, x25_state1_machine() treats it as a call collision and
clears the peer's new call:

	case X25_CALL_REQUEST:
		/* call collision */
		...
		x25_write_internal(sk, X25_CLEAR_REQUEST);
		x25_disconnect(sk, EISCONN, 0x01, 0x48);

In X25_STATE_3 the frame is dropped as unknown, and x25_release_cb()
then disconnects the socket.

Before 7781607938c8 the disconnect was immediate. The lock_sock()
version waited for the owner. Either way, the old (lci, nb) binding was
gone before the restart was answered.

This window also seems to contradict the note after the commit message
that a link that comes back quickly cannot have stale state kill
freshly connected calls.

Should the lookup skip sockets with X25_KILL_FLAG set? Alternatively,
the lci/neighbour binding could be dropped at kill time.

> +		else
> +			x25_disconnect(sk, ENETUNREACH, 0, 0);

[Severity: High]
This is a pre-existing issue, but the immediate disconnect here still
seems to race with x25_sendmsg().

x25_sendmsg() checks x25->neighbour under lock_sock(). It then drops
the lock around the skb allocation, and afterwards checks only skb:

	release_sock(sk);
	skb = sock_alloc_send_skb(sk, size, noblock, &rc);
	lock_sock(sk);
	if (!skb)
		goto out;

While the lock is dropped the socket is not owned. Two paths can then
run x25_disconnect() and set x25->neighbour to NULL:

  - this branch of x25_kill_sock(), reached from a RESTART or LAPB
    disconnect via x25_kill_by_neigh()
  - a remote CLEAR_REQUEST through x25_state3_machine()

sock_alloc_send_pskb() checks sk_err and SEND_SHUTDOWN only before it
allocates. A disconnect during the allocation or the relock goes
unnoticed, and the sender then reaches this before any TCP_ESTABLISHED
recheck:

	if (msg->msg_flags & MSG_OOB) {
		if (x25->neighbour->extended) {

The non-OOB branch has the same dereference. Can a remote peer trigger
a NULL pointer dereference this way?

The fragmentation loop in x25_output() has the same pattern. Each
fragment does release_sock() / sock_alloc_send_skb() / lock_sock(),
followed by an unchecked x25->neighbour->extended.

The old lock_sock() in x25_kill_by_neigh() could take the lock in the
same window, so this predates the patch. Still, the claim about keeping
the 7781607938c8 serialization holds only as far as that earlier fix
went.

> +	}
> +	bh_unlock_sock(sk);
> +	local_bh_enable();
> +}

[ ... ]

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