DPDK patches and discussions
 help / color / mirror / Atom feed
From: Anoob <anoob.joseph@caviumnetworks.com>
To: Akhil Goyal <akhil.goyal@nxp.com>,
	Declan Doherty <declan.doherty@intel.com>,
	Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>,
	Radu Nicolau <radu.nicolau@intel.com>
Cc: Narayana Prasad <narayanaprasad.athreya@caviumnetworks.com>,
	Jerin Jacob <jerin.jacob@caviumnetworks.com>,
	dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: fix usage of incorrect port
Date: Fri, 24 Nov 2017 15:28:38 +0530	[thread overview]
Message-ID: <c507fee2-5874-5931-ce12-1119251839b3@caviumnetworks.com> (raw)
In-Reply-To: <0349861e-de98-92b5-8b6f-7ab944dd45bf@nxp.com>

Hi Akhil,

Please see inline.

Thanks,

Anoob


On 11/24/2017 02:58 PM, Akhil Goyal wrote:
> Hi Anoob,
>
> On 11/15/2017 3:11 PM, Anoob Joseph wrote:
>> When security offload is enabled, the packet should be forwarded on the
>> port configured in the SA. Security session will be configured on that
>> port only, and sending the packet on other ports could result in
>> unencrypted packets being sent out.
>>
>> This would have performance improvements too, as the per packet LPM
>> lookup would be avoided for IPsec packets, in inline mode.
>>
>> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
>>
>> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
>> ---
>> v3
>> * Bug fix (fixed a wrong if condition)
>> * Minor changes in documentation
>>
>> v2:
>> * Updated documentation with the change in behavior for outbound inline
>>    offloaded packets.
>>
>>   doc/guides/sample_app_ug/ipsec_secgw.rst | 10 +++-
>>   examples/ipsec-secgw/ipsec-secgw.c       | 92 
>> +++++++++++++++++++++++++++-----
>>   2 files changed, 87 insertions(+), 15 deletions(-)
>>
>> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst 
>> b/doc/guides/sample_app_ug/ipsec_secgw.rst
>> index d6cfdbf..ae18acd 100644
>> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
>> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
>> @@ -61,6 +61,12 @@ In case of complete protocol offload, the 
>> processing of headers(ESP and outer
>>   IP header) is done by the hardware and the application does not 
>> need to
>>   add/remove them during outbound/inbound processing.
>>   +For inline offloaded outbound traffic, the application will not do 
>> the LPM
>> +lookup for routing, as the port on which the packet has to be 
>> forwarded will be
>> +part of the SA. Security parameters will be configured on that port 
>> only, and
>> +sending the packet on other ports could result in unencrypted 
>> packets being
>> +sent out.
>> +
>>   The Path for IPsec Inbound traffic is:
>>     *  Read packets from the port.
>> @@ -543,7 +549,9 @@ where each options means:
>>    ``<port_id>``
>>      * Port/device ID of the ethernet/crypto accelerator for which 
>> the SA is
>> -   configured. This option is used when *type* is NOT *no-offload*
>> +   configured. For *inline-crypto-offload* and 
>> *inline-protocol-offload*, this
>> +   port will be used for routing. The routing table will not be 
>> referred in
>> +   this case.
>>      * Optional: No, if *type* is not *no-offload*
>>   diff --git a/examples/ipsec-secgw/ipsec-secgw.c 
>> b/examples/ipsec-secgw/ipsec-secgw.c
>> index c98454a..cfcb9d5 100644
>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>> @@ -585,31 +585,72 @@ process_pkts_outbound_nosp(struct ipsec_ctx 
>> *ipsec_ctx,
>>           traffic->ip6.num = nb_pkts_out;
>>   }
>>   +static inline int32_t
>> +get_hop_for_offload_pkt(struct rte_mbuf *pkt)
>> +{
>> +    struct ipsec_mbuf_metadata *priv;
>> +    struct ipsec_sa *sa;
>> +
>> +    priv = get_priv(pkt);
>> +
>> +    sa = priv->sa;
>> +    if (unlikely(sa == NULL)) {
>> +        RTE_LOG(ERR, IPSEC, "SA not saved in private data\n");
>> +        return -1;
>> +    }
>> +
>> +    return sa->portid;
>> +}
>> +
>>   static inline void
>>   route4_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf *pkts[], uint8_t 
>> nb_pkts)
>>   {
>>       uint32_t hop[MAX_PKT_BURST * 2];
>>       uint32_t dst_ip[MAX_PKT_BURST * 2];
>> +    int32_t pkt_hop = 0;
>>       uint16_t i, offset;
>> +    uint16_t lpm_pkts = 0;
>>         if (nb_pkts == 0)
>>           return;
>>   +    /* Need to do an LPM lookup for non-offload packets. Offload 
>> packets
>> +     * will have port ID in the SA
>> +     */
>> +
>>       for (i = 0; i < nb_pkts; i++) {
>> -        offset = offsetof(struct ip, ip_dst);
>> -        dst_ip[i] = *rte_pktmbuf_mtod_offset(pkts[i],
>> -                uint32_t *, offset);
>> -        dst_ip[i] = rte_be_to_cpu_32(dst_ip[i]);
>> +        if (!(pkts[i]->ol_flags & PKT_TX_SEC_OFFLOAD)) {
>> +            /* Security offload not enabled. So an LPM lookup is
>> +             * required to get the hop
>> +             */
>> +            offset = offsetof(struct ip, ip_dst);
>> +            dst_ip[lpm_pkts] = *rte_pktmbuf_mtod_offset(pkts[i],
>> +                    uint32_t *, offset);
>> +            dst_ip[lpm_pkts] = rte_be_to_cpu_32(dst_ip[lpm_pkts]);
>> +            lpm_pkts++;
>> +        }
>>       }
>>   -    rte_lpm_lookup_bulk((struct rte_lpm *)rt_ctx, dst_ip, hop, 
>> nb_pkts);
>> +    rte_lpm_lookup_bulk((struct rte_lpm *)rt_ctx, dst_ip, hop, 
>> lpm_pkts);
>> +
>> +    lpm_pkts = 0;
>>         for (i = 0; i < nb_pkts; i++) {
>> -        if ((hop[i] & RTE_LPM_LOOKUP_SUCCESS) == 0) {
>> +        if (pkts[i]->ol_flags & PKT_TX_SEC_OFFLOAD) {
>> +            /* Read hop from the SA */
>> +            pkt_hop = get_hop_for_offload_pkt(pkts[i]);
>> +        } else {
>> +            /* Need to use hop returned by lookup */
>> +            pkt_hop = hop[lpm_pkts++];
>> +            if ((pkt_hop & RTE_LPM_LOOKUP_SUCCESS) == 0)
>> +                pkt_hop = -1;
>> +        }
>> +
> I believe the following check is redundant for non inline case. I 
> believe get_hop_for_offload_pkt can also set the 
> RTE_LPM_LOOKUP_SUCCESS if route is success and take the (pkt_hop & 
> RTE_LPM_LOOKUP_SUCCESS) == 0 check outside the if else block and free 
> the packet if it is unsuccessful.
>
> Same comment for route6_pkts. Checking with -1 may not be a good idea 
> if we have a flag available for the same.
> Others can comment.
The problem is ipv4 & ipv6 LPM lookups return different error values, 
but we are using a single routine to get the hop for offload packets. 
The flag(RTE_LPM_LOOKUP_SUCCESS) is only for ipv4 lookups. For ipv6, 
error is -1. If we need a cleaner solution, we can have ipv4 & ipv6 
variants of "get_hop_for_offload_pkt". But that would be repetition of 
some code.
>
>> +        if (pkt_hop == -1) {
>>               rte_pktmbuf_free(pkts[i]);
>>               continue;
>>           }
>> -        send_single_packet(pkts[i], hop[i] & 0xff);
>> +        send_single_packet(pkts[i], pkt_hop & 0xff);
>>       }
>>   }
>>   @@ -619,26 +660,49 @@ route6_pkts(struct rt_ctx *rt_ctx, struct 
>> rte_mbuf *pkts[], uint8_t nb_pkts)
>>       int32_t hop[MAX_PKT_BURST * 2];
>>       uint8_t dst_ip[MAX_PKT_BURST * 2][16];
>>       uint8_t *ip6_dst;
>> +    int32_t pkt_hop = 0;
>>       uint16_t i, offset;
>> +    uint16_t lpm_pkts = 0;
>>         if (nb_pkts == 0)
>>           return;
>>   +    /* Need to do an LPM lookup for non-offload packets. Offload 
>> packets
>> +     * will have port ID in the SA
>> +     */
>> +
>>       for (i = 0; i < nb_pkts; i++) {
>> -        offset = offsetof(struct ip6_hdr, ip6_dst);
>> -        ip6_dst = rte_pktmbuf_mtod_offset(pkts[i], uint8_t *, offset);
>> -        memcpy(&dst_ip[i][0], ip6_dst, 16);
>> +        if (!(pkts[i]->ol_flags & PKT_TX_SEC_OFFLOAD)) {
>> +            /* Security offload not enabled. So an LPM lookup is
>> +             * required to get the hop
>> +             */
>> +            offset = offsetof(struct ip6_hdr, ip6_dst);
>> +            ip6_dst = rte_pktmbuf_mtod_offset(pkts[i], uint8_t *,
>> +                    offset);
>> +            memcpy(&dst_ip[lpm_pkts][0], ip6_dst, 16);
>> +            lpm_pkts++;
>> +        }
>>       }
>>   -    rte_lpm6_lookup_bulk_func((struct rte_lpm6 *)rt_ctx, dst_ip,
>> -            hop, nb_pkts);
>> +    rte_lpm6_lookup_bulk_func((struct rte_lpm6 *)rt_ctx, dst_ip, hop,
>> +            lpm_pkts);
>> +
>> +    lpm_pkts = 0;
>>         for (i = 0; i < nb_pkts; i++) {
>> -        if (hop[i] == -1) {
>> +        if (pkts[i]->ol_flags & PKT_TX_SEC_OFFLOAD) {
>> +            /* Read hop from the SA */
>> +            pkt_hop = get_hop_for_offload_pkt(pkts[i]);
>> +        } else {
>> +            /* Need to use hop returned by lookup */
>> +            pkt_hop = hop[lpm_pkts++];
>> +        }
>> +
>> +        if (pkt_hop == -1) {
>>               rte_pktmbuf_free(pkts[i]);
>>               continue;
>>           }
>> -        send_single_packet(pkts[i], hop[i] & 0xff);
>> +        send_single_packet(pkts[i], pkt_hop & 0xff);
>>       }
>>   }
>>
>

  reply	other threads:[~2017-11-24  9:58 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-13 16:13 [dpdk-dev] [PATCH] " Anoob Joseph
2017-11-13 17:23 ` Radu Nicolau
2017-11-13 19:24   ` Anoob Joseph
2017-11-14 12:01     ` Nicolau, Radu
2017-11-14 15:37 ` [dpdk-dev] [PATCH v2] " Anoob Joseph
2017-11-14 16:16   ` Radu Nicolau
2017-11-15  9:41   ` [dpdk-dev] [PATCH v3] " Anoob Joseph
2017-11-24  9:28     ` Akhil Goyal
2017-11-24  9:58       ` Anoob [this message]
2017-11-24 10:49         ` Akhil Goyal
2017-11-29  4:21           ` Anoob Joseph
2017-12-04  7:49             ` Akhil Goyal
2017-12-06 11:08               ` Anoob
2017-12-11 10:26                 ` Radu Nicolau
2017-12-11 10:38                   ` Anoob Joseph
2017-12-11 15:35     ` [dpdk-dev] [PATCH v4] " Anoob Joseph
2017-12-12  6:54       ` Anoob Joseph
2017-12-12  7:34       ` Akhil Goyal
2017-12-12  8:32       ` [dpdk-dev] [PATCH v5] " Anoob Joseph
2017-12-12 11:27         ` Radu Nicolau
2017-12-14  9:01           ` De Lara Guarch, Pablo

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=c507fee2-5874-5931-ce12-1119251839b3@caviumnetworks.com \
    --to=anoob.joseph@caviumnetworks.com \
    --cc=akhil.goyal@nxp.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=jerin.jacob@caviumnetworks.com \
    --cc=narayanaprasad.athreya@caviumnetworks.com \
    --cc=radu.nicolau@intel.com \
    --cc=sergio.gonzalez.monroy@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).