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@xxxxxxxxxxxxxxxx>

[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