Re: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: fix usage of incorrect port

[Anoob <anoob.joseph@caviumnetworks.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);
>>       }
>>   }
>>
>