Re: [PATCH v3 net-next 1/9] dlm: fix send buffer backpressure handling

[netdev-bot+sashiko@kernel.org]

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

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

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] A TCP reset or TCP timeout while the connection is waiting for
  send space can stall the dlm connection permanently.
- [Low] In send_to_sock() (fs/dlm/lowcomms.c:1392-1397), the
  set_bit(SOCK_NOSPACE, &con->sock->sk->sk_socket->flags) under the…

--

dlm: fix send buffer backpressure handling

This commit changes send_to_sock() to test SOCK_NOSPACE instead of
SOCKWQ_ASYNC_NOSPACE in con->sock->flags. With that change the
CF_APP_LIMITED wait path is taken again when TCP returns -EAGAIN because
the send buffer is full. It also removes the clear_bit() of
SOCKWQ_ASYNC_NOSPACE from lowcomms_write_space(), since that bit no
longer lives in con->sock->flags.

> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 2aff1c7c17de4..abe9ae4c643fa 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c

[ ... ]

> @@ -1391,7 +1389,7 @@ static int send_to_sock(struct connection *con)
>  	if (ret == -EAGAIN || ret == 0) {
>  		lock_sock(con->sock->sk);
>  		spin_lock_bh(&con->writequeue_lock);
> -		if (test_bit(SOCKWQ_ASYNC_NOSPACE, &con->sock->flags) &&
> +		if (test_bit(SOCK_NOSPACE, &con->sock->flags) &&
>  		    !test_and_set_bit(CF_APP_LIMITED, &con->flags)) {

[Severity: High]
Can a TCP reset or timeout that arrives while the connection is in this
state leave it stuck for good?

With this change, send_to_sock() sets CF_APP_LIMITED, clears
CF_SEND_PENDING and returns DLM_IO_END. After that,
lowcomms_queue_swork() will not queue swork:

fs/dlm/lowcomms.c:lowcomms_queue_swork() {
	if (!test_bit(CF_IO_STOP, &con->flags) &&
	    !test_bit(CF_APP_LIMITED, &con->flags) &&
	    !test_and_set_bit(CF_SEND_PENDING, &con->flags))
		queue_work(io_workqueue, &con->swork);
}

>From then on, only lowcomms_write_space() or close_connection() clears
CF_APP_LIMITED.

tcp_reset() and tcp_write_err() (RTO or keepalive timeout) both go
through tcp_done_with_error():

net/ipv4/tcp_input.c:tcp_done_with_error() {
    ...
	tcp_write_queue_purge(sk);
	tcp_done(sk);

	if (!sock_flag(sk, SOCK_DEAD))
		sk_error_report(sk);
}

Neither path calls sk_write_space(). tcp_done() calls sk_state_change,
but add_sock() only installs lowcomms_state_change for SCTP.

Every dlm recovery path appears to end in lowcomms_queue_swork(), which
returns early here:

lowcomms_error_report()
  dlm_midcomms_unack_msg_resend()
    dlm_lowcomms_resend_msg()
      _dlm_lowcomms_commit_msg()
        lowcomms_queue_swork()

On a receive error on the main connection, process_recv_sockets() also
only calls lowcomms_queue_swork():

		if (ret < 0) {
			if (test_bit(CF_IS_OTHERCON, &con->flags)) {
				close_connection(con, false);
			} else {
				spin_lock_bh(&con->writequeue_lock);
				lowcomms_queue_swork(con);
				spin_unlock_bh(&con->writequeue_lock);
			}

A RST does not call sk_data_ready, so the receive worker may not run at
all. New outgoing messages hit the same CF_APP_LIMITED check. If the peer
reconnects, accept_from_sock() puts the new socket on othercon because
the main connection's sock is still set.

Before this patch, the send worker kept requeueing itself with
DLM_IO_RESCHED. The next sock_sendmsg() then returned the socket error,
and process_send_sockets() called close_connection() and reconnected.

A peer RST seems realistic here. It happens when the peer's dlm closes
its socket with unread data in the receive queue, which is likely when
the peer is slow to drain. It also happens with ss -K or a middlebox RST.

If this happens, won't traffic to that node stay queued on a dead socket
until dlm_lowcomms_close() or lowcomms shutdown?

The commit message says that for SCTP, returning DLM_IO_END "would wait
for a callback that never comes". Doesn't the TCP error path have the
same problem?

Later patches in the series convert these bit operations to
sk_set_nospace()/sk_clear_nospace() and add tp->tcp_nospace. None of
them seem to change this behavior.

>  			/* Notify TCP that we're limited by the
>  			 * application window size.

[Severity: Low]
This isn't a bug, but the statement under this comment now does
nothing:

			set_bit(SOCK_NOSPACE, &con->sock->sk->sk_socket->flags);

The branch is only entered when test_bit(SOCK_NOSPACE, &con->sock->flags)
is already true. con->sock->sk->sk_socket is con->sock, so this sets a
bit that is already set.

TCP already sets it on its -EAGAIN paths (tcp_sendmsg_locked() and
sk_stream_wait_memory()). While lock_sock() is held, TCP input that could
run lowcomms_write_space() is deferred to the backlog, so nothing can
clear the bit between the test and the set.

Should the set_bit() and the "Notify TCP" comment be removed or
reworded?

The same applies later in the series, where this line becomes
sk_set_nospace(con->sock->sk).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929071743.23624-1-edumazet%40kernel.org