Re: git: 10a62eb109ce - main - Use layer five checksum flags in the mbuf packet header to pass on crypto state.

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).