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