Re: [PATCH v3 net-next 2/9] net: add sk_set_nospace() and sk_clear_nospace()

[netdev-bot+sashiko@kernel.org]

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 kernel-doc added in net/core/sock.c for the new helpers does
  not match how the helpers are used, both in this patch and later in…
- [Low] sk_set_nospace() and sk_clear_nospace() (net/core/sock.c) read
  sk->sk_socket with a plain load (`struct socket *sock = sk->sk_socket;…

--

net: add sk_set_nospace() and sk_clear_nospace()

This commit adds two helpers, sk_set_nospace() and sk_clear_nospace().
They set or clear SOCK_NOSPACE in sk->sk_socket->flags after a NULL
check, and the commit converts the core networking callers to use them.
It prepares for a later patch that gives TCP a private copy of the bit.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 2948dffcc3e1b..11a22aec7e414 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2957,6 +2957,39 @@ void sock_kzfree_s(struct sock *sk, void *mem, int size)
>  }
>  EXPORT_SYMBOL(sock_kzfree_s);
>  
> +/**
> + *	sk_set_nospace - tell the transport a writer is waiting for space
> + *	@sk: socket
> + *
> + *	Must be called before the final check of the available send space,
> + *	so that the transport can not miss the request and forget to call
> + *	sk->sk_write_space() once space is available again.
> + */
> +void sk_set_nospace(struct sock *sk)
> +{
> +	struct socket *sock = sk->sk_socket;
> +
> +	if (sock)
> +		set_bit(SOCK_NOSPACE, &sock->flags);
> +}

[Severity: Low]

Should this be READ_ONCE(sk->sk_socket)?

The writer side uses a marked store:

include/net/sock.h:sk_set_socket() {
	WRITE_ONCE(sk->sk_socket, sock);
}

sock_orphan() makes that store holding only sk_callback_lock:

	write_lock_bh(&sk->sk_callback_lock);
	sock_set_flag(sk, SOCK_DEAD);
	sk_set_socket(sk, NULL);

At least one path can reach these helpers at the same time as that store.
It does not take sk_callback_lock and does not check socket ownership:

CPU1 (SMC CDC receive tasklet)
smc_cdc_msg_recv()
  bh_lock_sock(&smc->sk)
  smc_cdc_msg_recv_action()
    ...
      smc_tx_write_space()
        sk_clear_nospace()
          sock = sk->sk_socket;

CPU2
close()
  smc_release()
    lock_sock(sk)
    sock_orphan(sk)
      WRITE_ONCE(sk->sk_socket, NULL)

Won't KCSAN report this plain read racing with the marked write?

The compiler is also allowed to reload sk->sk_socket after the NULL
check when it computes &sock->flags. That would undo the NULL check that
the commit message relies on to make the helpers "more robust".

This patch also adds a second plain read of sk->sk_socket to
sk_stream_write_space() and smc_tx_write_space(). Both functions already
had the pointer in a local sock variable.

sk_clear_nospace() below has the same plain load. It is still there at
the end of the series, after "tcp: add tp->tcp_nospace".

> +EXPORT_SYMBOL(sk_set_nospace);
> +
> +/**
> + *	sk_clear_nospace - tell the transport no writer is waiting for space
> + *	@sk: socket
> + *
> + *	Called from ->sk_write_space() handlers, once send space has been
> + *	made available to writers.
> + */

[Severity: Low]

Does this kernel-doc, and the one for sk_set_nospace() above, match how
the helpers are actually used?

This comment says the helper is "Called from ->sk_write_space() handlers".
kcm_tx_work() is a workqueue handler, though, and it clears the bit
before it calls ->sk_write_space():

net/kcm/kcmsock.c:kcm_tx_work() {
    ...
	if (likely(sk->sk_socket) &&
	    test_bit(SOCK_NOSPACE, &sk->sk_socket->flags)) {
		sk_clear_nospace(sk);
		sk->sk_write_space(sk);
	}
    ...
}

Later in the series, "sunrpc: use sk_set_nospace() and
sk_clear_nospace()" adds a call to it from svc_tcp_has_wspace() in
net/sunrpc/svcsock.c. That is an ->xpo_has_wspace check, not a
write_space callback.

The sk_set_nospace() comment says it "Must be called before the final
check of the available send space". Several converted callers, however,
set the bit on an -EAGAIN exit and never check again. There the point is
to arm a later ->sk_write_space() or EPOLLOUT:

net/core/stream.c:sk_stream_wait_memory() {
    ...
do_eagain:
    ...
	sk_set_nospace(sk);
	err = -EAGAIN;
	goto out;
}

The !timeo branches in smc_tx_wait() and sock_alloc_send_pskb() work
the same way.

The comment also does not say that sk_set_nospace() is only a set_bit()
with no barrier. Callers have to provide a full barrier between setting
the bit and re-checking. Today that comes from smp_mb__after_atomic() in
tcp_poll(), smc_poll() and mptcp_check_writeable(), and from
prepare_to_wait() in sock_wait_for_wmem().

Could the kernel-doc also cover the -EAGAIN use, the callers outside
write_space handlers, and the barrier the caller must provide?

> +void sk_clear_nospace(struct sock *sk)
> +{
> +	struct socket *sock = sk->sk_socket;
> +
> +	if (sock)
> +		clear_bit(SOCK_NOSPACE, &sock->flags);
> +}
> +EXPORT_SYMBOL(sk_clear_nospace);

[ ... ]

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