Hi Ori,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Ori Kam <or...@mellanox.com>
> Sent: Thursday, January 9, 2020 1:06 PM
> To: Medvedkin, Vladimir <vladimir.medved...@intel.com>; Ananyev,
> Konstantin <konstantin.anan...@intel.com>; Anoob Joseph
> <ano...@marvell.com>; Akhil Goyal <akhil.go...@nxp.com>; Adrien
> Mazarguil <adrien.mazarg...@6wind.com>; Doherty, Declan
> <declan.dohe...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>; Jerin
> Jacob Kollanukkaran <jer...@marvell.com>; Thomas Monjalon
> <tho...@monjalon.net>
> Cc: Ankur Dwivedi <adwiv...@marvell.com>; Hemant Agrawal
> <hemant.agra...@nxp.com>; Matan Azrad <ma...@mellanox.com>;
> Nicolau, Radu <radu.nico...@intel.com>; Shahaf Shuler
> <shah...@mellanox.com>; Narayana Prasad Raju Athreya
> <pathr...@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-boun...@dpdk.org> On Behalf Of Medvedkin, Vladimir
> > Sent: Wednesday, January 8, 2020 4:30 PM
> > To: Ananyev, Konstantin <konstantin.anan...@intel.com>; Anoob Joseph
> > <ano...@marvell.com>; Akhil Goyal <akhil.go...@nxp.com>; Adrien
> > Mazarguil <adrien.mazarg...@6wind.com>; Doherty, Declan
> > <declan.dohe...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>; Jerin
> > Jacob Kollanukkaran <jer...@marvell.com>; Thomas Monjalon
> > <tho...@monjalon.net>
> > Cc: Ankur Dwivedi <adwiv...@marvell.com>; Hemant Agrawal
> > <hemant.agra...@nxp.com>; Matan Azrad <ma...@mellanox.com>;
> > Nicolau, Radu <radu.nico...@intel.com>; Shahaf Shuler
> > <shah...@mellanox.com>; Narayana Prasad Raju Athreya
> > <pathr...@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?
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.
>
> > >>>>>>>>>>>>> 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.anan...@intel.com>
> >
> > I also don't have objections.
> >
> > Acked-by: Vladimir Medvedkin <vladimir.medved...@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 <ano...@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
