Re: [PATCH v3 2/4] nvme-tcp: unify sockopt with do_sock_setsockopt

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Routing internal kernel socket configuration through user-facing do_sock_setsockopt() ignores return values and subjects critical NVMe settings to userspace BPF hooks.
--

--- Patch [2]: [PATCH v3 2/4] nvme-tcp: unify sockopt with do_sock_setsockopt ---
commit 2f6cd2b103bf52f5964e62f02552a246d7faa461
Author: Geliang Tang <[email protected]>

nvme-tcp: unify sockopt with do_sock_setsockopt

This patch consolidates socket option settings in nvme-tcp by utilizing
the generic do_sock_setsockopt() helper. It converts several direct socket
configuration calls to this unified mechanism.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index ce03a0ea4ded7..3bc41a4735d94 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1774,6 +1774,47 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
>  	return ret;
>  }
>  
> +static void nvme_tcp_sock_no_linger(struct sock *sk)
> +{
> +	struct linger ling = { .l_onoff = 1, .l_linger = 0 };
> +
> +	do_sock_setsockopt(sk->sk_socket, false, SOL_SOCKET, SO_LINGER,
> +			   KERNEL_SOCKPTR(&ling), sizeof(ling));
> +}

[Severity: High]
Does routing internal socket configuration through do_sock_setsockopt() expose
these settings to userspace BPF hooks and LSMs?

Since the kernel socket inherits the calling process's cgroup, a 
BPF_CGROUP_SETSOCKOPT hook or LSM rule could intercept and deny the 
operation. Because the return value is ignored here, would this silently 
fail to apply SO_LINGER?

[ ... ]

> +static void nvme_tcp_sock_set_nodelay(struct sock *sk)
> +{
> +	int val = 1;
> +
> +	do_sock_setsockopt(sk->sk_socket, false, SOL_TCP, TCP_NODELAY,
> +			   KERNEL_SOCKPTR(&val), sizeof(val));
> +}

[Severity: High]
If do_sock_setsockopt() is intercepted or fails, this also ignores the return
value. Could this leave Nagle's algorithm enabled on a storage protocol,
leading to severe performance regressions?

[ ... ]

> @@ -1819,24 +1860,24 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>  #endif
>  
>  	/* Single syn retry */
> -	tcp_sock_set_syncnt(queue->sock->sk, 1);
> +	nvme_tcp_sock_set_syncnt(queue->sock->sk, 1);
>  
>  	/* Set TCP no delay */
> -	tcp_sock_set_nodelay(queue->sock->sk);
> +	nvme_tcp_sock_set_nodelay(queue->sock->sk);

[Severity: High]
Executing these wrappers in sequence without checking for errors means that
failures are silently ignored. Can this result in an incorrectly configured
socket being used for NVMe over TCP connections?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2