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]

Message-ID: <1317ab48c253a040dc15b9c458ce640ac2257b9b.camel@redhat.com>
Date:   Tue, 26 Jul 2022 19:26:51 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
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, 2022-07-26 at 10:01 -0700, Jakub Kicinski wrote:
> 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.

I'm fine with your replies here. I'm ok with the code as-is. I think
this last bits could be updated with a later patch, if needed.

Acked-by: Paolo Abeni <pabeni@...hat.com>