Re: [dpdk-dev] [EXT] Re: [PATCH] ethdev: allow multiple security sessions to use one rte flow

[Anoob Joseph <anoobj@marvell.com>]

Hi Ori,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ori Kam
> Sent: Thursday, January 16, 2020 5:06 PM
> To: Anoob Joseph <anoobj@marvell.com>; Medvedkin, Vladimir
> <vladimir.medvedkin@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>; Doherty, Declan
> <declan.doherty@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Ankur Dwivedi <adwivedi@marvell.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Matan Azrad <matan@mellanox.com>; Nicolau,
> Radu <radu.nicolau@intel.com>; Shahaf Shuler <shahafs@mellanox.com>;
> Narayana Prasad Raju Athreya <pathreya@marvell.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] ethdev: allow multiple security
> sessions to use one rte flow
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> > Sent: Tuesday, January 14, 2020 11:28 AM
> > To: Ori Kam <orika@mellanox.com>; Medvedkin, Vladimir
> > <vladimir.medvedkin@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> > Adrien Mazarguil <adrien.mazarguil@6wind.com>; Doherty, Declan
> > <declan.doherty@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
> > <thomas@monjalon.net>
> > Cc: Ankur Dwivedi <adwivedi@marvell.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Matan Azrad <matan@mellanox.com>;
> Nicolau,
> > Radu <radu.nicolau@intel.com>; Shahaf Shuler <shahafs@mellanox.com>;
> > Narayana Prasad Raju Athreya <pathreya@marvell.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] ethdev: allow multiple
> > security sessions to use one rte flow
> >
> > Hi Ori,
> >
> > Please see inline.
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: Ori Kam <orika@mellanox.com>
> > > Sent: Thursday, January 9, 2020 1:06 PM
> > > To: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>; Ananyev,
> > > Konstantin <konstantin.ananyev@intel.com>; Anoob Joseph
> > > <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>; Adrien
> > > Mazarguil <adrien.mazarguil@6wind.com>; Doherty, Declan
> > > <declan.doherty@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
> > > <thomas@monjalon.net>
> > > Cc: Ankur Dwivedi <adwivedi@marvell.com>; Hemant Agrawal
> > > <hemant.agrawal@nxp.com>; Matan Azrad <matan@mellanox.com>;
> Nicolau,
> > > Radu <radu.nicolau@intel.com>; Shahaf Shuler <shahafs@mellanox.com>;
> > > Narayana Prasad Raju Athreya <pathreya@marvell.com>; dev@dpdk.org
> > > Subject: RE: [dpdk-dev] [EXT] Re: [PATCH] ethdev: allow multiple
> > > security sessions to use one rte flow
> > >
> > > Hi
> > > sorry for jumping in late.
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Medvedkin, Vladimir
> > > > Sent: Wednesday, January 8, 2020 4:30 PM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Anoob
> > Joseph
> > > > <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>; Adrien
> > > > Mazarguil <adrien.mazarguil@6wind.com>; Doherty, Declan
> > > > <declan.doherty@intel.com>; Yigit, Ferruh
> > > > <ferruh.yigit@intel.com>;
> > Jerin
> > > > Jacob Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>
> > > > Cc: Ankur Dwivedi <adwivedi@marvell.com>; Hemant Agrawal
> > > > <hemant.agrawal@nxp.com>; Matan Azrad <matan@mellanox.com>;
> > > > Nicolau, Radu <radu.nicolau@intel.com>; Shahaf Shuler
> > > > <shahafs@mellanox.com>; Narayana Prasad Raju Athreya
> > > > <pathreya@marvell.com>; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] ethdev: allow multiple
> > > > security sessions to use one rte flow
> > > >
> > > > Hi Anoob,
> > > >
> > > > On 23/12/2019 13:34, Ananyev, Konstantin wrote:
> > > > >
> > > > >>>>>>>>>>>>>> The rte_security API which enables inline
> > > protocol/crypto
> > > > >>>>>>>>>>>>>> feature mandates that for every security session an
> > > > rte_flow
> > > > >>>>>>>>>>>>>> is
> > > > >>>>> created.
> > > > >>>>>>>>>>>>>> This would internally translate to a rule in the
> > hardware
> > > > >>>>>>>>>>>>>> which would do packet classification.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> In rte_securty, one SA would be one security session.
> > > And
> > > > if
> > > > >>>>>>>>>>>>>> an rte_flow need to be created for every session,
> > > > >>>>>>>>>>>>>> the
> > > > number
> > > > >>>>>>>>>>>>>> of SAs supported by an inline implementation would
> > be
> > > > >>>>>>>>>>>>>> limited by the number of rte_flows the PMD would be
> > > > able to
> > > > >>> support.
> > > > >>>>>>>>>>>>>> If the fields SPI & IP addresses are allowed to be
> > > > >>>>>>>>>>>>>> a
> > > range,
> > > > >>>>>>>>>>>>>> then this limitation can be overcome. Multiple
> > > > >>>>>>>>>>>>>> flows
> > will
> > > > be
> > > > >>>>>>>>>>>>>> able to use one rule for SECURITY processing. In
> > > > >>>>>>>>>>>>>> this
> > > case,
> > > > >>>>>>>>>>>>>> the security session provided as conf would be NULL.
> > >
> > > Why is that?
> > > If the rte flow can have a range then this means that we need one
> > > security_session for the entire range, Am I missing something? As it
> > > is stated in the rte_fow.h  security_session
> > can
> > > be used for multiple flows.
> >
> > [Anoob] One SA would mean one security_session. So if we have one
> > security_session for the entire range, then it will be like having
> > single SA for a range of IP & SPI. Do you think we should allow that?
> >
> [Ori] I'm less familiar with security, but this is what I understand you are trying to
> do right?

[Anoob] Not exactly. In our implementation, h/w can index into a table which would hold security_sessions. So we can have one rte_flow rule, which will enable the packet steering in the hardware. Which session need to be used will be determined by the SPI.
 
> 
> > Also, the intent of the patch is to minimize the number of rte_flow
> > rules required for inline ipsec processing. Since the security session
> > is per SA, and if we need multiple SPIs to use same rte_flow rule,
> > then the security_session field in the rte_flow rule need to be NULL.
> > Having a non-zero security_session when SPI is a range would be incorrect.
> >
> [Ori] I'm all in favor decreasing number of flows.
> Sorry for the basic question, what is the security_session /SA dependent on?

[Anoob] No prob! In case of unicast IPsec, every SA would have a unique SPI. So we cannot have multiple SPI's referring to the same SA. And one SA would mean one security_session.

> Can one SA include number of different SPI?

[Anoob] No.

May be we need to reimagine this.

Currently, an rte_flow with SECURITY enables ipsec processing with a specific security_session on the packet. This is enabled on a specific IP/SPI specified in the rule.

My proposal: an rte_flow with SECURITY (and session = NULL), would enable ipsec processing on a range and SPI from the packet can be used by the h/w to further figure out the security_session.
 
> 
> > >
> > > > >>>>>>>>>>>>> Wonder what will be the usage model for it?
> > > > >>>>>>>>>>>>> AFAIK,  RFC 4301 clearly states that either SPI
> > > > >>>>>>>>>>>>> value
> > alone
> > > > >>>>>>>>>>>>> or in conjunction with dst (and src) IP should
> > > > >>>>>>>>>>>>> clearly identify SA for inbound SAD
> > > > >>>>>>>>>>> lookup.
> > > > >>>>>>>>>>>>> Am I missing something obvious here?
> > > > >>>>>>>>>>>> [Anoob] Existing SECURITY action type requires
> > application
> > > to
> > > > >>>>>>>>>>>> create an 'rte_flow' per SA, which is not really
> > > > >>>>>>>>>>>> required if h/w can use SPI to uniquely
> > > > >>>>>>>>>>> identify the security session/SA.
> > > > >>>>>>>>>>>> Existing rte_flow usage: IP (dst,src) + ESP + SPI ->
> > > > >>>>>>>>>>>> security processing enabled on one security session
> > > > >>>>>>>>>>>> (ie on SA)
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> The above rule would uniquely identify packets for an
> > SA.
> > > > But
> > > > >>>>>>>>>>>> with the above usage, we would quickly exhaust
> > > > >>>>>>>>>>>> entries available in h/w lookup tables (which are
> > > > >>>>>>>>>>>> limited on our hardware). But if h/w can use SPI
> > > > >>>>>>>>>>>> field to index
> > > > >>>>>>>>>>> into a table (for example), then the above requirement
> > > > >>>>>>>>>>> of
> > > > one
> > > > >>>>>>>>>>> rte_flow per SA is not required.
> > > > >>>>>>>>>>>> Proposed rte_flow usage: IP (any) + ESP + SPI (any)
> > > > >>>>>>>>>>>> -> security processing enabled on all ESP packets
> > > > >>>>>>>>> So this means that SA will be indexed only by spi? What
> > about
> > > > >>>>>>>>> SA's which are indexed by SPI+DIP+SIP?
> > > > >>>>>>>>>>>> Now h/w could use SPI to index into a pre-populated
> > table
> > > > to
> > > > >>>>>>>>>>>> get security session. Please do note that, SPI is not
> > ignored
> > > > >>>>>>>>>>>> during the actual
> > > > >>>>>>>>>>> lookup. Just that it is not used while creating 'rte_flow'.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> And this table will be prepopulated by user and
> > > > >>>>>>>>>>> pointer to
> > it
> > > > >>>>>>>>>>> will be somehow passed via rte_flow API?
> > > > >>>>>>>>>>> If yes, then what would be the mechanism?
> > > > >>>>>>>>>> [Anoob] I'm not sure what exactly you meant by user.
> > > > >>>>>>>>>> But
> > > may
> > > > be
> > > > >>>>>>>>>> I'll explain
> > > > >>>>>>>>> how it's done in OCTEONTX2 PMD.
> > > > >>>>>>>>>> The application would create security_session for every SA.
> > > SPI
> > > > >>>>>>>>>> etc would be
> > > > >>>>>>>>> available to PMD (in conf) when the session is created.
> > > > >>>>>>>>> Now
> > > the
> > > > >>>>>>>>> PMD would populate SA related params in a specific
> > > > >>>>>>>>> location
> > > that
> > > > >>>>>>>>> h/w would access. This memory is allocated during device
> > > > >>>>>>>>> configure and h/w would have the pointer after the
> > > initialization
> > > > is
> > > > >>> done.
> > > > >>>>>>>>> If memory is allocated during device configure what is
> > > > >>>>>>>>> upper limit for number of sessions? What if app needs more?
> > > > >>>>>>>>>> PMD uses SPI as index to write into specific
> > > > >>>>>>>>>> locations(during session create)
> > > > >>>>>>>>> and h/w would use it when it sees an ESP packet eligible
> > > > >>>>>>>>> for SECURITY (in receive path, per packet). As long as
> > > > >>>>>>>>> session creation could populate at memory locations that
> > > > >>>>>>>>> h/w would
> > > > look
> > > > >>>>>>>>> at, this scheme would
> > > > >>>>>>> work.
> > > > >>>>>>>> [Anoob] Yes. But we need to allow application to control
> > > > >>>>>>>> the
> > > h/w
> > > > >>>>>>>> ipsec
> > > > >>>>>>> processing as well. Let's say, application wants to handle
> > > > >>>>>>> a specific SPI range in lookaside mode (may be because of
> > > > unsupported
> > > > >>>>>>> capabilities?), in that case having rte_flow will help in
> > > > >>>>>>> fine tuning how the
> > > > >>>>> h/w packet steering happens.
> > > > >>>>>>> Also, rte_flow enables H/w parsing on incoming packets.
> > > > >>>>>>> This
> > info
> > > > >>>>>>> is useful even after IPsec processing is complete. Or if
> > > > >>>>>>> application wants to give higher priority to a range of
> > > > >>>>>>> SPIs, rte_flow would allow doing
> > > > >>>>> so.
> > > > >>>>>>>>> What algorithm of indexing by SPI is there? Could I use
> > > > >>>>>>>>> any arbitrary SPI? If some kind of hashing is used, what
> > > > >>>>>>>>> about
> > > > collisions?
> > > > >>>>>>>> [Anoob] That is implementation dependent. In our PMD, we
> > > map
> > > > it
> > > > >>>>>>>> one
> > > > >>>>> to one.
> > > > >>>>>>> As in, SPI is used as index in the table.
> > > > >>>>>>> So, as far as you are mapping one to one and using SPI as
> > > > >>>>>>> an
> > > index,
> > > > >>>>>>> a lot of memory is wasted in the table for unused SPI's.
> > > > >>>>>>> Also,
> > you
> > > > >>>>>>> are not able to have a table with 2^32 sessions. It is
> > > > >>>>>>> likely that some number of SPI's least significant bits are used as
> an index.
> > > > >>>>>>> And it raises a question - what if application needs two
> > > > >>>>>>> sessions with different
> > > > >>>>> SPI's which have the same lsb's?
> > > > >>>>>> [Anoob] rte_security_session_create() would fail. Why do
> > > > >>>>>> you
> > say
> > > > we
> > > > >>>>> cannot support 2^32 sessions? If it's memory limitation, the
> > > > >>>>> same memory limitation would apply even if you have dynamic
> > allocation
> > > of
> > > > >>>>> memory for sessions. So at some point session creation would
> > start
> > > > >>>>> failing. In our PMD, we allow user to specify the range it
> > > > >>>>> requires
> > > using
> > > > >>> devargs.
> > > > >>>>>> Also, collision of LSBs can be avoided by introducing a "MARK"
> > rule
> > > > >>>>>> in
> > > > >>>>> addition to "SECURITY" for the rte_flow created for inline ipsec.
> > > > >>>>> Currently that model is not supported (in the library), but
> > > > >>>>> that is one solution to the collisions that can be pursued later.
> > > > >>>>>>> Moreover, what about
> > > > >>>>>>> two sessions with same SPI but different dst and src ip
> > > addresses?
> > > > >>>>>> [Anoob] Currently our PMD only support UCAST IPSEC. So
> > another
> > > > >>>>>> session
> > > > >>>>> with same SPI would result in session creation failure.
> > > > >>>>>
> > > > >>>>> Aha, I see, thanks for the explanation. So my suggestion
> > > > >>>>> here
> > would
> > > > be:
> > > > >>>>>
> > > > >>>>> - Application defines that some subset of SA's would be
> > > > >>>>> inline protocol processed. And this SA's will be indexed by SPI only.
> > > > >>>>>
> > > > >>>>> - App defines special range for SPI values of this SA's
> > > > >>>>> (size of this range is defined using devargs) and first SPI
> > > > >>>>> value (from
> > > > configuration?).
> > > > >>>>>
> > > > >>>>> - App installs rte_flow only for this range (from first SPI
> > > > >>>>> to first SPI
> > > > >>>>> + range size), not for all SPI values.
> > > > >>>> [Anoob] This is exactly what this patch proposes. Allowing
> > > > >>>> the SPI
> > and
> > > > the
> > > > >>> IP addresses to be range and have security_session provided as
> > NULL.
> > > > What
> > > > >>> you have described would be achievable only if we can allow
> > > > >>> this modification in the lib.
> > > > >>>> So can I assume you are in agreement with this patch?
> > > > >>> Not exactly. I meant it is better to make more specified flow like:
> > > > >>>
> > > > >>> ...
> > > > >>>
> > > > >>> struct rte_flow_item_esp esp_spec = {
> > > > >>>
> > > > >>>           .hdr = {
> > > > >>>                   .spi = rte_cpu_to_be_32(first_spi),
> > > > >>>           },
> > > > >>>
> > > > >>> };
> > > > >>>
> > > > >>> struct rte_flow_item_esp esp_mask = {
> > > > >>>
> > > > >>>           .hdr = {
> > > > >>>                   .spi = rte_cpu_to_be_32(nb_ipsec_in_sa - 1),
> > > > >>>           },
> > > > >>>
> > > > >>> };
> > > > >>>
> > > > >>> pattern[0].type = RTE_FLOW_ITEM_TYPE_ESP;
> > > > >>>
> > > > >>> pattern[0].spec = & esp_spec;
> > > > >>>
> > > > >>> pattern[0].mask = &esp_mask;
> > > > >>>
> > > > >>> ...
> > > > >>>
> > > > >>> So this means inline proto device would process only special
> > > > >>> subset
> > of
> > > > SPI's.
> > > > >>> All other will be processed as usual. Sure, you can assign all
> > > > >>> 2^32 SPI range and it work as you intended earlier. I think we
> > > > >>> need to
> > > > have
> > > > >>> finer grained control here.
> > > > >>>
> > > > >> [Anoob] Allowing a range for SPI is what you have also described.
> > What
> > > > you described is one way to define a range. That will come as
> > > > >> part of the implementation, ie, a change in the example application.
> > This
> > > > patch intends to allow using a range for SPI than a fixed
> > > > >> value. I believe you are also in agreement there.
> > > > > I also don't have objections for that patch.
> > > > > The only obseravion from reading your replies to that at
> > > > > ipsec-secgw
> > > > patches:
> > > > > Extra API to retrieve size of that HW table seems to be needed.
> > > > > Though I suppose it could be a subject of separate patch/discussion.
> > > > >
> > > > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > >
> > > > I also don't have objections.
> > > >
> > > > Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> > > >
> > > > >
> > > > >>>>> - Other SPI values would be processed non inline.
> > > > >>>>>
> > > > >>>>> In this case we would be able to have SA addressed by longer
> > tuple
> > > > (i.e.
> > > > >>>>> SPI+DIP+SIP) outside of before mentioned range, as well as
> > > > >>>>> SPI+DIP+SA
> > with
> > > > >>>>> unsupported capabilities by inline protocol device.
> > > > >>>>>
> > > > >>>>>>>>>>>> The usage of one 'rte_flow' for multiple SAs is not
> > > > mandatory.
> > > > >>>>>>>>>>>> It is only required when application requires large
> > number
> > > of
> > > > >>> SAs.
> > > > >>>>>>>>>>>> The proposed
> > > > >>>>>>>>>>> change is to allow more efficient usage of h/w
> > > > >>>>>>>>>>> resources
> > > > where
> > > > >>>>>>>>>>> it's permitted by the PMD.
> > > > >>>>>>>>>>>>>> Application should do an rte_flow_validate() to
> > > > >>>>>>>>>>>>>> make
> > > sure
> > > > >>>>>>>>>>>>>> the flow is supported on the PMD.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > > >>>>>>>>>>>>>> ---
> > > > >>>>>>>>>>>>>>       lib/librte_ethdev/rte_flow.h | 6 ++++++
> > > > >>>>>>>>>>>>>>       1 file changed, 6 insertions(+)
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> diff --git a/lib/librte_ethdev/rte_flow.h
> > > > >>>>>>>>>>>>>> b/lib/librte_ethdev/rte_flow.h index
> > 452d359..21fa7ed
> > > > >>> 100644
> > > > >>>>>>>>>>>>>> --- a/lib/librte_ethdev/rte_flow.h
> > > > >>>>>>>>>>>>>> +++ b/lib/librte_ethdev/rte_flow.h
> > > > >>>>>>>>>>>>>> @@ -2239,6 +2239,12 @@ struct
> > rte_flow_action_meter
> > > {
> > > > >>>>>>>>>>>>>>        * direction.
> > > > >>>>>>>>>>>>>>        *
> > > > >>>>>>>>>>>>>>        * Multiple flows can be configured to use
> > > > >>>>>>>>>>>>>> the same security
> > > > >>>>> session.
> > > > >>>>>>>>>>>>>> + *
> > > > >>>>>>>>>>>>>> + * The NULL value is allowed for security session.
> > > > >>>>>>>>>>>>>> + If security session is NULL,
> > > > >>>>>>>>>>>>>> + * then SPI field in ESP flow item and IP
> > > > >>>>>>>>>>>>>> + addresses in
> > > flow
> > > > >>>>>>>>>>>>>> + items 'IPv4' and
> > > > >>>>>>>>>>>>>> + * 'IPv6' will be allowed to be a range. The rule
> > > > >>>>>>>>>>>>>> + thus created can enable
> > > > >>>>>>>>>>>>>> + * SECURITY processing on multiple flows.
> > > > >>>>>>>>>>>>>> + *
> > > > >>>>>>>>>>>>>>        */
> > > > >>>>>>>>>>>>>>       struct rte_flow_action_security {
> > > > >>>>>>>>>>>>>>       void *security_session; /**< Pointer to
> > > > >>>>>>>>>>>>>> security session
> > > > >>>>>>> structure.
> > > > >>>>>>>>>>>>>> */
> > > > >>>>>>>>>>>>>> --
> > > > >>>>>>>>>>>>>> 2.7.4
> > > > >>>>>>>>> --
> > > > >>>>>>>>> Regards,
> > > > >>>>>>>>> Vladimir
> > > > >>>>>>> --
> > > > >>>>>>> Regards,
> > > > >>>>>>> Vladimir
> > > > >>>>> --
> > > > >>>>> Regards,
> > > > >>>>> Vladimir
> > > > >>> --
> > > > >>> Regards,
> > > > >>> Vladimir
> > > >
> > > > --
> > > > Regards,
> > > > Vladimir