[PATCH net-next 0/3] net: qrtr: implement socket shutdown()

[PATCH net-next 0/3] net: qrtr: implement socket shutdown()

[Chunkai Deng]

QRTR does not support shutdown(), so a thread blocked in recv() cannot
be woken up by another thread. This series adds it.

Patch 1 stops recvmsg() from holding the socket lock while it waits for
data. Patch 2 stops sendmsg() from turning every allocation failure into
-ENOMEM, so errors such as -ENETRESET and -EPIPE reach userspace. Patch 3
adds qrtr_shutdown().

Patches 1 and 2 address long-standing behaviour rather than regressions,
so they carry no Fixes: tags.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
To: Manivannan Sadhasivam <mani@kernel.org>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Simon Horman <horms@kernel.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: tao.zhang1@oss.qualcomm.com
Cc: peter.chen@oss.qualcomm.com
Cc: jeff.johnson@oss.qualcomm.com
Cc: konrad.dybcio@oss.qualcomm.com
Cc: christopher.lew@oss.qualcomm.com>

---
Chunkai Deng (3):
      net: qrtr: do not take the sock lock on blocking recv
      net: qrtr: propagate sock_alloc_send_skb() errors from sendmsg()
      net: qrtr: implement shutdown()

 net/qrtr/af_qrtr.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)
---
base-commit: 34cf6dafc47441dfb6b356a095b89c3585a93714
change-id: 20260710-qrtr-add-shutdown-5f10da5af721

Best regards,
--  
Chunkai Deng <chunkai.deng@oss.qualcomm.com>

[PATCH net-next 1/3] net: qrtr: do not take the sock lock on blocking recv

[Chunkai Deng]

qrtr_recvmsg() takes lock_sock() before skb_recv_datagram() and keeps it
while waiting for a datagram. Any other call on the same socket, such as
sendmsg(), bind() or ioctl(), blocks until a packet arrives.

Take the lock only once a datagram has been dequeued. The receive queue
has its own lock and the enqueue path never takes the socket lock. The
lock is still needed around the copy and skb_free_datagram(), since
TIOCINQ peeks the queue under lock_sock() only.

Testing SOCK_ZAPPED without the lock means a recv() racing with bind()
may return -EADDRNOTAVAIL for a port that was just published. There is
no ordering between the two calls anyway.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
 net/qrtr/af_qrtr.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index d02ef9a74c3ca..2d7339338e460 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -1034,18 +1034,14 @@ static int qrtr_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct qrtr_cb *cb;
 	int copied, rc;
 
-	lock_sock(sk);
-
-	if (sock_flag(sk, SOCK_ZAPPED)) {
-		release_sock(sk);
+	if (sock_flag(sk, SOCK_ZAPPED))
 		return -EADDRNOTAVAIL;
-	}
 
 	skb = skb_recv_datagram(sk, flags, &rc);
-	if (!skb) {
-		release_sock(sk);
+	if (!skb)
 		return rc;
-	}
+
+	lock_sock(sk);
 	cb = (struct qrtr_cb *)skb->cb;
 
 	copied = skb->len;

-- 
2.43.0

[PATCH net-next 2/3] net: qrtr: propagate sock_alloc_send_skb() errors from sendmsg()

[Chunkai Deng]

Since commit 179d0ba0c454 ("net: qrtr: fix error return code of
qrtr_sendmsg()"), qrtr_sendmsg() returns -ENOMEM whenever
sock_alloc_send_skb() fails. The helper already sets rc on every failure
path, so this overwrites the real error.

In particular, sock_alloc_send_skb() consumes a pending sk_err, so the
ENETRESET set by qrtr_reset_ports() is lost and userspace never learns
that a new controller came up. -EAGAIN, -EPIPE and signal errors are
hidden as well.

Return the error from sock_alloc_send_skb() as is.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
 net/qrtr/af_qrtr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 2d7339338e460..09c1cb443f5cb 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -959,10 +959,8 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	plen = (len + 3) & ~3;
 	skb = sock_alloc_send_skb(sk, plen + QRTR_HDR_MAX_SIZE,
 				  msg->msg_flags & MSG_DONTWAIT, &rc);
-	if (!skb) {
-		rc = -ENOMEM;
+	if (!skb)
 		goto out_node;
-	}
 
 	skb_reserve(skb, QRTR_HDR_MAX_SIZE);
 

-- 
2.43.0

[PATCH net-next 3/3] net: qrtr: implement shutdown()

[Chunkai Deng]

QRTR uses sock_no_shutdown(), so shutdown() fails with -EOPNOTSUPP and
there is no way to wake up a thread blocked in recv() on the socket.

Implement it the same way as unix_shutdown(): map SHUT_* to the
*_SHUTDOWN bits, set them in sk_shutdown and call sk_state_change().

Do not use lock_sock() here. qrtr_sendmsg() holds it while sleeping in
qrtr_tx_wait() until the remote returns flow control credit, which would
leave shutdown() stuck in an uninterruptible wait. Take sk_lock.slock to
serialize concurrent callers instead, and use WRITE_ONCE() for the
lockless readers. Do the same for the store in qrtr_release().

A sender already sleeping in qrtr_tx_wait() is not woken by SHUT_WR;
later sendmsg() calls fail with -EPIPE.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
 net/qrtr/af_qrtr.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 09c1cb443f5c..c955c05e71bd 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2013, The Linux Foundation. All rights reserved.
  */
 #include <linux/module.h>
+#include <linux/net.h>
 #include <linux/netlink.h>
 #include <linux/qrtr.h>
 #include <linux/termios.h>	/* For TIOCINQ/OUTQ */
@@ -1193,6 +1194,36 @@ static int qrtr_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 	return rc;
 }
 
+static int qrtr_shutdown(struct socket *sock, int mode)
+{
+	struct sock *sk;
+
+	if (mode < SHUT_RD || mode > SHUT_RDWR)
+		return -EINVAL;
+
+	/*
+	 * This maps:
+	 * SHUT_RD   (0) -> RCV_SHUTDOWN  (1)
+	 * SHUT_WR   (1) -> SEND_SHUTDOWN (2)
+	 * SHUT_RDWR (2) -> SHUTDOWN_MASK (3)
+	 */
+	mode++;
+
+	sk = sock->sk;
+
+	/*
+	 * Do not use lock_sock() here: sendmsg() holds it while sleeping on
+	 * remote flow control, which would block shutdown() indefinitely.
+	 * The spinlock only serializes concurrent shutdown() callers.
+	 */
+	spin_lock_bh(&sk->sk_lock.slock);
+	WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | mode);
+	spin_unlock_bh(&sk->sk_lock.slock);
+	sk->sk_state_change(sk);
+
+	return 0;
+}
+
 static int qrtr_release(struct socket *sock)
 {
 	struct sock *sk = sock->sk;
@@ -1204,7 +1235,7 @@ static int qrtr_release(struct socket *sock)
 	lock_sock(sk);
 
 	ipc = qrtr_sk(sk);
-	sk->sk_shutdown = SHUTDOWN_MASK;
+	WRITE_ONCE(sk->sk_shutdown, SHUTDOWN_MASK);
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_state_change(sk);
 
@@ -1237,7 +1268,7 @@ static const struct proto_ops qrtr_proto_ops = {
 	.ioctl		= qrtr_ioctl,
 	.gettstamp	= sock_gettstamp,
 	.poll		= datagram_poll,
-	.shutdown	= sock_no_shutdown,
+	.shutdown	= qrtr_shutdown,
 	.release	= qrtr_release,
 	.mmap		= sock_no_mmap,
 };

-- 
2.43.0

Re: [PATCH net-next 1/3] net: qrtr: do not take the sock lock on blocking recv

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit "net: qrtr: do not take the sock lock on blocking
  recv" fixes a real, user-visible defect but has no Fixes: tag.
- [Low] The concern says qrtr_ioctl(TIOCINQ) (net/qrtr/af_qrtr.c:1234),
  which calls skb_peek(&sk->sk_receive_queue) under lock_sock() only, is…

--

net: qrtr: do not take the sock lock on blocking recv

This patch changes qrtr_recvmsg() so it no longer holds lock_sock() while
it blocks in skb_recv_datagram(). The lock is now taken only after a
datagram has been dequeued. Before this, other calls on the same socket
could not run while a recv() was waiting.

> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>

[Severity: Low]
Should this have a Fixes: tag? The commit message describes a hang that
users can see. A thread blocked in recv() holds lock_sock() inside
skb_recv_datagram(). While it waits, sendmsg(), bind(), connect() or
ioctl() from another thread on the same socket cannot proceed until a
datagram arrives.

In qrtr_recvmsg(), the lock_sock() held across skb_recv_datagram() and
the release_sock() calls on the early return paths all come from the
original driver, so perhaps:

Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index e7b3647424b8f..0851763d83baf 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -1109,18 +1109,14 @@ static int qrtr_recvmsg(struct socket *sock, struct msghdr *msg,
>  	struct qrtr_cb *cb;
>  	int copied, rc;
>  
> -	lock_sock(sk);
> -
> -	if (sock_flag(sk, SOCK_ZAPPED)) {
> -		release_sock(sk);
> +	if (sock_flag(sk, SOCK_ZAPPED))
>  		return -EADDRNOTAVAIL;
> -	}
>  
>  	skb = skb_recv_datagram(sk, flags, &rc);
> -	if (!skb) {
> -		release_sock(sk);
> +	if (!skb)
>  		return rc;
> -	}
> +
> +	lock_sock(sk);

[Severity: Low]
This isn't a bug, but the dequeue now happens outside lock_sock(), so
qrtr_ioctl(TIOCINQ) is no longer serialized against it:

net/qrtr/af_qrtr.c:qrtr_ioctl() {
    ...
	lock_sock(sk);
    ...
	case TIOCINQ:
		skb = skb_peek(&sk->sk_receive_queue);
		if (skb)
			len = skb->len;
    ...
}

As far as I can tell this is still memory safe. skb_free_datagram() in
qrtr_recvmsg() still runs under lock_sock(). The consume_skb() in
skb_set_peeked() only drops a reference on a shared skb.

The length TIOCINQ reports also matches some valid ordering of the ioctl
and the recv. The unlocked skb_peek() racing with __skb_queue_tail() on
the enqueue path was already possible before this patch.

Would it be worth taking sk_receive_queue.lock around the skb_peek() in
TIOCINQ, as packet_ioctl() does? Then the ioctl would not rely on
qrtr_recvmsg() freeing the skb under lock_sock().

>  	cb = (struct qrtr_cb *)skb->cb;
>  
>  	copied = skb->len;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-qrtr-add-shutdown-v1-0-e1cc01ca5919%40oss.qualcomm.com

Re: [PATCH net-next 2/3] net: qrtr: propagate sock_alloc_send_skb() errors from sendmsg()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch fixes a userspace-visible errno regression in
  qrtr_sendmsg() (net/qrtr/af_qrtr.c) and names the commit that caused…

--

net: qrtr: propagate sock_alloc_send_skb() errors from sendmsg()

This patch removes the rc = -ENOMEM override in qrtr_sendmsg() that runs
when sock_alloc_send_skb() fails. The error set by the helper is now
returned to userspace instead. This brings back reporting of a pending
ENETRESET from qrtr_reset_ports(), as well as -EAGAIN, -EPIPE and signal
errors.

> Since commit 179d0ba0c454 ("net: qrtr: fix error return code of
> qrtr_sendmsg()"), qrtr_sendmsg() returns -ENOMEM whenever
> sock_alloc_send_skb() fails. The helper already sets rc on every failure
> path, so this overwrites the real error.

[ ... ]

> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>

[Severity: Low]
This isn't a bug, but the body names the commit that introduced the
-ENOMEM override. Should the patch also carry a Fixes: trailer for it?

Fixes: 179d0ba0c454 ("net: qrtr: fix error return code of qrtr_sendmsg()")

179d0ba0c454 has been in stable kernels since v5.12. Without the tag, the
stable backport tooling may not pick up this fix. Those kernels would then
keep returning -ENOMEM in these cases:

A pending ENETRESET set by qrtr_reset_ports(). sock_error() clears it when
it reads it inside sock_alloc_send_pskb(), so the notification is lost.

MSG_DONTWAIT senders with a full sndbuf, which should get -EAGAIN.

SEND_SHUTDOWN (-EPIPE) and signal interruptions (-ERESTARTSYS/-EINTR).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-qrtr-add-shutdown-v1-0-e1cc01ca5919%40oss.qualcomm.com

Re: [PATCH net-next 3/3] net: qrtr: implement shutdown()

[netdev-bot+sashiko]

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

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

- [Medium] net/qrtr: shutdown() API violation leaves sendmsg() threads
  hung

--

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -1193,6 +1194,36 @@ static int qrtr_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>  	return rc;
>  }
>  
> +static int qrtr_shutdown(struct socket *sock, int mode)
> +{
> +	struct sock *sk;
> +
> +	if (mode < SHUT_RD || mode > SHUT_RDWR)
> +		return -EINVAL;
> +
> +	/*
> +	 * This maps:
> +	 * SHUT_RD   (0) -> RCV_SHUTDOWN  (1)
> +	 * SHUT_WR   (1) -> SEND_SHUTDOWN (2)
> +	 * SHUT_RDWR (2) -> SHUTDOWN_MASK (3)
> +	 */
> +	mode++;
> +
> +	sk = sock->sk;
> +
> +	/*
> +	 * Do not use lock_sock() here: sendmsg() holds it while sleeping on
> +	 * remote flow control, which would block shutdown() indefinitely.
> +	 * The spinlock only serializes concurrent shutdown() callers.
> +	 */
> +	spin_lock_bh(&sk->sk_lock.slock);
> +	WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | mode);
> +	spin_unlock_bh(&sk->sk_lock.slock);
> +	sk->sk_state_change(sk);
> +
> +	return 0;
> +}
> +
>  static int qrtr_release(struct socket *sock)
>  {
>  	struct sock *sk = sock->sk;

[Severity: Medium]
With this change shutdown(SHUT_WR) returns 0, but a thread that is
already sleeping in qrtr_tx_wait() from qrtr_sendmsg() is not woken.
sk->sk_state_change() only wakes the socket wait queue. The sender
sleeps on flow->resume_tx instead, and the wait condition in
qrtr_tx_wait() only looks at flow->pending, flow->tx_failed and
node->ep, never at sk->sk_shutdown.

The changelog does call this out, but it breaks the common pattern of
calling shutdown() from one thread to kick a blocked writer out before
joining it. That now hangs until the remote hands back credit or a
signal arrives. Before this patch userspace at least got -EOPNOTSUPP
and knew it had to use some other mechanism.

The stuck sender also still holds lock_sock(). So the close() that
normally follows the shutdown() then sleeps uninterruptibly in
lock_sock() in qrtr_release(), as do ioctl() and the other
lock_sock() users, which is hung task territory.

Would it be feasible to also check SEND_SHUTDOWN in the qrtr_tx_wait()
condition and wake the relevant flow->resume_tx from qrtr_shutdown()?
The flow is keyed by node and destination port, so it is not obvious
how to reach it from the socket. Did you consider recording the flow
the sender is waiting on, or is there a reason to leave this for a
follow-up?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-qrtr-add-shutdown-v1-0-e1cc01ca5919%40oss.qualcomm.com