diff mbox

[dpdk-dev,09/11] examples/ipsec-secgw: Fixed ip length in case of transport

Message ID 1507987683-12315-9-git-send-email-aviadye@dev.mellanox.co.il (mailing list archive)
State Changes Requested, archived
Delegated to: Pablo de Lara Guarch
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Aviad Yehezkel Oct. 14, 2017, 1:28 p.m. UTC 
  From: Aviad Yehezkel <aviadye@mellanox.com>

IP length was incorrect causing corrupted ICMP packets for example

Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
---
 examples/ipsec-secgw/esp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Aviad Yehezkel Oct. 15, 2017, 12:56 p.m. UTC | #1 
On 10/14/2017 4:28 PM, aviadye@dev.mellanox.co.il wrote:
> From: Aviad Yehezkel <aviadye@mellanox.com>
>
> IP length was incorrect causing corrupted ICMP packets for example
>
> Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
> ---
>   examples/ipsec-secgw/esp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> index 81ebf55..12c6f8c 100644
> --- a/examples/ipsec-secgw/esp.c
> +++ b/examples/ipsec-secgw/esp.c
> @@ -205,13 +205,13 @@ esp_inbound_post(struct rte_mbuf *m, struct ipsec_sa *sa,
>   		if (likely(ip->ip_v == IPVERSION)) {
>   			memmove(ip4, ip, ip->ip_hl * 4);
>   			ip4->ip_p = *nexthdr;
> -			ip4->ip_len = htons(rte_pktmbuf_data_len(m));
> +			ip4->ip_len = htons(rte_pktmbuf_pkt_len(m));
>   		} else {
>   			ip6 = (struct ip6_hdr *)ip4;
>   			/* XXX No option headers supported */
>   			memmove(ip6, ip, sizeof(struct ip6_hdr));
>   			ip6->ip6_nxt = *nexthdr;
> -			ip6->ip6_plen = htons(rte_pktmbuf_data_len(m));
> +			ip6->ip6_plen = htons(rte_pktmbuf_pkt_len(m));
>   		}
>   	} else
>   		ipip_inbound(m, sizeof(struct esp_hdr) + sa->iv_len);

Tested-by: Aviad Yehezkel <aviadye@mellanox.com>
Sergio Gonzalez Monroy Oct. 16, 2017, 9:43 a.m. UTC | #2 
On 14/10/2017 14:28, aviadye@dev.mellanox.co.il wrote:
> From: Aviad Yehezkel <aviadye@mellanox.com>
>
> IP length was incorrect causing corrupted ICMP packets for example
>
> Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
> ---
>   examples/ipsec-secgw/esp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> index 81ebf55..12c6f8c 100644
> --- a/examples/ipsec-secgw/esp.c
> +++ b/examples/ipsec-secgw/esp.c
> @@ -205,13 +205,13 @@ esp_inbound_post(struct rte_mbuf *m, struct ipsec_sa *sa,
>   		if (likely(ip->ip_v == IPVERSION)) {
>   			memmove(ip4, ip, ip->ip_hl * 4);
>   			ip4->ip_p = *nexthdr;
> -			ip4->ip_len = htons(rte_pktmbuf_data_len(m));
> +			ip4->ip_len = htons(rte_pktmbuf_pkt_len(m));
>   		} else {
>   			ip6 = (struct ip6_hdr *)ip4;
>   			/* XXX No option headers supported */
>   			memmove(ip6, ip, sizeof(struct ip6_hdr));
>   			ip6->ip6_nxt = *nexthdr;
> -			ip6->ip6_plen = htons(rte_pktmbuf_data_len(m));
> +			ip6->ip6_plen = htons(rte_pktmbuf_pkt_len(m));
>   		}
>   	} else
>   		ipip_inbound(m, sizeof(struct esp_hdr) + sa->iv_len);

AFAIK the app does not support multi-segments (chain mbufs), so data_len 
should be the same as pkt_len.
Is that not the case?

Thanks,
Sergio
Aviad Yehezkel Oct. 16, 2017, 11:44 a.m. UTC | #3 
On 10/16/2017 12:43 PM, Sergio Gonzalez Monroy wrote:
> On 14/10/2017 14:28, aviadye@dev.mellanox.co.il wrote:
>> From: Aviad Yehezkel <aviadye@mellanox.com>
>>
>> IP length was incorrect causing corrupted ICMP packets for example
>>
>> Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
>> ---
>>   examples/ipsec-secgw/esp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
>> index 81ebf55..12c6f8c 100644
>> --- a/examples/ipsec-secgw/esp.c
>> +++ b/examples/ipsec-secgw/esp.c
>> @@ -205,13 +205,13 @@ esp_inbound_post(struct rte_mbuf *m, struct 
>> ipsec_sa *sa,
>>           if (likely(ip->ip_v == IPVERSION)) {
>>               memmove(ip4, ip, ip->ip_hl * 4);
>>               ip4->ip_p = *nexthdr;
>> -            ip4->ip_len = htons(rte_pktmbuf_data_len(m));
>> +            ip4->ip_len = htons(rte_pktmbuf_pkt_len(m));
>>           } else {
>>               ip6 = (struct ip6_hdr *)ip4;
>>               /* XXX No option headers supported */
>>               memmove(ip6, ip, sizeof(struct ip6_hdr));
>>               ip6->ip6_nxt = *nexthdr;
>> -            ip6->ip6_plen = htons(rte_pktmbuf_data_len(m));
>> +            ip6->ip6_plen = htons(rte_pktmbuf_pkt_len(m));
>>           }
>>       } else
>>           ipip_inbound(m, sizeof(struct esp_hdr) + sa->iv_len);
>
> AFAIK the app does not support multi-segments (chain mbufs), so 
> data_len should be the same as pkt_len.
> Is that not the case?
>
This is the inbound function (RX side), so mbufs are allocated by PMD.
PMD is allocating mbuf with additional 14 bytes for ETH header but trim 
it before passing the mbuf.
As a result seg len is 14 bytes smaller than data len.

Thanks,
Aviad

> Thanks,
> Sergio
Sergio Gonzalez Monroy Oct. 16, 2017, 12:03 p.m. UTC | #4 
On 16/10/2017 12:44, Aviad Yehezkel wrote:
> On 10/16/2017 12:43 PM, Sergio Gonzalez Monroy wrote:
>> On 14/10/2017 14:28, aviadye@dev.mellanox.co.il wrote:
>>> From: Aviad Yehezkel <aviadye@mellanox.com>
>>>
>>> IP length was incorrect causing corrupted ICMP packets for example
>>>
>>> Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
>>> ---
>>>   examples/ipsec-secgw/esp.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
>>> index 81ebf55..12c6f8c 100644
>>> --- a/examples/ipsec-secgw/esp.c
>>> +++ b/examples/ipsec-secgw/esp.c
>>> @@ -205,13 +205,13 @@ esp_inbound_post(struct rte_mbuf *m, struct 
>>> ipsec_sa *sa,
>>>           if (likely(ip->ip_v == IPVERSION)) {
>>>               memmove(ip4, ip, ip->ip_hl * 4);
>>>               ip4->ip_p = *nexthdr;
>>> -            ip4->ip_len = htons(rte_pktmbuf_data_len(m));
>>> +            ip4->ip_len = htons(rte_pktmbuf_pkt_len(m));
>>>           } else {
>>>               ip6 = (struct ip6_hdr *)ip4;
>>>               /* XXX No option headers supported */
>>>               memmove(ip6, ip, sizeof(struct ip6_hdr));
>>>               ip6->ip6_nxt = *nexthdr;
>>> -            ip6->ip6_plen = htons(rte_pktmbuf_data_len(m));
>>> +            ip6->ip6_plen = htons(rte_pktmbuf_pkt_len(m));
>>>           }
>>>       } else
>>>           ipip_inbound(m, sizeof(struct esp_hdr) + sa->iv_len);
>>
>> AFAIK the app does not support multi-segments (chain mbufs), so 
>> data_len should be the same as pkt_len.
>> Is that not the case?
>>
> This is the inbound function (RX side), so mbufs are allocated by PMD.
> PMD is allocating mbuf with additional 14 bytes for ETH header but 
> trim it before passing the mbuf.
> As a result seg len is 14 bytes smaller than data len.
>

Sorry, I am still missing something here.
rte_pktmbuf_trim updates both data_len and pkt_len, so how can they not 
be the same when we have a single mbuf?

I think to remember using data_len instead of pkt_len so it is easy to 
see that the application does not support multi-segments (aka. chained 
mbufs)

Thanks,
Sergio

> Thanks,
> Aviad
>
>> Thanks,
>> Sergio
>
Aviad Yehezkel Oct. 19, 2017, 6:44 p.m. UTC | #5 
Solved that issue, this was an issue with mlx5 PMD with the new inline 
ipsec code.

The PMD wasn't updating mbuf->data_len correctly.


Thanks!


On 10/16/2017 3:03 PM, Sergio Gonzalez Monroy wrote:
> On 16/10/2017 12:44, Aviad Yehezkel wrote:
>> On 10/16/2017 12:43 PM, Sergio Gonzalez Monroy wrote:
>>> On 14/10/2017 14:28, aviadye@dev.mellanox.co.il wrote:
>>>> From: Aviad Yehezkel <aviadye@mellanox.com>
>>>>
>>>> IP length was incorrect causing corrupted ICMP packets for example
>>>>
>>>> Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
>>>> ---
>>>>   examples/ipsec-secgw/esp.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
>>>> index 81ebf55..12c6f8c 100644
>>>> --- a/examples/ipsec-secgw/esp.c
>>>> +++ b/examples/ipsec-secgw/esp.c
>>>> @@ -205,13 +205,13 @@ esp_inbound_post(struct rte_mbuf *m, struct 
>>>> ipsec_sa *sa,
>>>>           if (likely(ip->ip_v == IPVERSION)) {
>>>>               memmove(ip4, ip, ip->ip_hl * 4);
>>>>               ip4->ip_p = *nexthdr;
>>>> -            ip4->ip_len = htons(rte_pktmbuf_data_len(m));
>>>> +            ip4->ip_len = htons(rte_pktmbuf_pkt_len(m));
>>>>           } else {
>>>>               ip6 = (struct ip6_hdr *)ip4;
>>>>               /* XXX No option headers supported */
>>>>               memmove(ip6, ip, sizeof(struct ip6_hdr));
>>>>               ip6->ip6_nxt = *nexthdr;
>>>> -            ip6->ip6_plen = htons(rte_pktmbuf_data_len(m));
>>>> +            ip6->ip6_plen = htons(rte_pktmbuf_pkt_len(m));
>>>>           }
>>>>       } else
>>>>           ipip_inbound(m, sizeof(struct esp_hdr) + sa->iv_len);
>>>
>>> AFAIK the app does not support multi-segments (chain mbufs), so 
>>> data_len should be the same as pkt_len.
>>> Is that not the case?
>>>
>> This is the inbound function (RX side), so mbufs are allocated by PMD.
>> PMD is allocating mbuf with additional 14 bytes for ETH header but 
>> trim it before passing the mbuf.
>> As a result seg len is 14 bytes smaller than data len.
>>
>
> Sorry, I am still missing something here.
> rte_pktmbuf_trim updates both data_len and pkt_len, so how can they 
> not be the same when we have a single mbuf?
>
> I think to remember using data_len instead of pkt_len so it is easy to 
> see that the application does not support multi-segments (aka. chained 
> mbufs)
>
> Thanks,
> Sergio
>
>> Thanks,
>> Aviad
>>
>>> Thanks,
>>> Sergio
>>
>
diff mbox

Patch

diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
index 81ebf55..12c6f8c 100644
--- a/examples/ipsec-secgw/esp.c
+++ b/examples/ipsec-secgw/esp.c
@@ -205,13 +205,13 @@  esp_inbound_post(struct rte_mbuf *m, struct ipsec_sa *sa,
 		if (likely(ip->ip_v == IPVERSION)) {
 			memmove(ip4, ip, ip->ip_hl * 4);
 			ip4->ip_p = *nexthdr;
-			ip4->ip_len = htons(rte_pktmbuf_data_len(m));
+			ip4->ip_len = htons(rte_pktmbuf_pkt_len(m));
 		} else {
 			ip6 = (struct ip6_hdr *)ip4;
 			/* XXX No option headers supported */
 			memmove(ip6, ip, sizeof(struct ip6_hdr));
 			ip6->ip6_nxt = *nexthdr;
-			ip6->ip6_plen = htons(rte_pktmbuf_data_len(m));
+			ip6->ip6_plen = htons(rte_pktmbuf_pkt_len(m));
 		}
 	} else
 		ipip_inbound(m, sizeof(struct esp_hdr) + sa->iv_len);