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.
