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
| ||
|
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>
Powered by blists - more mailing lists