Re: [dpdk-dev] [PATCH v3 0/3] add fallback session

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Anoob,

> > > I've few more observations regarding the proposed feature.
> > >
> > > 1. From what I understood, if an ESP packet ends up on an unprotected
> > > interface and doesn't have 'PKT_RX_SEC_OFFLOAD' bit set, then the packet
> > would be looked up to see the associated SA and then fallback session is figured
> > out and then further processing is done.
> > >
> > > Can you confirm if I understood the sequence correctly? If yes, then
> > > aren't we doing an extra lookup in the s/w? The packet may be looked
> > > by the h/w using rte_flow and that information could be used to determine the
> > SA. Also, if the ESP packet is expected to be forwarded, then the above logic will
> > add an unnecessary lookup even after your h/w has detected that the packet
> > need not be security processed.
> >
> > Not sure I understood your whole statement above.
> > AFAIK, right now (with dpdk master) for unprotected iface it works like that:
> >
> > 1.  slit incoming traffic into 3 groups: ESP packets, IPv4 packets, IPv6 packets.
> > For ESP packets:
> > 2. perform SAD lookup
> >     a. if it fails (non SA found for that SPI), then drop the packet.
> >     b. otherwise (SA found) process the packet using found SA
> >
> > What fall-back patch adds:
> > Before step 2.b check:
> > does that SA has its primary session  with type INLINE-CRYPTO and
> > does HW fail to do IPsec realted processing for it (by some reason)?
> > If yes, then mark this packet to be processed by fall-back session.
> > if (MBUF_NO_SEC_OFFLOAD(pkt) && sa->fallback_sessions > 0) {
> >                 uintptr_t intsa = (uintptr_t)sa;
> >                 intsa |= IPSEC_SA_OFFLOAD_FALLBACK_FLAG;
> >                 result_sa = (void *)intsa;  }
> >
> > So from my perspective, no additional lookup where introduced.
> 
> [Anoob] For inline processing, h/w does a lookup and figures out the security processing to be done on the packet.
> "rte_security_get_userdata()" allows s/w to further segregate that info. In this approach, I believe we don't consider how such info from
> h/w can be used.

Right now it is not the case with ipsec-secgw:
for each inbound ESP packet it *always* does a lookup in SW based SADB,
and if lookup fails - drops the packet (see 2.a above).
So, I still not see any additional lookups introduced with the patch.

> 
> > Also AFAIK, right now there is no possibility to configure ipsec-secgw to BYPASS
> > some ESP traffic.
> > Should we do it (to conform to ipsec RFC) - that's probably subject of another
> > discussion.
> 
> [Anoob] The approach (choice of flags) forces a software-based SA lookup for packets that need to be bypassed instead of leveraging
> possible hardware SA lookup. I don't think what ipsec-secgw does matters here.

I do not agree with that statement.
First of all - current patch doesn't introduce any additional SW lookups, see above.
Second, if someone would like to add BYPASS for ESP packets into ipsec-secgw, I think he would have to
insert new code that do de-mux *before* any ESP related processing starts.
Most obvious variant: at prepare_one_packet() when we split our incoming traffic to IPsec and non-IPsec ones.
Second variant - at early stage of single_inbound_lookup(), straight after actual SAD lookup failure.
In both cases, I don't see how session selection (primary or faillback) would affect us here,
actual decision do we want to PROCESS or BYPASS particular ESP packet needs to take place before that.
So I still believe we are ok here.

> 
> For example, ESN was not supported by ipsec-secgw (before library mode was introduced), but every single library change and application
> change was added keeping in mind that ESN is a valid feature for ipsec. So it is one thing to say ipsec-secgw doesn't support ESP bypass and
> saying the solution doesn't consider a possibility of ESP bypass.
> 
> >
> > >
> > > 2. The solution proposed here seems like adding the handling in
> > > ipsec-secgw instead of ipsec library. In other words, this feature is not getting
> > added in ipsec library, which was supposed to simplify the whole ipsec usage in
> > DPDK, but fails to handle the case of fragmentation.
> >
> > What we have right now with ipsec library is SA (low) level API.
> > It can handle multi-segment packets properly, but expects someone else to do
> > other steps (fragmentation/reassembly).
> > ipsec-secgw demonstrates how librte_ip_frag and librte_ipsec can be used
> > together to deal with fragmented IPsec traffic in a proper manner.
> > Probably in future we'll consider adding some high-level API that will be able to
> > do whole processing under hood (SPD/SAD lookup, fragment/reassembly, actual
> > IPsec packet processing, matching inbound selectors, etc.), but right now it is
> > not the case.
> >
> > > Also, since the fallback feature is entirely done in the application, it begs the
> > question why the same feature is omitted for legacy use case.
> >
> > Because legacy mode doesn't support multi-seg packets at first place.
> > Also it is an extra overhead to support 2 code-paths (legacy and library) for the
> > same app, so we'd like in future to deprecate and later remove legacy code-
> > path.
> > As a first step we propose to make library code-path a default one:
> > http://patches.dpdk.org/cover/58247/
> 
> [Anoob] Even if we need the application to do the reassembly, it would look simple if application uses the same "rte_ipsec_session" to
> process the fallback packet.

I think there is some sort of misunderstanding here.
With current librte_ipsec design:
rte_ipsec_sa - opaque SW representation of the SA (HW neutral)
 rte_ipsec_session associates SA (rte_ipsec_sa)with particular HW device (session).
Same SA can be referred by multiple sessions.

From rte_ipsec.h:
/**
 * rte_ipsec_session is an aggregate structure that defines particular
 * IPsec Security Association IPsec (SA) on given security/crypto device:
 * - pointer to the SA object
 * - security session action type
 * - pointer to security/crypto session, plus other related data
 * - session/device specific functions to prepare/process IPsec packets.
 */

> In that case, the processing required to handle the other packet will be hidden from the application. Here
> application has to make sure it chooses the correct session, which I believe should be moved to ipsec library.

From my side it is a good thing to let application define its own usage model.
I.E. librte_ipsec supports multiple-sessions per SA, upper-layer (app) decides how it
will use that feature.
Though if you strongly feel that some higher-level API is needed here,
and have some good ideas about it  - please go ahead and submit RFC for it. 

> 
> >
> > >
> > > 3. It seems like ordering won't be maintained once this processing is
> > > done. Again, this is the sequence I understood. Please correct me if I missed
> > something,
> > >        a. Application receives a bunch of packets (let's say 6
> > > packets), in which few are fragmented (P3 & P4) and the rest can be inline
> > processed.
> > >        b. Application receives P1->P2->P3->P4->P5->P6 (in this, P1, P2, P5, P6 are
> > inline processed successfully) and P4 & P5 are the fragments
> > >        c. Application groups packets. P1->P2->P5->P6 becomes one group and P3-
> > >P4 becomes another and goes for fallback processing.
> > > Now how is ordering maintained? I couldn't figure out how that is done in this
> > case.
> >
> > You right, fallback session can introduce packet reordering.
> > At least till we'll have ability to process packets in sync mode too.
> > See our presentation at dpdk userspace (slides 17, 18):
> > https://static.sched.com/hosted_files/dpdkbordeaux2019/8f/DPDK-IPSECu9.pdf
> > Right now the only way to deal with it - have replay window big enough to
> > sustain reordering and async processing latency.
> > That's actually another reason why we add this feature into ipsec-secgw sample
> > app:
> > so people can evaluate it on their platforms, determine what replay window size
> > would be needed, what issues/slowdowns it might cause, etc.
> 
> [Anoob] This is something I had noticed while going through the code flow. The ordering info is lost because of the approach. 

Once again, it is a known limitation and we are not trying to hide it from you :)
It was outlined here:
https://static.sched.com/hosted_files/dpdkbordeaux2019/8f/DPDK-IPSECu9.pdf
And in the latest patch version Marcin clearly stated it inside the AG session:
http://patches.dpdk.org/patch/60039/
If you think even further clarification within the doc is needed, please let us know.

About the actual packet re-oridering:
 AFAIK, for some use-cases it might be acceptable, for others not.
Though it is an optional feature, that wouldn't be enabled by default, so user can always
choose is it does he needs/wants this feature or not.
If/when we'll have CPU_CRYPTO processing mode:
http://patches.dpdk.org/cover/58862/
we'll add ability for the user to avoid this reordering problem 

> As we dig deeper, the feature looks hardly complete. The concerning part is the approach doesn't seem conducive to fixing any of these issues in the
> future.

Again, don't agree with that statement.
From my perspective: while current implementation has its limitations (packet reordering, inline-proto/lksd-proto),
it is totally functional and provides users with a reference how to deal with such kind of problems. 

> 
> Also, if the new ipsec related features are introduced via ipsec-secgw than via rte_ipsec, then it raises doubts over the utility of rte_ipsec
> library.