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

[netdev-bot+sashiko@kernel.org]

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