From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 71405A0C4E; Mon, 12 Jul 2021 19:02:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 56C944069E; Mon, 12 Jul 2021 19:02:14 +0200 (CEST) Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by mails.dpdk.org (Postfix) with ESMTP id 54A394069D for ; Mon, 12 Jul 2021 19:02:12 +0200 (CEST) Received: by mail-pj1-f54.google.com with SMTP id p4-20020a17090a9304b029016f3020d867so372604pjo.3 for ; Mon, 12 Jul 2021 10:02:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=V9dUuC4xnawLCK/k3Bgfeo1W/ouu9G7ze4tRnA47ZC0=; b=X1JQBod62lciAeoG2TQxgrTzylBwYzWENjbW5ZAEcC3FYqIV+5QXi9UfCZfzFOmW85 u4RHSkv6+zE4gSE1jlySDJnhnMGSOOzJ8ZnzIJyifcqV2Ur+IXYKqLUv+qBHtjcZNYFq WqrkwDWB8VIvhrGMepZ29yprWe+AWXsSbIG1saBClixxMsc1UBa5Y6yqKvRsbEICc6fF uya2efgLPoIY3dLCY3/sKOR0Z5MzODvbRM+BZdn5qPVPQgKcUDRoAf0oeuV+7PqzuLug z79j09tU0uH8gjfP6XVRCQqG7icfclpFFCxjjgWrBsn1YntQfdWxaB36X3Hw6aW0tiqZ h8HQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=V9dUuC4xnawLCK/k3Bgfeo1W/ouu9G7ze4tRnA47ZC0=; b=QQIxW1/8iUuBCNd4Q2/7WBcRuxFXbCEecwDd87StZdYBSs54F15YV164X/6LiKtg/V og/ZFA81qdFKBRXUERlwKWndKL6GRotys/iIn5/hTj3Wkyi6E9TnJCR5fvTn9rWKomOB VcSZjYdndQApzDfTPpb6o8Oc51wkKfckQHLIkndDv30dDTvW561Bk+LQkeqD9Ew5Phew OMxf9S5gF9IjF/eNBqbN+SpBaiBSPMdWGW0c/YNsMQEpWjLuhYavR1rJHcv1kUUCMSfn EPmcOc/l84ZmojfCttc5RxtDc5sLCWz4+EoHW7c9pK+XL+YNmAActnaegfpzcGxz2sP5 FtZQ== X-Gm-Message-State: AOAM531NJHqAtZo/V+dvm+ytzJAUjVgLID88pqL7g3uzyuG5wIwQKnmD 7ZP4yG5CqdiHCumpfADbgwA= X-Google-Smtp-Source: ABdhPJwO6qGcmURYZpT9H9XW7WldqPuDvolwXLAWaXLCdGoLyhrqqMtzIpecWtqYHZ+AX/KoXeyiWA== X-Received: by 2002:a17:902:c941:b029:12b:27b:a7b0 with SMTP id i1-20020a170902c941b029012b027ba7b0mr110320pla.10.1626109331401; Mon, 12 Jul 2021 10:02:11 -0700 (PDT) Received: from gmail.com ([1.6.215.26]) by smtp.gmail.com with ESMTPSA id n32sm6943798pfv.59.2021.07.12.10.02.08 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 12 Jul 2021 10:02:10 -0700 (PDT) Date: Mon, 12 Jul 2021 22:31:31 +0530 From: Nithin Dabilpuram To: "Ananyev, Konstantin" Cc: Akhil Goyal , "dev@dpdk.org" , "hemant.agrawal@nxp.com" , "thomas@monjalon.net" , "g.singh@nxp.com" , "Yigit, Ferruh" , "Zhang, Roy Fan" , "olivier.matz@6wind.com" , "jerinj@marvell.com" Message-ID: References: <20210624102848.3878788-1-gakhil@marvell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH 1/2] security: enforce semantics for Tx inline processing X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Sat, Jul 10, 2021 at 12:57:19PM +0000, Ananyev, Konstantin wrote: > > > > > > > > > > > For Tx inline processing, when RTE_SECURITY_TX_OLOAD_NEED_MDATA is > > > > > > > > > > set, rte_security_set_pkt_metadata() needs to be called for pkts > > > > > > > > > > to associate a Security session with a mbuf before submitting > > > > > > > > > > to Ethdev Tx. This is apart from setting PKT_TX_SEC_OFFLOAD in > > > > > > > > > > mbuf.ol_flags. rte_security_set_pkt_metadata() is also used to > > > > > > > > > > set some opaque metadata in mbuf for PMD's use. > > > > > > > > > > This patch updates documentation that rte_security_set_pkt_metadata() > > > > > > > > > > should be called only with mbuf containing Layer 3 and above data. > > > > > > > > > > This behaviour is consistent with existing PMD's such as ixgbe. > > > > > > > > > > > > > > > > > > > > On Tx, not all net PMD's/HW can parse packet and identify > > > > > > > > > > L2 header and L3 header locations on Tx. This is inline with other > > > > > > > > > > Tx offloads requirements such as L3 checksum, L4 checksum offload, > > > > > > > > > > etc, where mbuf.l2_len, mbuf.l3_len etc, needs to be set for > > > > > > > > > > HW to be able to generate checksum. Since Inline IPSec is also > > > > > > > > > > such a Tx offload, some PMD's at least need mbuf.l2_len to be > > > > > > > > > > valid to find L3 header and perform Outbound IPSec processing. > > > > > > > > > > Hence, this patch updates documentation to enforce setting > > > > > > > > > > mbuf.l2_len while setting PKT_TX_SEC_OFFLOAD in mbuf.ol_flags > > > > > > > > > > for Inline IPSec Crypto / Protocol offload processing to > > > > > > > > > > work on Tx. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Nithin Dabilpuram > > > > > > > > > > Reviewed-by: Akhil Goyal > > > > > > > > > > --- > > > > > > > > > > doc/guides/nics/features.rst | 2 ++ > > > > > > > > > > doc/guides/prog_guide/rte_security.rst | 6 +++++- > > > > > > > > > > lib/mbuf/rte_mbuf_core.h | 2 ++ > > > > > > > > > > 3 files changed, 9 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst > > > > > > > > > > index 403c2b03a..414baf14f 100644 > > > > > > > > > > --- a/doc/guides/nics/features.rst > > > > > > > > > > +++ b/doc/guides/nics/features.rst > > > > > > > > > > @@ -430,6 +430,7 @@ of protocol operations. See Security library and PMD documentation for more deta > > > > > > > > > > > > > > > > > > > > * **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > * **[uses] rte_eth_txconf,rte_eth_txmode**: ``offloads:DEV_TX_OFFLOAD_SECURITY``. > > > > > > > > > > +* **[uses] mbuf**: ``mbuf.l2_len``. > > > > > > > > > > * **[implements] rte_security_ops**: ``session_create``, ``session_update``, > > > > > > > > > > ``session_stats_get``, ``session_destroy``, ``set_pkt_metadata``, ``capabilities_get``. > > > > > > > > > > * **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > @@ -451,6 +452,7 @@ protocol operations. See security library and PMD documentation for more details > > > > > > > > > > > > > > > > > > > > * **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > * **[uses] rte_eth_txconf,rte_eth_txmode**: ``offloads:DEV_TX_OFFLOAD_SECURITY``. > > > > > > > > > > +* **[uses] mbuf**: ``mbuf.l2_len``. > > > > > > > > > > * **[implements] rte_security_ops**: ``session_create``, ``session_update``, > > > > > > > > > > ``session_stats_get``, ``session_destroy``, ``set_pkt_metadata``, ``get_userdata``, > > > > > > > > > > ``capabilities_get``. > > > > > > > > > > diff --git a/doc/guides/prog_guide/rte_security.rst b/doc/guides/prog_guide/rte_security.rst > > > > > > > > > > index f72bc8a78..7b68c698d 100644 > > > > > > > > > > --- a/doc/guides/prog_guide/rte_security.rst > > > > > > > > > > +++ b/doc/guides/prog_guide/rte_security.rst > > > > > > > > > > @@ -560,7 +560,11 @@ created by the application is attached to the security session by the API > > > > > > > > > > > > > > > > > > > > For Inline Crypto and Inline protocol offload, device specific defined metadata is > > > > > > > > > > updated in the mbuf using ``rte_security_set_pkt_metadata()`` if > > > > > > > > > > -``DEV_TX_OFFLOAD_SEC_NEED_MDATA`` is set. > > > > > > > > > > +``RTE_SECURITY_TX_OLOAD_NEED_MDATA`` is set. ``rte_security_set_pkt_metadata()`` > > > > > > > > > > +should be called on mbuf only with Layer 3 and above data present and > > > > > > > > > > +``mbuf.data_off`` should be pointing to Layer 3 Header. > > > > > > > > > > > > > > > > > > Hmm... not sure why mbuf.data_off should point to L3 hdr. > > > > > > > > > Who will add L2 hdr to the packet in that case? > > > > > > > > > Or did you mean ``mbuf.data_off + mbuf.l2_len`` here? > > > > > > > > > > > > > > > > That is the semantics I was trying to define. I think below are the sequence of > > > > > > > > operations to be done for ipsec processing, > > > > > > > > > > > > > > > > 1. receive_pkt() > > > > > > > > 2. strip_l2_hdr() > > > > > > > > 3. Do policy lookup () > > > > > > > > 4. Call rte_security_set_pkt_metadata() if pkt needs to be encrypted with a > > > > > > > > particular SA. Now pkt only has L3 and above data. > > > > > > > > 5. Do route_lookup() > > > > > > > > 6. add_l2hdr() which might be different from stripped l2hdr. > > > > > > > > 7. Send packet out. > > > > > > > > > > > > > > > > The above sequence is what I believe the current poll mode worker thread in > > > > > > > > ipsec-secgw is following. > > > > > > > > > > > > > > That's just a sample app, it doesn't mean it has to be the only possible way. > > > > > > > > > > > > > > > While in event mode, step 2 and step 6 are missing. > > > > > > > > > > > > > > I think this L2 hdr manipulation is totally optional. > > > > > > > If your rte_security_set_pkt_metadata() implementation really needs to know L3 hdr offset (not sure why?), > > > > > > Since rte_security_set_pkt_metadata() is PMD specific function ptr call, we are currently doing some pre-processing > > > > > > here before submitting packet to inline IPSec via rte_eth_tx_burst(). This saves us cycles later in rte_eth_tx_burst(). > > > > > > If we cannot know for sure, the pkt content at the time of rte_security_set_pkt_metadata() call, then I think > > > > > > having a PMD specific callback is not much of use except for saving SA priv data to rte_mbuf. > > > > > > > > > > > > > then I suppose we can add a requirement that l2_len has to be set properly before calling rte_security_set_pkt_metadata(). > > > > > > > > > > > > This is also fine with us. > > > > > > > > > > Ok, so to make sure we are on the same page, you propose: > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set. > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified? > > > > Yes. > > > > > > > > > > > > > > Is that correct understanding? > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept? > > > > > > > > Since our PMD doesn't have a prepare function, I missed that but, since > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare() > > > > are callbacks from same PMD, do you see any issue ? > > > > > > > > The restriction is from user side, data is not supposed to be modified unless > > > > rte_security_set_pkt_metadata() is called again. > > > > > > Yep, I do have a concern here. > > > Right now it is perfectly valid to do something like that: > > > rte_security_set_pkt_metadata(..., mb, ...); > > > /* can modify contents of the packet */ > > > rte_eth_tx_prepare(..., &mb, 1); > > > rte_eth_tx_burst(..., &mb, 1); > > > > > > With the new restrictions you are proposing it wouldn't be allowed any more. > > You can still modify L2 header and IPSEC is only concerned about L3 and above. > > > > I think insisting that rte_security_set_pkt_metadata() be called after all L3 > > and above header modifications is no a problem. I guess existing ixgbe/txgbe > > PMD which are the ones only implementing the call back are already expecting the > > same ? > > AFAIK, no there are no such requirements for ixgbe or txgbe. > All that ixgbe callback does - store session related data inside mbuf. > It's only expectation to have ESP trailer at the proper place (after ICV): This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't have ESP trailer updated or when mbuf->pkt_len = 0 > > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *) > rte_security_dynfield(m); > mdata->enc = 1; > mdata->sa_idx = ic_session->sa_index; > mdata->pad_len = ixgbe_crypto_compute_pad_len(m); > > Then this data will be used by tx_burst() function. So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(), mbuf data / packet len cannot be modified right as if modified, then tx_burst() will be using incorrect pad len ? This patch is also trying to add similar restriction on when rte_security_set_pkt_metadata() should be called and what cannot be done after calling rte_security_set_pkt_metadata(). > > > > > > > > > > > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for > > > > security, > > > > > > Yes, that was my thought. > > > > > > > my only argument was that since there is already a hit in > > > > rte_security_set_pkt_metadata() to PMD specific callback and > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to > > > > do security related pre-processing there. > > > > > > Yes, it would be extra callback call that way. > > > Though tx_prepare() accepts burst of packets, so the overhead > > > of function call will be spread around the whole burst, and I presume > > > shouldn't be too high. > > > > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and > > > > non-security pkts. > > > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which > > > modifications are required for the packet. > > > > But the major issues I see are > > > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag. > > In our case, we need to know the security session details to do things. > > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no? We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata() just for storing session pointer in rte_security_dynfield consumes unnecessary cycles per pkt. > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time > > macro RTE_ETHDEV_TX_PREPARE_NOOP. > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate > > struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same. > > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf, > specially for that - to store secuiryt related data inside the mbuf. > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal. > > > So I think instead of enforcing yet another callback tx_prepare() for inline security > > processing, it can be done via security specific set_pkt_metadata(). > > But what you proposing introduces new limitations and might existing functionality. > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst() > itself is not an option? We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for rte_security_set_pkt_metadata(). Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not set, then, user needs to update struct rte_security_session's sess_private_data in a in rte_security_dynfield like below ? static inline void inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[], uint16_t num) { uint32_t i, ol_flags; ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA; for (i = 0; i != num; i++) { mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD; if (ol_flags != 0) rte_security_set_pkt_metadata(ss->security.ctx, ss->security.ses, mb[i], NULL); else *rte_security_dynfield(mb[i]) = (uint64_t)ss->security.ses->sess_private_data; If the above can be done, then in our PMD, we will not have a callback for set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set in capabilities. > > > I'm fine to > > introduce a burst call for the same(I was thinking to propose it in future) to > > compensate for the overhead. > > > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and > > rte_mbuf had space for struct rte_security_session pointer, > > But it does, see above. > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field. > If your PMD requires more data to be associated with mbuf > - you can request it via mbuf_dynfield and store there whatever is needed. > > > then then I guess it would have been better to do the way you proposed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to enforce semantics as above so that > > > > > > > > rte_security_set_pkt_metadata() can predict what comes in the pkt when he is > > > > > > > > called. > > > > > > > > > > > > > > > > I also think above sequence is what Linux kernel stack or other stacks follow. > > > > > > > > Does it makes sense ? > > > > > > > > > > > > > > > > > > > > > > > > > > > Once called, > > > > > > > > > > +Layer 3 and above data cannot be modified or moved around unless > > > > > > > > > > +``rte_security_set_pkt_metadata()`` is called again. > > > > > > > > > > > > > > > > > > > > For inline protocol offloaded ingress traffic, the application can register a > > > > > > > > > > pointer, ``userdata`` , in the security session. When the packet is received, > > > > > > > > > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h > > > > > > > > > > index bb38d7f58..9d8e3ddc8 100644 > > > > > > > > > > --- a/lib/mbuf/rte_mbuf_core.h > > > > > > > > > > +++ b/lib/mbuf/rte_mbuf_core.h > > > > > > > > > > @@ -228,6 +228,8 @@ extern "C" { > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > * Request security offload processing on the TX packet. > > > > > > > > > > + * To use Tx security offload, the user needs to fill l2_len in mbuf > > > > > > > > > > + * indicating L2 header size and where L3 header starts. > > > > > > > > > > */ > > > > > > > > > > #define PKT_TX_SEC_OFFLOAD (1ULL << 43) > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > 2.25.1 > > > > > > > > >