From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 7EFA2A0C4A;
Wed, 7 Jul 2021 13:23:17 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 526AD406FF;
Wed, 7 Jul 2021 13:23:17 +0200 (CEST)
Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com
[209.85.215.180])
by mails.dpdk.org (Postfix) with ESMTP id EE83C406B4
for <dev@dpdk.org>; Wed, 7 Jul 2021 13:23:15 +0200 (CEST)
Received: by mail-pg1-f180.google.com with SMTP id f5so1896832pgv.3
for <dev@dpdk.org>; Wed, 07 Jul 2021 04:23:15 -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=Peq5DZYfPP3GZovV7LQTaeqfyEVFR7EauK0gahL49ao=;
b=qjBo8fHOpVGq7p0a5plo7wXm9iq1hotx9njaSHKO7Jkp1nmfw4WsUEWhc8EICIc3G7
wswwsyPNMohNlHeMOevcswZcqy+4JLAufH1SiZBEF8lowNg4Dd2hlKABmiAy+r3F4jGE
VJQOti9ksWG8nxqlL9fzq6OiDQjg78FMoGhsKuCFkx2wIUkRfQDRVeuR094BXQ1gNBl5
GGAa/pOUwrIdm+5ORWoSikk8GFIEKiOgRDubIszoipAT6igfvNNAamah04MhIRwgwGNL
1jpXUV282yWe6Obk2IMzExSPShiVCiJbqhMES/n95uwboQEAmDwEwaUCxd9zffAP+eDZ
Y4GA==
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=Peq5DZYfPP3GZovV7LQTaeqfyEVFR7EauK0gahL49ao=;
b=Uhb5jU6/+otBxMUUllsFzoFqfZ+NUmV+tFX5zredDnHVkh2uD+P72JJASfbUsT6pYF
LI5nxQuDneCunQ1h0y4aMEdDcif+a0Sv6dwCJi9a5A3n5KNxBg00cHhvndDuVFHe10oj
hKM110nHa64DCFRKvle789qf12QrtigYmrOlEZxwU0gcVNYXZjhCbPm+xpXCewOZtfyC
0gH6WnnBYF9cO4/CtGP71xJlwYKEcXaHFYS1UFKmJhcqB/q3KPBujIG7+mpmZ7Ns09EC
LCTU0QMDOdZ4z7UQTwNjxIii67EBvCVkszMtHwZVHqXQdCngr1j8xHuxH+q7WZlSJeBp
eFlQ==
X-Gm-Message-State: AOAM532Mtll8dqN1fVCzmTPmpm9sHZqPuI/99SHbKW+KNsKYPxGtMLMm
5O5VtofPjluG3evpmRILqvo=
X-Google-Smtp-Source: ABdhPJzolnBouf594p/snFsOgQyRWbESVFwHgqQ4nSDw52g6VFTuDa/wWI+Yszn4Am1Gs6kGeGfQ5Q==
X-Received: by 2002:a62:3244:0:b029:308:22b0:52ff with SMTP id
y65-20020a6232440000b029030822b052ffmr24743659pfy.68.1625656995060;
Wed, 07 Jul 2021 04:23:15 -0700 (PDT)
Received: from gmail.com ([1.6.215.26])
by smtp.gmail.com with ESMTPSA id l83sm14831321pfd.140.2021.07.07.04.23.11
(version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128);
Wed, 07 Jul 2021 04:23:14 -0700 (PDT)
Date: Wed, 7 Jul 2021 16:52:48 +0530
From: Nithin Dabilpuram <nithind1988@gmail.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Cc: Akhil Goyal <gakhil@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"g.singh@nxp.com" <g.singh@nxp.com>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>,
"Zhang, Roy Fan" <roy.fan.zhang@intel.com>,
"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
"jerinj@marvell.com" <jerinj@marvell.com>
Message-ID: <YOWOiHz6knkO7PYM@gmail.com>
References: <20210624102848.3878788-1-gakhil@marvell.com>
<DM6PR11MB449123EB3FD6B425A7F5E1C79A1B9@DM6PR11MB4491.namprd11.prod.outlook.com>
<YORL8sHOiuIed+Ey@gmail.com>
<DM6PR11MB4491154742EB1963AD59D0839A1B9@DM6PR11MB4491.namprd11.prod.outlook.com>
<YORTaLXgzv2RXuEp@gmail.com>
<DM6PR11MB4491CCB767ED325BC7F74FFB9A1B9@DM6PR11MB4491.namprd11.prod.outlook.com>
<YOVuHUFWSl3XBnNC@gmail.com>
<DM6PR11MB4491FF183E595D7625D950C79A1A9@DM6PR11MB4491.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DM6PR11MB4491FF183E595D7625D950C79A1A9@DM6PR11MB4491.namprd11.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On Wed, Jul 07, 2021 at 09:59:10AM +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 <ndabilpuram@marvell.com>
> > > > > > > > Reviewed-by: Akhil Goyal <gakhil@marvell.com>
> > > > > > > > ---
> > > > > > > > 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 ?
>
> >
> > 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.
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.
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(). 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,
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
> > > > > > >