From: Akhil Goyal <akhil.goyal@nxp.com>
To: Mariusz Drost <mariuszx.drost@intel.com>,
"radu.nicolau@intel.com" <radu.nicolau@intel.com>,
"wenzhuo.lu@intel.com" <wenzhuo.lu@intel.com>,
"konstantin.ananyev@intel.com" <konstantin.ananyev@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes
Date: Thu, 20 Jun 2019 13:08:53 +0000 [thread overview]
Message-ID: <VE1PR04MB6639CDDF606A0530CF24EE0CE6E40@VE1PR04MB6639.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20190604100644.13724-3-mariuszx.drost@intel.com>
Hi Marius,
> Application ipsec-secgw is not working for IPv4 transport mode and for
> IPv6 both transport and tunnel mode.
>
> IPv6 tunnel mode is not working due to wrongly assigned fields of
> security association patterns, as it was IPv4, during creation of
> inline crypto session.
>
> IPv6 and IPv4 transport mode is iterating through security capabilities
> until it reaches tunnel, which causes session to be created as tunnel,
> instead of transport. Another issue, is that config file does not
> provide source and destination ip addresses for transport mode, which
> are required by NIC to perform inline crypto. It uses default addresses
> stored in security association (all zeroes), which causes dropped
> packages.
>
> To fix that, reorganization of code in create_session() is needed,
> to behave appropriately to given protocol (IPv6/IPv4). Change in
> iteration through security capabilities is also required, to check
> for expected mode (not only tunnel).
>
> For lack of addresses issue, some resolving mechanism is needed.
> Approach is to store addresses in security association, as it is
> for tunnel mode. Difference is that they are obtained from sp rules,
> instead of config file. To do that, sp[4/6]_spi_present() function
> is used to find addresses based on spi value, and then stored in
> corresponding sa rule. This approach assumes, that every sp rule
> for inline crypto have valid addresses, as well as range of addresses
> is not supported.
>
> New flags for ipsec_sa structure are required to distinguish between
> IPv4 and IPv6 transport modes. Because of that, there is need to
> change all checks done on these flags, so they work as expected.
>
> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
>
This is a very well written description. Thanks. This helps in review of the patch.
I have a few small comments, rest all is fine.
> Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
> ---
> examples/ipsec-secgw/esp.c | 12 +--
> examples/ipsec-secgw/ipsec.c | 19 +++--
> examples/ipsec-secgw/ipsec.h | 21 +++++-
> examples/ipsec-secgw/sa.c | 142 ++++++++++++++++++++++++++---------
> examples/ipsec-secgw/sp4.c | 24 +++++-
> examples/ipsec-secgw/sp6.c | 42 ++++++++++-
> 6 files changed, 205 insertions(+), 55 deletions(-)
>
> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> index f11d095ba..764e08dcf 100644
> --- a/examples/ipsec-secgw/esp.c
> +++ b/examples/ipsec-secgw/esp.c
> @@ -192,7 +192,7 @@ esp_inbound_post(struct rte_mbuf *m, struct ipsec_sa
> *sa,
> }
> }
>
> - if (unlikely(sa->flags == TRANSPORT)) {
> + if (unlikely(IS_TRANSPORT(sa->flags))) {
> ip = rte_pktmbuf_mtod(m, struct ip *);
> ip4 = (struct ip *)rte_pktmbuf_adj(m,
> sizeof(struct rte_esp_hdr) + sa->iv_len);
> @@ -233,13 +233,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa
> *sa,
>
> ip4 = rte_pktmbuf_mtod(m, struct ip *);
> if (likely(ip4->ip_v == IPVERSION)) {
> - if (unlikely(sa->flags == TRANSPORT)) {
> + if (unlikely(IS_TRANSPORT(sa->flags))) {
> ip_hdr_len = ip4->ip_hl * 4;
> nlp = ip4->ip_p;
> } else
> nlp = IPPROTO_IPIP;
> } else if (ip4->ip_v == IP6_VERSION) {
> - if (unlikely(sa->flags == TRANSPORT)) {
> + if (unlikely(IS_TRANSPORT(sa->flags))) {
> /* XXX No option headers supported */
> ip_hdr_len = sizeof(struct ip6_hdr);
> ip6 = (struct ip6_hdr *)ip4;
> @@ -258,13 +258,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa
> *sa,
> pad_len = pad_payload_len + ip_hdr_len - rte_pktmbuf_pkt_len(m);
>
> RTE_ASSERT(sa->flags == IP4_TUNNEL || sa->flags == IP6_TUNNEL ||
> - sa->flags == TRANSPORT);
> + IS_TRANSPORT(sa->flags));
I can see that at multiple places, sa->flags are accessed without your defined macros. Could you please update this at all places, so that it will be uniform across the application.
>
> if (likely(sa->flags == IP4_TUNNEL))
> ip_hdr_len = sizeof(struct ip);
> else if (sa->flags == IP6_TUNNEL)
> ip_hdr_len = sizeof(struct ip6_hdr);
> - else if (sa->flags != TRANSPORT) {
> + else if (!IS_TRANSPORT(sa->flags)) {
> RTE_LOG(ERR, IPSEC_ESP, "Unsupported SA flags: 0x%x\n",
> sa->flags);
> return -EINVAL;
> @@ -291,7 +291,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> rte_prefetch0(padding);
> }
>
> - switch (sa->flags) {
> + switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
I do not get the intent of this macro " WITHOUT_TRANSPORT_VERSION ". could you explain this in comments or some better name of the macro.
> case IP4_TUNNEL:
> ip4 = ip4ip_outbound(m, sizeof(struct rte_esp_hdr) + sa->iv_len,
> &sa->src, &sa->dst);
Regards,
Akhil
next prev parent reply other threads:[~2019-06-20 13:08 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-04 10:06 [dpdk-dev] [PATCH 0/2] fixes for inline-crypto ipsec Mariusz Drost
2019-06-04 10:06 ` [dpdk-dev] [PATCH 1/2] net/ixgbe: fix lack of ip type for crypto session Mariusz Drost
2019-06-06 10:43 ` Ananyev, Konstantin
2019-06-04 10:06 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes Mariusz Drost
2019-06-06 10:43 ` Ananyev, Konstantin
2019-06-20 13:08 ` Akhil Goyal [this message]
2019-06-25 13:14 ` Akhil Goyal
2019-06-25 13:48 ` Drost, MariuszX
2019-06-26 7:06 ` Akhil Goyal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=VE1PR04MB6639CDDF606A0530CF24EE0CE6E40@VE1PR04MB6639.eurprd04.prod.outlook.com \
--to=akhil.goyal@nxp.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=mariuszx.drost@intel.com \
--cc=radu.nicolau@intel.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).