[PATCH net v2] tipc: defer local Nagle backlog receive processing

[PATCH net v2] tipc: defer local Nagle backlog receive processing

[Bart Chronowski]

A local TIPC stream can soft-lock a CPU when receiving a packet sends
queued Nagle data while the socket's spinlock is held. Local delivery
enters the receive path again before that lock is released. The nested
call cannot take the lock, so it retries the input queue without
making progress.

When an unowned socket has queued data for a local peer, put the
incoming packet on sk_backlog. Queue work to take socket ownership
and drain the backlog through release_sock(). Leave the older data
on sk_write_queue, where later send and shutdown cannot overtake it.

The owned-socket path already puts incoming packets on the backlog.
Only inspect sk_write_queue and the peer node when no user owns the
socket. tipc_sk_enqueue() holds sk_lock.slock at that point, so a
user cannot take ownership and change those fields during the check.

Hold a socket reference for the work, and drop the extra reference
if it is already pending. Queue the work after leaving the receive
loop, even when the loop stops at its time limit or finds no matching
packet. Set up the work before publishing the socket in the TIPC
port hash.

Fixes: c0bceb97db9e ("tipc: add smart nagle feature")
Reported-by: syzbot+10a41dc44eef71aa9450@syzkaller.appspotmail.com
Link: https://lore.kernel.org/r/GV1P189MB1988F6301FA670CA9818F7FCC6DB2@GV1P189MB1988.EURP189.PROD.OUTLOOK.COM
Signed-off-by: Bart Chronowski <immersa.bartosz.chronowski@gmail.com>
---
Changes in v2 (since the posted v1):
  - Ordering update: keep older output on
    sk_write_queue, where later sends append and shutdown pushes the
    queue before FIN. Defer incoming local packets to sk_backlog
    instead of detaching queued output into a separate TX queue.
  - Initialize the backlog work before publishing the socket, and
    inspect the TX queue and peer node only when no user owns the socket.

v1: https://lore.kernel.org/r/631b75b4-d1e3-40ea-a129-5ed090f914bf@mail.kernel.org

Validation:
  For the reported soft lockup, the C reproducer stalled all eight
  unpatched guests. None of the eight patched guests crashed during
  the 270-second run, with the same base and config.

  For send/close ordering, a framed AF_TIPC stream test kept accepted
  data in order in 300 later-send, 300 shutdown, and 300 last-close
  cases on the patched image.

  A complete workqueue trace recorded 547 balanced callbacks and
  four requeues while work was active, with no overlapping callbacks.

 net/tipc/socket.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 65cc0c4964b31..b03dd4a3d3ea2 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -94,6 +94,7 @@ struct sockaddr_pair {
  * @peer: 'connected' peer for dgram/rdm
  * @node: hash table node
  * @mc_method: cookie for use between socket and broadcast layer
+ * @backlog_work: processes receive backlog under socket ownership
  * @rcu: rcu struct for tipc_sock
  * @group: TIPC communications group
  * @oneway: message count in one direction (FIXME)
@@ -128,6 +129,7 @@ struct tipc_sock {
 	struct sockaddr_tipc peer;
 	struct rhash_head node;
 	struct tipc_mc_method mc_method;
+	struct work_struct backlog_work;
 	struct rcu_head rcu;
 	struct tipc_group *group;
 	u32 oneway;
@@ -143,6 +145,7 @@ struct tipc_sock {
 };
 
 static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb);
+static void tipc_sk_backlog_work(struct work_struct *work);
 static void tipc_data_ready(struct sock *sk);
 static void tipc_write_space(struct sock *sk);
 static void tipc_sock_destruct(struct sock *sk);
@@ -499,6 +502,7 @@ static int tipc_sk_create(struct net *net, struct socket *sock,
 	/* Finish initializing socket data structures */
 	sock->ops = ops;
 	sock_init_data(sock, sk);
+	INIT_WORK(&tsk->backlog_work, tipc_sk_backlog_work);
 	tipc_set_sk_state(sk, TIPC_OPEN);
 	if (tipc_sk_insert(tsk)) {
 		sk_free(sk);
@@ -2428,6 +2432,17 @@ static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 }
 
+static void tipc_sk_backlog_work(struct work_struct *work)
+{
+	struct tipc_sock *tsk = container_of(work, struct tipc_sock,
+					     backlog_work);
+	struct sock *sk = &tsk->sk;
+
+	lock_sock(sk);
+	release_sock(sk);
+	sock_put(sk);
+}
+
 /**
  * tipc_sk_enqueue - extract all buffers with destination 'dport' from
  *                   inputq and try adding them to socket or backlog queue
@@ -2445,18 +2460,30 @@ static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,
 	struct sk_buff *skb;
 	unsigned int lim;
 	atomic_t *dcnt;
+	bool deferred = false;
+	bool owned;
+	bool local_backlog;
 	u32 onode;
 
 	while (skb_queue_len(inputq)) {
 		if (unlikely(time_after_eq(jiffies, time_limit)))
-			return;
+			break;
 
 		skb = tipc_skb_dequeue(inputq, dport);
 		if (unlikely(!skb))
-			return;
+			break;
 
-		/* Add message directly to receive queue if possible */
-		if (!sock_owned_by_user(sk)) {
+		/*
+		 * A local Nagle backlog may loop back into this socket while its
+		 * spinlock is held.  Queue the triggering input for processing under
+		 * socket ownership, which keeps a concurrent sender from overtaking
+		 * the already queued output.
+		 */
+		owned = sock_owned_by_user(sk);
+		local_backlog = !owned && !skb_queue_empty(&sk->sk_write_queue) &&
+				in_own_node(sock_net(sk),
+					    tsk_peer_node(tipc_sk(sk)));
+		if (!owned && !local_backlog) {
 			tipc_sk_filter_rcv(sk, skb, xmitq);
 			continue;
 		}
@@ -2467,6 +2494,7 @@ static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,
 			atomic_set(dcnt, 0);
 		lim = rcvbuf_limit(sk, skb) + atomic_read(dcnt);
 		if (likely(!sk_add_backlog(sk, skb, lim))) {
+			deferred |= !owned;
 			trace_tipc_sk_overlimit1(sk, skb, TIPC_DUMP_SK_BKLGQ,
 						 "bklg & rcvq >90% allocated!");
 			continue;
@@ -2483,6 +2511,12 @@ static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,
 		}
 		break;
 	}
+
+	if (deferred) {
+		sock_hold(sk);
+		if (!schedule_work(&tipc_sk(sk)->backlog_work))
+			sock_put(sk);
+	}
 }
 
 /**

Re: [PATCH net v2] tipc: defer local Nagle backlog receive processing

[netdev-bot+sashiko]

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

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

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] The fix is incomplete.
- [Low] The deferral condition is broader than the subject and the in-
  code comment ('A local Nagle backlog may loop back...') say.
- [Low] Before this patch, an unowned socket handled CONN_MANAGER
  messages directly in tipc_sk_filter_rcv() -> tipc_sk_proto_rcv(), with…
- [Low] The new work handler tipc_sk_backlog_work()
  (net/tipc/socket.c:2435-2444) ends with sock_put(sk), which can drop…

--

tipc: defer local Nagle backlog receive processing

This patch targets a soft lockup that happens when a TIPC stream receives
a packet, pushes queued Nagle data to a local peer while sk_lock.slock is
held, and the looped-back delivery then spins in tipc_sk_rcv(). When an
unowned socket has queued output for a local peer, tipc_sk_enqueue() now
puts the incoming packet on sk_backlog. It then schedules a work item that
drains the backlog through lock_sock()/release_sock().

Link: https://lore.kernel.org/r/GV1P189MB1988F6301FA670CA9818F7FCC6DB2@GV1P189MB1988.EURP189.PROD.OUTLOOK.COM

> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 65cc0c4964b31..b03dd4a3d3ea2 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c

[ ... ]

> @@ -2428,6 +2432,17 @@ static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>  	return 0;
>  }
>  
> +static void tipc_sk_backlog_work(struct work_struct *work)
> +{
> +	struct tipc_sock *tsk = container_of(work, struct tipc_sock,
> +					     backlog_work);
> +	struct sock *sk = &tsk->sk;
> +
> +	lock_sock(sk);
> +	release_sock(sk);
> +	sock_put(sk);
> +}

[Severity: Low]
Can this sock_put() drop the last socket reference while the kworker is
still running TIPC module text?

If tipc_release() and tipc_sk_callback() have already dropped their
references, the final put goes:

tipc_sk_backlog_work()
  sock_put()
    sk_free()
      __sk_free()
        __sk_destruct()
          sk_prot_free()
            module_put(prot->owner)

Nothing in net/tipc flushes or cancels backlog_work. The only
cancel_work_sync() in TIPC is for tn->work. If tipc is removed with rmmod
at the same moment, could the module text be freed before
tipc_sk_backlog_work() returns?

A pending work item holds a socket reference, and so a module reference.
The window is only the tail after the final put. It needs root to unload
the module, and sk_free() is often emitted as a tail call. The existing
tipc_sk_timeout() also ends with a trailing sock_put() in the same way.

> +
>  /**
>   * tipc_sk_enqueue - extract all buffers with destination 'dport' from
>   *                   inputq and try adding them to socket or backlog queue
> @@ -2445,18 +2460,30 @@ static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,

[ ... ]

> +		/*
> +		 * A local Nagle backlog may loop back into this socket while its
> +		 * spinlock is held.  Queue the triggering input for processing under
> +		 * socket ownership, which keeps a concurrent sender from overtaking
> +		 * the already queued output.
> +		 */
> +		owned = sock_owned_by_user(sk);
> +		local_backlog = !owned && !skb_queue_empty(&sk->sk_write_queue) &&
> +				in_own_node(sock_net(sk),
> +					    tsk_peer_node(tipc_sk(sk)));

[Severity: High]
Does the in_own_node() check cover every case where tipc_node_xmit()
delivers back into the receive path synchronously?

When the peer node is in another network namespace on the same host,
n->peer_net is set. In that case tipc_node_xmit() hands the list to
tipc_lxc_xmit(), which calls tipc_sk_rcv(peer_net, list) inline:

net/tipc/node.c:tipc_node_xmit() {
    ...
	if (node_up && peer_net && check_net(peer_net)) {
		/* xmit inner linux container */
		tipc_lxc_xmit(peer_net, list);
    ...
}

For that kind of peer, in_own_node() is false and local_backlog is false.
tipc_sk_filter_rcv() still runs with sk_lock.slock held. In
TIPC_ESTABLISHED this call chain looks possible:

tipc_sk_rcv(net1)                     /* takes socket A's slock */
  tipc_sk_enqueue()
    tipc_sk_filter_rcv()
      tipc_sk_filter_connect()
        tipc_sk_push_backlog()
          tipc_node_xmit()
            tipc_lxc_xmit()
              tipc_sk_rcv(net2)       /* peer socket B */
                B sends its Nagle ACK via tipc_node_distr_xmit(),
                or pushes its own non-empty write queue
                  tipc_lxc_xmit()
                    tipc_sk_rcv(net1) /* socket A again */

In the inner tipc_sk_rcv() the trylock fails, because an outer frame on
the same CPU holds A's slock:

	if (likely(spin_trylock_bh(&sk->sk_lock.slock))) {
		tipc_sk_enqueue(inputq, sk, dport, &xmitq);
		spin_unlock_bh(&sk->sk_lock.slock);
	}
	...
	continue;

Wouldn't this retry forever with BH disabled, inside the rcu_read_lock()
taken in tipc_node_xmit()?

Nagle is active for these peers, because tipc_node_get_mtu() returns
MAX_MSG_SIZE for peer_net connections. A two-CPU ABBA variant also looks
possible: each CPU holds one socket's slock and spins on the other's.

The setup is two namespaces linked by a bearer such as veth/UDP.
TIPC_NL_BEARER_ENABLE uses GENL_UNS_ADMIN_PERM, so an unprivileged user
with user namespaces could set this up.

Would the guard need to cover peer_net delivery as well, so that the
lockup described in the commit message cannot happen through the
cross-netns path?

[Severity: Low]
This condition doesn't check Nagle or the socket state. Is the deferral
meant to apply during connection setup too?

__tipc_sendmsg() clones the SYN onto sk_write_queue and sets the phdr
destnode to the target node:

	if (unlikely(syn && !tipc_msg_skb_clone(&pkts, &sk->sk_write_queue))) {

The clone stays on the queue until tipc_sk_finish_conn() purges it. That
only happens after the SYN-ACK has been processed.

During a blocking connect (sk_wait_event() drops ownership), or after a
non-blocking connect, the SYN-ACK reaches tipc_sk_enqueue() unowned. The
write queue is non-empty and destnode is local, so local_backlog is true.
The SYN-ACK then goes through sk_backlog and a work item on
system_percpu_wq.

This also covers SOCK_SEQPACKET, which never uses Nagle because
tsk_set_nagle() returns early for anything other than SOCK_STREAM. The
TIPC_CONNECTING case of tipc_sk_filter_connect() never calls
tipc_sk_push_backlog(), so it cannot cause the recursion.

Would checking for sk_state == TIPC_ESTABLISHED match the intent in the
comment and the commit message better? The commit message does not
mention the extra kworker round trip on every local connect.

> +		if (!owned && !local_backlog) {
>  			tipc_sk_filter_rcv(sk, skb, xmitq);
>  			continue;
>  		}
> @@ -2467,6 +2494,7 @@ static void tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk,
>  			atomic_set(dcnt, 0);
>  		lim = rcvbuf_limit(sk, skb) + atomic_read(dcnt);
>  		if (likely(!sk_add_backlog(sk, skb, lim))) {
> +			deferred |= !owned;
>  			trace_tipc_sk_overlimit1(sk, skb, TIPC_DUMP_SK_BKLGQ,
>  						 "bklg & rcvq >90% allocated!");
>  			continue;

[Severity: Low]
Before this patch, an unowned socket handled CONN_MANAGER messages
directly through tipc_sk_filter_rcv()->tipc_sk_proto_rcv(), with no
receive buffer limit.

When local_backlog is true, those messages now go through
sk_add_backlog(). If the limit is exceeded they are rejected later in
tipc_sk_enqueue():

		/* Overload => reject message back to sender */
		...
		if (tipc_msg_reverse(onode, &skb, TIPC_ERR_OVERLOAD)) {

On the peer, tipc_sk_conn_proto_rcv() treats any errcode as fatal:

	if (unlikely(msg_errcode(hdr))) {
		tipc_set_sk_state(sk, TIPC_DISCONNECTING);
		tipc_node_remove_conn(sock_net(sk), tsk_peer_node(tsk),
				      tsk_peer_port(tsk));

Could a CONN_ACK or CONN_PROBE rejected here abort the connection?

The limit for CONN_ACK is sk_rcvbuf << TIPC_SYSTEM_IMPORTANCE. For
CONN_PROBE it is sk_rcvbuf when TIPC_BLOCK_FLOWCTL is set. Hitting these
needs a very small SO_RCVBUF, or receive queue plus backlog above
sk_rcvbuf, which flow control normally prevents.

The owned-socket path already behaves this way. The patch widens the
window to every unowned period in which output is queued for a local
peer.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/657t6njsrgnpnfs2mc2lv776442fhv5zsosg3vxya5vqrtpag7%40nq3uxt6ymw7s