Re: [dpdk-dev] [RFC] ipsec: new library for IPsec data-path processing

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Mon, 17 Sep 2018 18:12:48 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Joseph, Anoob"
>  <Anoob.Joseph@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Awal, Mohammad Abdul"
>  <mohammad.abdul.awal@intel.com>, "Doherty, Declan"
>  <declan.doherty@intel.com>, Narayana Prasad
>  <narayanaprasad.athreya@caviumnetworks.com>
> Subject: RE: [dpdk-dev] [RFC] ipsec: new library for IPsec data-path
>  processing
> 
> 
> Hi Jerin,


Hi Konstantin,

> 
> 
> > >
> > > >
> > > > Anyway, let's pretend we found some smart way to distribute inbound packets for the same SA to multiple HW queues/CPU
> > cores.
> > > > To make ipsec processing for such case to work correctly just atomicity on check/update segn/replay_window is not enough.
> > > > I think it would require some extra synchronization:
> > > > make sure that we do final packet processing (seq check/update) at the same order as we received the packets
> > > > (packets entered ipsec processing).
> > > > I don't really like to introduce such heavy mechanisms on SA level,  after all it supposed to be light and simple.
> > > > Though we plan CTX level API to support such scenario.
> > > > What I think would be useful addition for SA level API - have an ability to do one update seqn/replay_window and multiple checks
> > concurrently.
> > > >
> > > > > In case of ingress also, the same problem exists. We will not be able to use RSS and spread the traffic to multiple cores.
> > Considering
> > > > > IPsec being CPU intensive, this would limit the net output of the chip.
> > > > That's true - but from other side implementation can offload heavy part
> > > > (encrypt/decrypt, auth) to special HW (cryptodev).
> > > > In that case single core might be enough for SA and extra synchronization would just slowdown things.
> > > > That's why I think it should be configurable  what behavior (ST or MT) to use.
> > > I do agree that these are the issues that we need to address to make the
> > > library MT safe. Whether the extra synchronization would slow down things is
> > > a very subjective question and will heavily depend on the platform. The
> > > library should have enough provisions to be able to support MT without
> > > causing overheads to ST. Right now, the library assumes ST.
> >
> >
> > I agree with Anoob here.
> >
> > I have two concerns with librte_ipsec as a separate library
> >
> > 1) There is an overlap with rte_security and new proposed library.
> 
> I don't think there really is an overlap.

As mentioned in your other email. IMO, There is an overlap as
RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL can support almost everything
in HW or HW + SW if some PMD wishes to do so. 

Answering some of the questions, you have asked in other thread based on
my understanding.

Regarding RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL support,
Marvell/Cavium CPT hardware on next generation HW(Planning to upstream
around v19.02) can support RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL and
Anoob already pushed the application changes in ipsec-gw.

In our understanding of HW/SW roles/responsibilities for that type of
devices are:

INLINE_PROTOCOL
----------------
In control path, security session is created with the given SA and
rte_flow configuration etc.
 
For outbound traffic, the application will have to do SA lookup and
identify the security action (inline/look aside crypto/protocol). For
packets identified for inline protocol processing, the application would
submit as plain packets to the ethernet device and the security capable
ethernet device would perform IPSec and send out the packet. For PMDs
which would need extra metadata (capability flag), set_pkt_metadata
function pointer would be called (from application). This can be used to
set some per packet field to identify the security session to be used to
process the packet. Sequence number updation will be done by the PMD.
For inbound traffic, the packets for IPSec would be identified by using
rte_flow (hardware accelerated packet filtering). For the packets
identified for inline offload (SECURITY action), hardware would perform
the processing. For inline protocol processed IPSec packets, PMD would
set “user data” so that application can get the details of the security
processing done on the packet. Once the plain packet (after IPSec
processing) is received, a selector check need to be performed to make
sure we have a valid packet after IPSec processing. The user data is used
for that. Anti-replay check is handled by the PMD. The PMD would raise
an eth event in case of sequence number expiry or any SA expiry.


LOOKASIDE_PROTOCOL
------------------
In control path, security session is created with the given SA.
 
Enqueue/dequeue is similar to what is done for regular crypto
(RTE_SECURITY_ACTION_TYPE_NONE) but all the protocol related processing
would be offloaded. Application will need to do SA lookup and identify
the processing to be done (both in case of outbound & inbound), and
submit packet to crypto device. Application need not do any IPSec
related transformations other than the lookup. Anti-replay need to be
handled in the PMD (the spec says the device “may be handled” do anti-replay check,
but a complete protocol offload would need anti-replay check also).


> rte_security is a 'framework for management and provisioning of security protocol operations offloaded to hardware based devices'.
> While rte_ipsec is aimed to be a library for IPsec data-path processing.
> There is no plans for rte_ipsec to 'obsolete' rte_security.
> Quite opposite rte_ipsec supposed to work with both rte_cryptodev and rte_security APIs (devices).
> It is possible to have an SA that would use both crypto and  security devices.
> Or to have an SA that would use multiple crypto devs
> (though right now it is up the user level to do load-balancing logic).
> 
> > For IPsec, If an application needs to use rte_security for HW
> > implementation and and application needs to use librte_ipsec for
> >  SW implementation then it is bad and a lot duplication of work on
> > he slow path too.
> 
> The plan is that application would need to use just rte_ipsec API for all data-paths
> (HW/SW, lookaside/inline).
> Let say right now there is rte_ipsec_inline_process() function if user
> prefers to use inline security device to process given group packets,
> and rte_ipsec_crypto_process(/prepare) if user decides to use
> lookaside security or simple crypto device for it.
> 
> >
> > The rte_security spec can support both inline and look-aside IPSec
> > protocol support.
> 
> AFAIK right now rte_security just provides API to create/free/manipulate security sessions.
> I don't see how it can support all the functionality mentioned above,
> plus SAD and SPD.


At least for INLINE_PROTOCOL case SA lookup for inbound traffic does by
HW.

> 
> >
> > 2) This library is tuned for fat CPU core in mind like single SA on core
> > etc. Which is fine for x86 servers and arm64 server category of machines
> > but it does not work very well with NPU class of SoC or FPGA.
> >
> > As there  are the different ways to implement the IPSec, For instance,
> > use of eventdev can help in situation for handling millions of SA and
> > equence number of update and anti reply check can be done by leveraging
> > some of the HW specific features like
> > ORDERED, ATOMIC schedule type(mapped as eventdev feature)in HW with PIPELINE model.
> >
> > # Issues with having one SA one core,
> > - In the outbound side, there could be multiple flows using the same SA.
> >   Multiple flows could be processed parallel on different lcores,
> > but tying one SA to one core would mean we won't be able to do that.
> >
> > - In the inbound side, we will have a fat flow hitting one core. If
> >   IPsec library assumes single core, we will not be able to to spread
> > fat flow to multiple cores. And one SA-one core would mean all ports on
> > which we would expect IPsec traffic has to be handled by that core.
> 
> I suppose that all refers to the discussion about MT safe API for rte_ipsec, right?
> If so, then as I said in my reply to Anoob:
> We will try to make API usable in MT environment for v1,
> so you can review and provide comments at early stages.

OK

> 
> >
> > I have made a simple presentation. This presentation details ONE WAY to
> > implement the IPSec with HW support on NPU.
> >
> > https://docs.google.com/presentation/d/1e3IDf9R7ZQB8FN16Nvu7KINuLSWMdyKEw8_0H05rjj4/edit?usp=sharing
> >
> 
> Thanks, quite helpful.
> Actually from page 3, it looks like your expectations don't contradict in general with proposed API:
> 
> ...
> } else if (ev.event_type == RTE_EVENT_TYPE_LCORE && ev.sub_event_id == APP_STATE_SEQ_UPDATE) {
>                         sa = ev.flow_queue_id;
>                         /* do critical section work per sa */
>                         do_critical_section_work(sa);
> 
> [KA] that's the place where I expect either
> rte_ipsec_inline_process(sa, ...); OR rte_ipsec_crypto_prepare(sa, ...);
> would be called.

Makes sense. But currently, the library defines what is
rte_ipsec_inline_process() and rte_ipsec_crypto_prepare(), but it should
be based on underneath security device or crypto device.

So, IMO for better control, these functions should be the function pointer
based and based on underlying device, library can fill the
implementation.

IMO, it is not possible to create "static inline function" with all "if"
checks. I think, we can have four ipsec functions with function pointer
scheme.

rte_ipsec_inbound_prepare()
rte_ipsec_inbound_process()
rte_ipsec_outbound_prepare()
rte_ipsec_outbound_process()

Some of the other concerns:
1) For HW implementation, rte_ipsec_sa needs to opaque like rte_security
as some of the structure defined by HW or Microcode. We can choose
absolute generic items as common and device/rte_security specific can be opaque.

2)I think, in order to accommodate the event drivern model. We need to pass
void ** in prepare() and process() function with an additional argument
of type(TYPE_EVENT/TYPE_MBUF) can be passed to detect packet object
type as some of the functions in prepare() and process() may need
rte_event to operate on.

> 
>                      /* Issue the crypto request and generate the following on crypto work completion */
> [KA] that's the place where I expect rte_ipsec_crypto_process(...) be invoked.
> 
>                         ev.flow_queue_id = tx_port;
>                         ev.sub_event_id = tx_queue_id;
>                         ev.sched_sync = RTE_SCHED_SYNC_ATOMIC;
>                         rte_cryptodev_event_enqueue(cryptodev, ev.mbuf, eventdev, ev);
>                 }
> 
> 
> > I am not saying this should be the ONLY way to do as it does not work
> > very well with non NPU/FPGA class of SoC.
> >
> > So how about making the proposed IPSec library as plugin/driver to
> > rte_security.
> 
> As I mentioned above, I don't think that pushing whole IPSec data-path into rte_security
> is the best possible approach.
> Though I probably understand your concern:
> In RFC code we always do whole prepare/process in SW (attach/remove ESP headers/trailers, so paddings etc.),
> i.e. right now only device types: RTE_SECURITY_ACTION_TYPE_NONE and RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO are covered.
> Though there are devices where most of prepare/process can be done in HW
> (RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL/RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL),
> plus in future could be devices where prepare/process would be split between HW/SW in a custom way.
> Is that so?
> To address that issue I suppose we can do:
> 1. Add support for RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL and RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL
>     security devices into ipsec.
>     We planned to do it anyway, just don't have it done yet.
> 2. For custom case - introduce RTE_SECURITY_ACTION_TYPE_INLINE_CUSTOM and RTE_SECURITY_ACTION_TYPE_LOOKASIDE_CUSTOM

The problem is, CUSTOM may have different variants and "if" conditions won't
scale if we choose to have non function pointer scheme. Otherwise, it
looks OK to create new SECURITY TYPE and associated plugin for prepare() and process()
function in librte_ipsec library.


>     and add into rte_security_ops   new functions:
>     uint16_t lookaside_prepare(struct rte_security_session *sess, struct rte_mbuf *mb[], struct struct rte_crypto_op *cop[], uint16_t num);
>     uint16_t lookaside_process(struct rte_security_session *sess, struct rte_mbuf *mb[], struct struct rte_crypto_op *cop[], uint16_t num);
>     uint16_t inline_process(struct rte_security_session *sess, struct rte_mbuf *mb[], struct struct rte_crypto_op *cop[], uint16_t num);
>     So for custom HW, PMD can overwrite normal prepare/process behavior.
> 
> > This would give flexibly for each vendor/platform choose to different
> > IPse implementation based on HW support WITHOUT CHANGING THE APPLICATION
> > INTERFACE.
> 
> Not sure what API changes you are referring to?
> As I am aware we do introduce new API, but all existing APIs remain in place.


What I meant was, Single application programming interface to enable IPSec processing to
application.


> 
> >
> > IMO, rte_security IPsec look aside support can be simply added by
> > creating the virtual crypto device(i.e move the proposed code to the virtual crypto device)
> > likewise inline support
> > can be added by the virtual ethdev device.
> 
> That's probably possible and if someone would like to introduce such abstraction - NP in general
> (though my suspicion - it might be too heavy to be really useful).
> Though I don't think it should be the only possible way for the user to enable IPsec data-processing inside his app.
> Again I guess such virtual-dev will still use rte_ipsec inside.

I don't have strong opinion on virtual devices VS function pointer based
prepare() and process() function in librte_ipsec library.

> 
> > This would avoid the need for
> > updating ipsec-gw application as well i.e unified interface to application.
> 
> I think - it would  really good to simplify existing ipsec-secgw sample app.
> Some parts of it seems unnecessary complex to me.
> One of the reasons for it -  we don't really have an unified (and transparent) API for ipsec data-path.
> Let's look at ipsec_enqueue() and related code (examples/ipsec-secgw/ipsec.c:365)
> It is huge (and ugly) -  user has to handle dozen different cases just to enqueue packet for IPsec processing.
> One of the aims of rte_ipsec library - hide all that complexities inside the library and provide to
> the upper layer clean and transparent API.
> 
> >
> > If you don't like the above idea, any scheme of plugin based
> > implementation would be fine so that vendor or platform can choose its own implementation.
> > It can be based on partial HW implement too. i.e SA look can be used in SW, remaining stuff in HW
> > (for example IPsec inline case)
> 
> I am surely ok with the idea to give vendors an ability to customize implementation
> and enable their HW capabilities.

I think, We are on the same page, just that the fine details of "framework"
for customizing implementation based on their HW capabilities need to
iron out.

> Do you think proposed additions to the rte_security would be  enough,
> or something extra is needed?

See above.

Jerin

> 
> Konstantin
> 
> 
> >
> > # For protocols like UDP, it makes sense to create librte_udp as there
> > no much HW specific offload other than ethdev provides.
> >
> > # PDCP could be another library to offload to HW, So talking
> > rte_security path makes more sense in that case too.
> >
> > Jerin