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

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

[Nguyen Ngoc Thang]

x25_kill_by_neigh() is reached from the receive path when the link layer
goes away or the peer restarts:

  net_rx_action()
    lapbeth_napi_poll()
      x25_lapb_receive_frame()
        x25_link_terminated() / x25_link_control()
          x25_kill_by_neigh()
            lock_sock()

lock_sock() may sleep, which is not allowed in softirq context:

  BUG: sleeping function called from invalid context at net/core/sock.c:3832
  in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 29, name: ktimers/1
  RCU nest depth: 2, expected: 0
  ...
   lock_sock_nested+0x56/0x130 net/core/sock.c:3832
   x25_kill_by_neigh+0x134/0x2a0 net/x25/af_x25.c:1778
   x25_lapb_receive_frame+0x1b0/0xfb0 net/x25/x25_dev.c:138

Take the socket spinlock instead, the same way the x25 timers and receive
path already do. If the socket is not owned by user, disconnect it right
away. If it is, mark it with X25_KILL_FLAG and let a new release_cb do
the disconnect when the owner releases the socket. This keeps the
serialization against x25_sendmsg()/x25_recvmsg()/x25_connect() that
commit 7781607938c8 ("net/x25: Fix null-ptr-deref caused by
x25_disconnect") added lock_sock() for. Sockets that already have the
flag set are skipped, so the rescan loop always terminates.

NETDEV_DOWN runs in process context and the device may be freed right
after, so keep waiting for socket owners there as before: a sleeping
sweep makes sure no socket still uses the neighbour on return.

Fixes: 7781607938c8 ("net/x25: Fix null-ptr-deref caused by x25_disconnect")
Reported-by: syzbot+9faa82ae2e94c5c7d2ef@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9faa82ae2e94c5c7d2ef
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
Tested in QEMU with the syzbot config (PREEMPT_RT, KASAN, lockdep) and
the syzbot C reproducer: the unpatched kernel hits the BUG twice in 240s,
the patched kernel runs 300s clean. The owned-by-user branch was
exercised with a temporary debug hack forcing the deferred path:
release_cb disconnected the socket and userspace saw ENETUNREACH.
"ip link set lapb0 down" with a connecting socket also disconnects it
right away. No lockdep splats in any run.

syzbot also has an AI-generated workqueue variant of this fix in
moderation. This version keeps the teardown synchronous for sockets
that are not owned, so a link that comes back quickly cannot have a
stale work item kill freshly connected calls. Owned sockets are cleared
when their owner releases them.

 include/net/x25.h |  1 +
 net/x25/af_x25.c  | 62 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/include/net/x25.h b/include/net/x25.h
index 414f3fd99345..ed8ba01fa3cb 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -118,6 +118,7 @@ enum {
 #define X25_Q_BIT_FLAG		0
 #define X25_INTERRUPT_FLAG	1
 #define X25_ACCPT_APPRV_FLAG	2
+#define X25_KILL_FLAG		3
 
 /**
  *	struct x25_route - x25 routing entry
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 033e7d059f58..4f9ca22231ac 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);
+			if (x25_sk(s)->neighbour == nb)
+				x25_disconnect(s, ENETUNREACH, 0, 0);
+			release_sock(s);
+			sock_put(s);
+			goto again;
+		}
+	}
+	write_unlock_bh(&x25_list_lock);
+}
+
 /*
  *	Handle device status changes.
  */
@@ -222,6 +248,7 @@ static int x25_device_event(struct notifier_block *this, unsigned long event,
 			nb = x25_get_neigh(dev);
 			if (nb) {
 				x25_link_terminated(nb);
+				x25_kill_by_neigh_sync(nb);
 				x25_neigh_put(nb);
 			}
 			x25_route_device_down(dev);
@@ -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);
+}
+
 static struct proto x25_proto = {
 	.name	  = "X25",
 	.owner	  = THIS_MODULE,
 	.obj_size = sizeof(struct x25_sock),
+	.release_cb = x25_release_cb,
 };
 
 static struct sock *x25_alloc_socket(struct net *net, int kern)
@@ -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);
+		else
+			x25_disconnect(sk, ENETUNREACH, 0, 0);
+	}
+	bh_unlock_sock(sk);
+	local_bh_enable();
+}
+
 void x25_kill_by_neigh(struct x25_neigh *nb)
 {
 	struct sock *s;
@@ -1772,13 +1826,11 @@ void x25_kill_by_neigh(struct x25_neigh *nb)
 	write_lock_bh(&x25_list_lock);
 
 	sk_for_each(s, &x25_list) {
-		if (x25_sk(s)->neighbour == nb) {
+		if (x25_sk(s)->neighbour == nb &&
+		    !test_bit(X25_KILL_FLAG, &x25_sk(s)->flags)) {
 			sock_hold(s);
 			write_unlock_bh(&x25_list_lock);
-			lock_sock(s);
-			if (x25_sk(s)->neighbour == nb)
-				x25_disconnect(s, ENETUNREACH, 0, 0);
-			release_sock(s);
+			x25_kill_sock(s, nb);
 			sock_put(s);
 			goto again;
 		}
-- 
2.43.0

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

[netdev-bot+sashiko]

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