Re: [PATCH net-next 3/5] tls: rx: add sockopt for enabling optimistic decrypt with TLS 1.3

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 8 Jul 2022 14:14:44 +0000 Maxim Mikityanskiy wrote:
> On Tue, 2022-07-05 at 16:59 -0700, Jakub Kicinski wrote:
> > +static int do_tls_getsockopt_no_pad(struct sock *sk, char __user *optval,
> > +				    int __user *optlen)
> > +{
> > +	struct tls_context *ctx = tls_get_ctx(sk);
> > +	unsigned int value;
> > +	int err, len;
> > +
> > +	if (ctx->prot_info.version != TLS_1_3_VERSION)
> > +		return -EINVAL;
> > +
> > +	if (get_user(len, optlen))
> > +		return -EFAULT;
> > +	if (len < sizeof(value))
> > +		return -EINVAL;
> > +
> > +	lock_sock(sk);
> > +	err = -EINVAL;
> > +	if (ctx->rx_conf == TLS_SW || ctx->rx_conf == TLS_HW)
> > +		value = ctx->rx_no_pad;
> > +	release_sock(sk);
> > +	if (err)
> > +		return err;  
> 
> Bug: always returns -EINVAL here, because it's assigned a few lines
> above unconditionally.

Ah, thanks. Let me add a self-test while at it.

> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > index 2bac57684429..7592b6519953 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -1601,6 +1601,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
> >  	if (unlikely(darg->zc && prot->version == TLS_1_3_VERSION &&
> >  		     darg->tail != TLS_RECORD_TYPE_DATA)) {
> >  		darg->zc = false;
> > +		TLS_INC_STATS(sock_net(sk), LINUX_MIN_TLSDECRYPTRETRY);
> >  		return decrypt_skb_update(sk, skb, dest, darg);
> >  	}  
> 
> I recall you planned to have two counters:
> 
> > You have a point about the more specific counter, let me add a
> > counter for NoPad being violated (tail == 0) as well as the overall
> > "decryption happened twice" counter.  
> 
> Did you decide to stick with one?

I was going back and forth on whether it's "worth the memory" because 
I was considering breaking the counters out per socket. At least that's
what I recall, it was like 3 rewrites ago, getting rid of strparser was
tricky. But I never made the stats per sock so let me add it. Also I
think s/MIN/MIB/ in the name of the retry?

Thanks for the review!