From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5F52DA2EDB for ; Mon, 30 Sep 2019 15:31:23 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C5AF73237; Mon, 30 Sep 2019 15:31:22 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id B1B992A6C for ; Mon, 30 Sep 2019 15:31:20 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Sep 2019 06:31:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,567,1559545200"; d="scan'208";a="204835343" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga001.fm.intel.com with ESMTP; 30 Sep 2019 06:31:17 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.164]) by IRSMSX107.ger.corp.intel.com ([169.254.10.7]) with mapi id 14.03.0439.000; Mon, 30 Sep 2019 14:31:17 +0100 From: "Ananyev, Konstantin" To: Anoob Joseph , "Smoczynski, MarcinX" , "akhil.goyal@nxp.com" CC: "dev@dpdk.org" , Jerin Jacob Kollanukkaran , Narayana Prasad Raju Athreya , Archana Muniganti Thread-Topic: [dpdk-dev] [PATCH v3 0/3] add fallback session Thread-Index: AQHVcgRpybp7qxFpeEujftbhitMyNac9nfaAgAA+IYCABNOdgIABgBhA Date: Mon, 30 Sep 2019 13:31:16 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258019196D5B4@irsmsx105.ger.corp.intel.com> References: <20190904141642.14820-1-marcinx.smoczynski@intel.com> <20190923114415.17932-1-marcinx.smoczynski@intel.com> <2601191342CEEE43887BDE71AB977258019196B087@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzI0NTBlOTItNjdjZi00NmZmLWIyYjUtZDhhZTNlYWVhNjY1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiYXZNY0lrcTlsTFlER0ZXemN4NmdsclZ5KzRtYmhEa1ZIT25IcWpcL2FiVzFhWm4yN3hJT0E1RW9xbnpLazE1c2MifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 0/3] add fallback session X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 pac= ket > > would be looked up to see the associated SA and then fallback session i= s 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 deter= mine 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 pac= ket > > 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, IPv= 6 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 =3D (uintptr_t)sa; > > intsa |=3D IPSEC_SA_OFFLOAD_FALLBACK_FLAG; > > result_sa =3D (void *)intsa; } > > > > So from my perspective, no additional lookup where introduced. >=20 > [Anoob] For inline processing, h/w does a lookup and figures out the secu= rity 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. >=20 > > 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. >=20 > [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, s= ee above. Second, if someone would like to add BYPASS for ESP packets into ipsec-secg= w, 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 tr= affic 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) wou= ld 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. >=20 > For example, ESN was not supported by ipsec-secgw (before library mode wa= s introduced), but every single library change and application > change was added keeping in mind that ESN is a valid feature for ipsec. S= o 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. >=20 > > > > > > > > 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 use= d > > 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, ac= tual > > IPsec packet processing, matching inbound selectors, etc.), but right n= ow 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 librar= y) for the > > same app, so we'd like in future to deprecate and later remove legacy c= ode- > > path. > > As a first step we propose to make library code-path a default one: > > http://patches.dpdk.org/cover/58247/ >=20 > [Anoob] Even if we need the application to do the reassembly, it would lo= ok 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 (s= ession). 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 beli= eve should be moved to ipsec library. >From my side it is a good thing to let application define its own usage mod= el. I.E. librte_ipsec supports multiple-sessions per SA, upper-layer (app) deci= des 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.= =20 >=20 > > > > > > > > 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 i= nline > > processed. > > > b. Application receives P1->P2->P3->P4->P5->P6 (in this, P1, P= 2, P5, P6 are > > inline processed successfully) and P4 & P5 are the fragments > > > c. Application groups packets. P1->P2->P5->P6 becomes one grou= p and P3- > > >P4 becomes another and goes for fallback processing. > > > Now how is ordering maintained? I couldn't figure out how that is don= e 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 win= dow size > > would be needed, what issues/slowdowns it might cause, etc. >=20 > [Anoob] This is something I had noticed while going through the code flow= . The ordering info is lost because of the approach.=20 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 sess= ion: http://patches.dpdk.org/patch/60039/ If you think even further clarification within the doc is needed, please le= t 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 u= ser 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=20 > 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 (pack= et reordering, inline-proto/lksd-proto), it is totally functional and provides users with a reference how to deal wi= th such kind of problems.=20 >=20 > Also, if the new ipsec related features are introduced via ipsec-secgw th= an via rte_ipsec, then it raises doubts over the utility of rte_ipsec > library.