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

Ananyev, Konstantin konstantin.ananyev at intel.com
Thu Oct 3 16:46:03 CEST 2019


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:
> 
> [Anoob] Are you saying there is no h/w lookup involved when doing inline crypto processing? Then I'm confused. My understanding is that
> rte_flow with ACTION type as SECURITY is being used to setup h/w lookups.

I am saying that current ipsec-secgw code for each inbound ESP packet *always* does SW lookup.
No matter did HW lookup take place already and does information about that lookup is available
via mbuf in some way or not.
Just look at the ipsec-secgw code yourself:

nb_rx = rte_eth_rx_burst(portid, queueid,  pkts, MAX_PKT_BURST);

if (nb_rx > 0)
	process_pkts(qconf, pkts, nb_rx, portid);

Inside process_pkts()
http://lxr.dpdk.org/dpdk/latest/ident/prepare_one_packet
 it first calls prepare_traffic() which does sort of de-muxing:
put packet in one of 3 arrays: 
   1) ESP packets
   2) non ESP IPv4 packets
   3) non ESP IPv6 packets
Also it checks is PKT_RX_SEC_OFFLOAD set for that packet.
If yes, it retrieves associated security user-data and stores it inside private mbuf area.

Then it goes into process_pkts_inbound()
http://lxr.dpdk.org/dpdk/latest/ident/process_pkts_inbound
and here for all ESP packets:
legacy code path:
ipsec_inbound()->inbound_sa_lookup()->single_inbound_lookup()
for librte_ipsec code path:
inbound_sa_lookup()->single_inbound_lookup()

single_inbound_lookup() is the one that does actual SW lookup for each input packet.
http://lxr.dpdk.org/dpdk/latest/ident/single_inbound_lookup

> 
> > 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.
> 
> [Anoob] In case of inline crypto, you could do that. No disagreement. But that doesn't mean that is the only way. If PMDs can retrieve info
> about h/w lookups and pass it on to the upper layers, isn't that the better approach?

Please let's keep our conversation in a constructive way.
We are not discussing what could be done in theory, but a particular patch for ipsec-secgw:
Right now ipsec-secgw does a SW lookup for each inbound ESP packet
(no matter what HW offload does) and this patch doesn't introduce
any extra SW lookups.

If you are not happy with current ipsec-secgw approach, that's fine -
feel free to submit RFC/patch to fix that, or start a discussion in a new thread.
I just don't see why we have to discuss it in context of this 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.
> 
> [Anoob] I don't think you understood what I'm trying to highlight here. You could have a situation when h/w can detect that the packet has
> to be "PROTECT"ed using an "inline crypto" session. But then it might detect that it cannot proceed with inline processing because of some
> issue (fragmentation, h/w queue overflow etc). Now the h/w has already figured out the action to be done to the packet. If PMDs allow this
> to be communicated to the application, the application won't be required to do the lookup.
> 
> In inline crypto, application can ignore the h/w lookup data and do a s/w lookup with the protocol headers as they are not stripped off. It
> was done this way, because API and the associated means to get this info from PMD was not introduced when inline crypto and
> corresponding support in Intel's PMD was added. But inline protocol cannot do the lookup in the application and so the concept of userdata
> etc was introduced. The same can be applied to inline crypto also. Advantage? It could remove the extra lookup done in s/w.
> 
> To summarize, we cannot assume that every inline crypto packet need to looked up in the application to figure out the processing done on
> the packet. There can be applications which relies on h/w lookup data to figure out the processing done.
> 
> Example: Using rte_flow, I can create a rule for ESP packets to be MARKed. There is no security session created for the flow and the
> application intends to forward this flow packets to a different port. With your approach, even these packets would be looked up. The
> packet will have info from the h/w lookup which doesn't get used, because the approach fails to introduce the required concepts.

I think I understood your point.
Basically you saying that in future your PMD for unprocessed ESP packets might provide some additional information
(via rte_security_get_userdata) that might be used by SW - let say to choose BYPASS for such packets.
Current ipsec-secgw doesn't support such ability.
Your concern is that this patch will somehow prevent (or will make it harder) for you to make your future changes.
Correct?
If, so then I said before, I don't think that concern is valid.
Most obvious variant where to add such code is inisde
inside prepare_one_packet() when we split our incoming traffic to IPsec and non-IPsec ones.
To be more specific something like that

if (eth->ether_type == rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV4)) {

                iph4 = (const struct rte_ipv4_hdr *)rte_pktmbuf_adj(pkt,
                        RTE_ETHER_HDR_LEN);
                adjust_ipv4_pktlen(pkt, iph4, 0);

-                if (iph4->next_proto_id == IPPROTO_ESP)
+                if (iph4->next_proto_id == IPPROTO_ESP) {
+		if (pkt->ol_flags & PKT_RX_SEC_OFFLOAD) {
+			/* extract extra info, make decision based on that */
+		} else
			t->ipsec.pkts[(t->ipsec.num)++] = pkt;		
+	   } 

Of course some code reordering might be needed to avoid performance drop,
but I suppose the main idea is clear enough. 
 Second variant - at early stage of single_inbound_lookup(),
either before or straight after actual SAD SW lookup failure.
In both cases, this new code will be executed *before* session selection code.

> 
> Does the above sound like BYPASS? Yes.
> Does ipsec-secgw do it this way? No.

Correct, it doesn't.
You have to submit patches if you'd like to support such ability. 

> Does this approach prevent an application from introducing such a BYPASS? Yes.

That's not correct, I believe, see above.

> 
> >
> > >
> > > 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:
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.org
> > > >
> > _cover_58247_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyL
> > Ws2
> > > > n6B-WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-K5XfH8wG-
> > 7hwr1d8oM4uJGErAkbf
> > > > DvA&s=vz7ioUzJOuzoJdmV7QO0QLPKYn1ytFsj_0eYatbSCrg&e=
> > >
> > > [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.
> 
> [Anoob] If you are okay with application defining how it implements ipsec, then probably there is no use case for lib ipsec?

:)
Once again - library provides an implementation.
Application defines the way to use it, i.e. how it will apply functionality library provides for different use-case scenarios.
As an example: snpritnf() provides user with ability to format strings,
application decides what exactly format to use and for which string.
Same for librte_ipsec, library provides functionality to perform processing for ESP packets.
Application defines which packets to process and what session to use. 

> 
> >
> > >
> > > >
> > > > >
> > > > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__static.sched.co
> > > > m_hosted-5Ffiles_dpdkbordeaux2019_8f_DPDK-
> > 2DIPSECu9.pdf&d=DwIFAg&c=n
> > > > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > WYLn1v9SyTMrT5EQqh2TU&
> > > > m=b3E429fuo8P-K5XfH8wG-
> > 7hwr1d8oM4uJGErAkbfDvA&s=MLRnnYcykjnsqXrHGUuR
> > > > YHc5uDxaAod0Yl7f06EQr1M&e= 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 :)
> 
> [Anoob] Never said it was hidden. Initially I had okayed the approach even though it wasn't a solution suitable for inline protocol. But as
> more cases were considered, few shortcomings were observed, and I was skeptical about how any of that would be solved in the long run.
> 
> My suggestion, if these limitations and the plans to address it were mentioned in the cover letter, we could've saved few cycles here.

Point taken.

> But my reply would still be the same 😊
> 
> > It
> > was outlined here:
> > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__static.sched.com_hosted-5Ffiles_dpdkbordeaux2019_8f_DPDK-
> > 2DIPSECu9.pdf&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyL
> > Ws2n6B-WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-K5XfH8wG-
> > 7hwr1d8oM4uJGErAkbfDvA&s=MLRnnYcykjnsqXrHGUuRYHc5uDxaAod0Yl7f06E
> > Qr1M&e=
> > And in the latest patch version Marcin clearly stated it inside the AG session:
> > https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__patches.dpdk.org_patch_60039_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtf
> > Q&r=jPfB8rwwviRSxyLWs2n6B-WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-
> > K5XfH8wG-
> > 7hwr1d8oM4uJGErAkbfDvA&s=zrn91Vjf_ZElAlDf7IeZGmGevcA6RMwpPTLUCGlgZ
> > cY&e=
> > 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:
> 
> [Anoob] Shouldn't we get that accepted first then?

I don't think so.
Current implementation does provide expected functionality (with known limitations).
In future, we can try to improve it and/or remove existing limitations.
That's a common iteration development approach that is used though whole DPDK:
- provide initial solution with basic functionality first
- improve/extend with future releases/patches

> 
> Also, the ordering is lost because application is not considering that. Why can't we use re-ordering library? Or an event-dev?

No-one saying it can't be improved in one way or another.
We don't plan to introduce such code right now due to its complexity.
Might be something in future.
Though you are welcome to go ahead and submit your own patches to improve that solution.   

> 
> > https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__patches.dpdk.org_cover_58862_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtf
> > Q&r=jPfB8rwwviRSxyLWs2n6B-WYLn1v9SyTMrT5EQqh2TU&m=b3E429fuo8P-
> > K5XfH8wG-
> > 7hwr1d8oM4uJGErAkbfDvA&s=k1zef_1uqELXblKN3_qjX04WXNFGAUEFIIJrvKPr6
> > eA&e=
> > 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.
> 
> [Anoob] Well, that is exactly my problem. It gives a skewed reference on how to deal with the problem, without considering other
> possibilities.

Once again, please feel free and come-up with your own patches, that will address this problem in a smarter way.

> If every application starts adopting this method and starts doing lookup for every ESP packet, h/w based lookups (and the
> associated h/w) would be rendered useless. I really think this series, which happens to be the first step in solving a complex problem, will be
> very useful if all these issues are addressed properly.
> 
> If you can introduce the inline+lookaside via librte_ipsec, its perhaps ok for the patch to have some limitations(like lack of ordering,
> assumptions on ESP etc). However, if the patch is introduced directly into ipsec-secgw, it needs to be more comprehensive and robust.

I think common DPDK policy is exactly opposite:
library/driver has to be as robust and comprehensive as possible.
sample app code allowed to have some limitations (as long as they are documented, etc.). 

> I also expect performance impact due to these changes

Could you be more specific here?
What performance impact do you expect and why?
Can you point to the exact piece of patch code that you believe would cause a degradation.
I don't expect any and performance tests on our platforms didn't show any regression so far.

> and so I prefer if the inline+lookaside can be made separate datapath rather than
> overloading the existing one.

I don't see any good reason for that.
This feature is optional.
User have to enable it manually for each session (via config file).
for all other sessions there is no impact.
What we probably can do - add extra check inside sa.c to allow
fallback sessions only for inline-crypto-offload type of sessions.

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



More information about the dev mailing list