[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@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=9faa82ae2e94c5c7d2ef
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
---
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