Re: [PATCH v3 4/4] nvme-tcp: support IPv6 traffic class

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The NVMe TCP target silently fails to set the IPv6 traffic class because it passes a 1-byte option length for IPV6_TCLASS.
--

--- Patch [4]: [PATCH v3 4/4] nvme-tcp: support IPv6 traffic class ---
commit f06d75be38a565495a1d84b7e856f1a1a552b88e
Author: Geliang Tang <[email protected]>

nvme-tcp: support IPv6 traffic class

This commit extends the NVMe TCP host queue socket setup to support the IPv6
traffic class. It applies the TOS value to IPv6 sockets via the IPV6_TCLASS
socket option.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 3bc41a4735d94..feedd46bd3a2b 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
[ ... ]
> @@ -1815,6 +1815,15 @@ static void nvme_tcp_sock_set_tos(struct sock *sk, int tos)
>  			   KERNEL_SOCKPTR(&tos), sizeof(tos));
>  }
>  
> +static void nvme_tcp_sock_set_tclass(struct sock *sk, int tclass)
> +{
> +#if IS_ENABLED(CONFIG_IPV6)
> +	if (sk->sk_family == AF_INET6)
> +		do_sock_setsockopt(sk->sk_socket, false, SOL_IPV6, IPV6_TCLASS,
> +				   KERNEL_SOCKPTR(&tclass), sizeof(tclass));

While reviewing this patch, I noticed a related bug in the target implementation
which handles this exact same logic.

In drivers/nvme/target/tcp.c:nvmet_tcp_sock_set_tclass():

	u8 tclass = ip6_tclass(inet6_sk(sk)->rcv_flowinfo);

	if (tclass > 0)
		do_sock_setsockopt(sk->sk_socket, false, SOL_IPV6,
				   IPV6_TCLASS, KERNEL_SOCKPTR(&tclass),
				   sizeof(tclass));
                                   ^^^^^^^^^^^^^^

[Severity: Medium]
Will this setsockopt call fail silently in the target code?

The tclass variable is declared as a u8, so sizeof(tclass) evaluates to 1. The
IPv6 networking core strictly requires an option length of at least 4 bytes for
IPV6_TCLASS.

Passing sizeof(u8) causes do_ipv6_setsockopt() to return -EINVAL, which means
the IPv6 traffic class is silently ignored instead of being applied to the
socket, leading to incorrect QoS behavior without alerting the user.

Does the target code need to be updated to pass a 4-byte length?

> +#endif
> +}
> +

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