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

[Ori Kam <orika@mellanox.com>]

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.

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