Date: Fri, 5 Nov 2021 07:08:30 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Hans Petter Selasky <hselasky@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 10a62eb109ce - main - Use layer five checksum flags in the mbuf packet header to pass on crypto state. Message-ID: <YYS8TkpXuTHPT8fD@kib.kiev.ua> In-Reply-To: <202111041754.1A4HsVkY050121@gitrepo.freebsd.org> References: <202111041754.1A4HsVkY050121@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Nov 04, 2021 at 05:54:31PM +0000, Hans Petter Selasky wrote: > The branch main has been updated by hselasky: > > URL: https://cgit.FreeBSD.org/src/commit/?id=10a62eb109ceafce32aa2b18ec835b3b7285c2dd > > commit 10a62eb109ceafce32aa2b18ec835b3b7285c2dd > Author: Hans Petter Selasky <hselasky@FreeBSD.org> > AuthorDate: 2021-11-04 17:43:24 +0000 > Commit: Hans Petter Selasky <hselasky@FreeBSD.org> > CommitDate: 2021-11-04 17:52:06 +0000 > > Use layer five checksum flags in the mbuf packet header to pass on crypto state. > > The mbuf protocol flags get cleared between layers, and also it was discovered > that M_DECRYPTED conflicts with M_HASFCS when receiving ethernet patckets. > > Add the proper CSUM_TLS_MASK and CSUM_TLS_DECRYPTED defines, and start using > these instead of M_DECRYPTED inside the TCP LRO code. > > This change is needed by coming TLS RX hardware offload support patches. > > Suggested by: kib@ > Reviewed by: jhb@ > MFC after: 1 week > Sponsored by: NVIDIA Networking > --- > sys/netinet/tcp_lro.c | 11 ++++++++++- > sys/sys/mbuf.h | 2 ++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/sys/netinet/tcp_lro.c b/sys/netinet/tcp_lro.c > index cb9681559777..ea23ad75994d 100644 > --- a/sys/netinet/tcp_lro.c > +++ b/sys/netinet/tcp_lro.c > @@ -395,7 +395,8 @@ tcp_lro_parser(struct mbuf *m, struct lro_parser *po, struct lro_parser *pi, boo > htons(m->m_pkthdr.ether_vtag) & htons(EVL_VLID_MASK); > } > /* Store decrypted flag, if any. */ > - if (__predict_false(m->m_flags & M_DECRYPTED)) > + if (__predict_false((m->m_pkthdr.csum_flags & > + CSUM_TLS_MASK) == CSUM_TLS_DECRYPTED)) > po->data.lro_flags |= LRO_FLAG_DECRYPTED; > } > > @@ -833,6 +834,8 @@ tcp_flush_out_entry(struct lro_ctrl *lc, struct lro_entry *le) > le->m_head->m_pkthdr.csum_flags = CSUM_DATA_VALID | > CSUM_PSEUDO_HDR | CSUM_IP_CHECKED | CSUM_IP_VALID; > le->m_head->m_pkthdr.csum_data = 0xffff; > + if (__predict_false(le->outer.data.lro_flags & LRO_FLAG_DECRYPTED)) > + le->m_head->m_pkthdr.csum_flags |= CSUM_TLS_DECRYPTED; > break; > case LRO_TYPE_IPV6_TCP: > csum = tcp_lro_update_checksum(&le->inner, le, > @@ -844,6 +847,8 @@ tcp_flush_out_entry(struct lro_ctrl *lc, struct lro_entry *le) > le->m_head->m_pkthdr.csum_flags = CSUM_DATA_VALID | > CSUM_PSEUDO_HDR; > le->m_head->m_pkthdr.csum_data = 0xffff; > + if (__predict_false(le->outer.data.lro_flags & LRO_FLAG_DECRYPTED)) > + le->m_head->m_pkthdr.csum_flags |= CSUM_TLS_DECRYPTED; > break; > case LRO_TYPE_NONE: > switch (le->outer.data.lro_type) { > @@ -854,6 +859,8 @@ tcp_flush_out_entry(struct lro_ctrl *lc, struct lro_entry *le) > le->m_head->m_pkthdr.csum_flags = CSUM_DATA_VALID | > CSUM_PSEUDO_HDR | CSUM_IP_CHECKED | CSUM_IP_VALID; > le->m_head->m_pkthdr.csum_data = 0xffff; > + if (__predict_false(le->outer.data.lro_flags & LRO_FLAG_DECRYPTED)) > + le->m_head->m_pkthdr.csum_flags |= CSUM_TLS_DECRYPTED; > break; > case LRO_TYPE_IPV6_TCP: > csum = tcp_lro_update_checksum(&le->outer, le, > @@ -862,6 +869,8 @@ tcp_flush_out_entry(struct lro_ctrl *lc, struct lro_entry *le) > le->m_head->m_pkthdr.csum_flags = CSUM_DATA_VALID | > CSUM_PSEUDO_HDR; > le->m_head->m_pkthdr.csum_data = 0xffff; > + if (__predict_false(le->outer.data.lro_flags & LRO_FLAG_DECRYPTED)) > + le->m_head->m_pkthdr.csum_flags |= CSUM_TLS_DECRYPTED; > break; > default: > break; > diff --git a/sys/sys/mbuf.h b/sys/sys/mbuf.h > index 413854cc9a57..d0f90805fa78 100644 > --- a/sys/sys/mbuf.h > +++ b/sys/sys/mbuf.h > @@ -721,6 +721,8 @@ m_epg_pagelen(const struct mbuf *m, int pidx, int pgoff) > #define CSUM_UDP_IPV6 CSUM_IP6_UDP > #define CSUM_TCP_IPV6 CSUM_IP6_TCP > #define CSUM_SCTP_IPV6 CSUM_IP6_SCTP > +#define CSUM_TLS_MASK (CSUM_L5_CALC|CSUM_L5_VALID) > +#define CSUM_TLS_DECRYPTED CSUM_L5_CALC It is worth explaining in a comment that selection of the value for CSUM_TLS_DECRYPTED does not reuse previous value, in the sense that correct use of the CSUM_L5 flags from drivers (are there any?) is still correct. We usurp a value for L5 CSUM flags which should not be returned from driver, to mean something new. It also may be worth discussing do we need L5 flags in its present form, and are there any existing consumers that do not abuse there semi-free flags for something else. If no external consumers exist (*) then perhaps officially re-purposing them is better route. We could define them as something extensible or at least context (inpcb etc)-dependent. (*) it seems that there is no in-tree users > > /* > * mbuf types describing the content of the mbuf (including external storage).
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YYS8TkpXuTHPT8fD>