[dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

Akhil Goyal akhil.goyal at nxp.com
Wed Jun 26 09:06:35 CEST 2019


Hi Marius,

> 
> Hi,
> 
> About your comments:
> 
> 1) I used macros around sa->flags where it was needed. Not all checks for that
> set of flags use information if it is transport mode. As for macro
> WITHOUT_TRANSPORT_VERSION, it was set only for checks that required
> information from set of flags without taking into account new transport flags ->
> I can set it in more places (like initialization stage), but I do not see a point of
> that, besides being uniform.

I think it would be better if we are adding certain flags to simplify code, we should add
In all the checks.
I can see that in single if else sequence there are 2 different ways to check for the values
Of sa->flags. I think that this can be avoided.


> 
> 2) WITHOUT_TRANSPORT_VERSION is a macro which masks sa->flags as they
> were before change. It cuts newly proposed flags for transport mode, so
> behavior of switches, where such flags were used before as variable, is
> unchanged. I will provide a comment to the macro.
> 
> I will provide patch as soon as possible (probably tomorrow).
> 
> Kind regards,
> Mariusz Drost.
> 
> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Tuesday, June 25, 2019 3:15 PM
> To: Akhil Goyal <akhil.goyal at nxp.com>; Drost, MariuszX
> <mariuszx.drost at intel.com>; Nicolau, Radu <radu.nicolau at intel.com>; Lu,
> Wenzhuo <wenzhuo.lu at intel.com>; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>
> Cc: dev at dpdk.org
> Subject: RE: [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec
> modes
> 
> Hi Marius,
> 
> Could you please send the updated patch soon, so that they can be applied
> before RC1.
> 
> Thanks,
> Akhil
> 
> >
> > 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 at 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


More information about the dev mailing list