Re: [PATCH net-next v2 6/7] tls: convert getsockopt to sockopt_t

[Sabrina Dubroca <sd@queasysnail.net>]

2026-07-20, 09:17:47 -0700, Breno Leitao wrote:
> Continue converting the proto-layer getsockopt callbacks to the sockopt_t
> interface, converting do_tls_getsockopt() and its per-option helpers to
> take a sockopt_t.
> 
> The thin tls_getsockopt() wrapper keeps its __user signature for now: it
> builds a user-backed sockopt_t with sockopt_init_user(), calls the helper,
> and writes the returned length back to optlen. The helpers use
> copy_to_iter() instead of copy_to_user(); the NULL optval check in the
> TLS_TX/TLS_RX path is preserved by testing the iterator user buffer.
> 
> No functional change.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

Looks ok, just a few small comments:

> -static int do_tls_getsockopt_conf(struct sock *sk, char __user *optval,
> -				  int __user *optlen, int tx)
> +static int do_tls_getsockopt_conf(struct sock *sk, sockopt_t *opt, int tx)
>  {
>  	int rc = 0;
>  	const struct tls_cipher_desc *cipher_desc;
>  	struct tls_context *ctx = tls_get_ctx(sk);
>  	struct tls_crypto_info *crypto_info;
>  	struct cipher_context *cctx;
> -	int len;
> +	int len = opt->optlen;
>  
> -	if (get_user(len, optlen))
> -		return -EFAULT;
> -
> -	if (!optval || (len < sizeof(*crypto_info))) {
> +	if (!opt->iter_out.ubuf || len < sizeof(*crypto_info)) {

Not something about your patch but... I really wonder what this NULL
check was trying to accomplish. The other getsockopts in tls don't
have one, I don't think the rest of networkng does that either.


> @@ -591,12 +574,25 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
>  			  char __user *optval, int __user *optlen)
>  {
>  	struct tls_context *ctx = tls_get_ctx(sk);
> +	sockopt_t opt;
> +	int err;
>  
>  	if (level != SOL_TLS)
>  		return ctx->sk_proto->getsockopt(sk, level,
>  						 optname, optval, optlen);
>  
> -	return do_tls_getsockopt(sk, optname, optval, optlen);
> +	err = sockopt_init_user(&opt, optval, optlen);
> +	if (err)
> +		return err;
> +
> +	err = do_tls_getsockopt(sk, optname, &opt);
> +	if (err)
> +		return err;
> +
> +	if (put_user(opt.optlen, optlen))
> +		return -EFAULT;

One of the sashikos complains that we're now writing the length with
put_user in cases where we didn't before. I don't think we need to
care, but if someone complains, we could make this conditional on
optlen having been changed by the handler.

It also complains that optlen was getting updated on EFAULT and now
it's not. There's possibly some code out there that's crazy enough to
pass a bogus buffer to get the size it should have provided?

Also some complaints about "what if optlen is negative". I think we
can ignore all of that.

-- 
Sabrina