netdev - Re: [PATCH net-next v3 7/7] tls: rx: do not use the standard strparser

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Jul 2022 10:01:12 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        borisp@...dia.com, john.fastabend@...il.com, maximmi@...dia.com,
        tariqt@...dia.com, vfedorenko@...ek.ru
Subject: Re: [PATCH net-next v3 7/7] tls: rx: do not use the standard
 strparser

On Tue, 26 Jul 2022 11:27:36 +0200 Paolo Abeni wrote:
> On Fri, 2022-07-22 at 16:50 -0700, Jakub Kicinski wrote:
> > diff --git a/net/tls/tls.h b/net/tls/tls.h
> > index 154a3773e785..0e840a0c3437 100644
> > --- a/net/tls/tls.h
> > +++ b/net/tls/tls.h
> > @@ -1,4 +1,5 @@
> >  /*
> > + * Copyright (c) 2016 Tom Herbert <tom@...bertland.com>  
> 
> It's a little strange to me the above line ??! digging this file
> history, you created it out of include/net/tls.h and the latter was
> originally authored by Dave Watson (modulo ENOCOFFEE here...)
> 
> > +++ b/net/tls/tls_strp.c
> > @@ -1,37 +1,493 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2016 Tom Herbert <tom@...bertland.com> */  
> 
> Same here ...

I tried to add the Copyrights as I copied some code around, since I'm
lazy around legal stuff. I think I copied parts of the strparser
at some point and the structure definition (workqueue handling?). 
I'd rather keep too many copyrights than too few, tho. 
The semi-custom license is more annoying :(

> > +static int tls_strp_copyin(read_descriptor_t *desc, struct sk_buff *in_skb,
> > +			   unsigned int offset, size_t in_len)
> > +{
> > +	struct tls_strparser *strp = (struct tls_strparser *)desc->arg.data;
> > +	size_t sz, len, chunk;
> > +	struct sk_buff *skb;
> > +	skb_frag_t *frag;
> > +
> > +	if (strp->msg_ready)
> > +		return 0;
> > +
> > +	skb = strp->anchor;
> > +	frag = &skb_shinfo(skb)->frags[skb->len / PAGE_SIZE];  
> 
> I'm wondering if TSOv2 GRO packets can reach here? Even without TSO v2,
> I *think* the TCP stack is allowed to grow queued skbs above 64K via
> tcp_queue_rcv()/tcp_try_coalesce().

I don't think the TSO skbs can get here, the @skb is completely
constructed by me and the length is bounded by max TLS record size
(16k + overheads). We should be safe to use 4k pages, I think.

> > +static int tls_strp_read_copyin(struct tls_strparser *strp)
> > +{
> > +	struct socket *sock = strp->sk->sk_socket;
> > +	read_descriptor_t desc;
> > +
> > +	desc.arg.data = strp;
> > +	desc.error = 0;
> > +	desc.count = 1; /* give more than one skb per call */
> > +
> > +	/* sk should be locked here, so okay to do read_sock */
> > +	sock->ops->read_sock(strp->sk, &desc, tls_strp_copyin);  
> 
> If you are concerned by indirect calls/retpoline, you can use directly
> tcp_read_sock here, as read_sock is always tcp_read_sock since commit
> 965b57b469a589d64d81b1688b38dcb537011bb0. Or you can use
> indirect_call_wrapper.h

This is a slowpath which only gets triggered if we are so rbuf
constrained that TCP will not be able to buffer a full record.
Otherwise we try to avoid doing any copying/read_sock at all.

> > -int tls_strp_msg_hold(struct sock *sk, struct sk_buff *skb,
> > -		      struct sk_buff_head *dst)
> > +/* strp must already be stopped so that tls_strp_recv will no longer be called.
> > + * Note that tls_strp_done is not called with the lower socket held.
> > + */
> > +void tls_strp_done(struct tls_strparser *strp)
> >  {
> > -	struct sk_buff *clone;
> > +	WARN_ON(!strp->stopped);
> >  
> > -	clone = skb_clone(skb, sk->sk_allocation);
> > -	if (!clone)
> > +	cancel_work_sync(&strp->work);
> > +	tls_strp_anchor_free(strp);
> > +}
> > +
> > +int __init tls_strp_dev_init(void)
> > +{
> > +	tls_strp_wq = create_singlethread_workqueue("kstrp");  
> 
> I guess it's better to change the name to avoid confusing with plain
> strparser ?!?
> 
> Out of sheer ignorance and not related to this patch: If I read
> correctly, the above means that multiple tls flows on top of different
> TCP sockets will use a single CPU, isn't that a relevant bottle-neck?
> isn't enough to rely on queue_work() to submit the work on the same CPU
> that just did the TCP stack processing? 

Oh yeah, this is a slow/rare path too but you're right. I copied this
code from strparser without thinking, not sure what the motivation was
there.