diff mbox series

examples/l3fwd: fix unaligned memory access

Message ID 20190724164354.18811-1-hariprasad.govindharajan@intel.com (mailing list archive)
State Superseded, archived
Headers
Series examples/l3fwd: fix unaligned memory access |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-Compile-Testing success Compile Testing PASS
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Hariprasad Govindharajan July 24, 2019, 4:43 p.m. UTC 
  Fix unaligned memory access when reading IPv6 header which
leads to segmentation fault by changing aligned memory read
to unaligned memory read.

Bugzilla ID: 279
Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
Cc: maciej.czekaj@caviumnetworks.com
Cc: stable@dpdk.org
Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
---
 examples/l3fwd/l3fwd_em.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ruifeng Wang July 25, 2019, 7:01 a.m. UTC | #1 
Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of hgovindh
> Sent: Thursday, July 25, 2019 00:44
> To: Remy Horton <remy.horton@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.com>; Ori Kam <orika@mellanox.com>; Bruce
> Richardson <bruce.richardson@intel.com>; Pablo de Lara
> <pablo.de.lara.guarch@intel.com>; Radu Nicolau <radu.nicolau@intel.com>;
> Akhil.goyal@nxp.com; Tomasz Kantecki <tomasz.kantecki@intel.com>
> Cc: dev@dpdk.org; hgovindh <hariprasad.govindharajan@intel.com>;
> maciej.czekaj@caviumnetworks.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] examples/l3fwd: fix unaligned memory access
> 
> Fix unaligned memory access when reading IPv6 header which leads to
> segmentation fault by changing aligned memory read to unaligned memory
> read.
> 
> Bugzilla ID: 279
> Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
> Cc: maciej.czekaj@caviumnetworks.com
> Cc: stable@dpdk.org
> Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
> ---
>  examples/l3fwd/l3fwd_em.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
> index fa8f82be6..f0c443dae 100644
> --- a/examples/l3fwd/l3fwd_em.c
> +++ b/examples/l3fwd/l3fwd_em.c
> @@ -285,7 +285,7 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t
> portid, void *lookup_struct)
>  	 * Get part of 5 tuple: dst IP address lower 96 bits
>  	 * and src IP address higher 32 bits.
>  	 */
> -	key.xmm[1] = *(xmm_t *)data1;
> +	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);

The use of SSE intrinsics on general path will break build on other architectures.
How about use em_mask_key() instead?
> 
>  	/*
>  	 * Get part of 5 tuple: dst port and src port
> --
> 2.22.0
Bruce Richardson July 25, 2019, 9:05 a.m. UTC | #2 
On Wed, Jul 24, 2019 at 05:43:54PM +0100, hgovindh wrote:
> Fix unaligned memory access when reading IPv6 header which
> leads to segmentation fault by changing aligned memory read
> to unaligned memory read.
> 
> Bugzilla ID: 279
> Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
> Cc: maciej.czekaj@caviumnetworks.com
> Cc: stable@dpdk.org
> Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
> ---
>  examples/l3fwd/l3fwd_em.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
> index fa8f82be6..f0c443dae 100644
> --- a/examples/l3fwd/l3fwd_em.c
> +++ b/examples/l3fwd/l3fwd_em.c
> @@ -285,7 +285,7 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t portid, void *lookup_struct)
>  	 * Get part of 5 tuple: dst IP address lower 96 bits
>  	 * and src IP address higher 32 bits.
>  	 */
> -	key.xmm[1] = *(xmm_t *)data1;
> +	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);
>  

Minor nit, but since data1 is defined as "void *" the cast to xmm_t is
unnecessary.

Although we can't reproduce the bug, the fix looks correct for the bug as
described and harmless otherwise, so:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Aaron Conole July 25, 2019, 1:27 p.m. UTC | #3 
hgovindh <hariprasad.govindharajan@intel.com> writes:

> Fix unaligned memory access when reading IPv6 header which
> leads to segmentation fault by changing aligned memory read
> to unaligned memory read.
>
> Bugzilla ID: 279
> Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
> Cc: maciej.czekaj@caviumnetworks.com
> Cc: stable@dpdk.org
> Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
> ---
>  examples/l3fwd/l3fwd_em.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
> index fa8f82be6..f0c443dae 100644
> --- a/examples/l3fwd/l3fwd_em.c
> +++ b/examples/l3fwd/l3fwd_em.c
> @@ -285,7 +285,7 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t portid, void *lookup_struct)
>  	 * Get part of 5 tuple: dst IP address lower 96 bits
>  	 * and src IP address higher 32 bits.
>  	 */
> -	key.xmm[1] = *(xmm_t *)data1;
> +	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);

Nak.  Please use a generic unaligned load, rather than an intel specific
one.  Otherwise, supported platforms like arm64 will have broken builds.

Additionally, which chip and compiler did you use to get this error?

>  
>  	/*
>  	 * Get part of 5 tuple: dst port and src port
Anatoly Burakov July 25, 2019, 2:01 p.m. UTC | #4 
On 25-Jul-19 2:27 PM, Aaron Conole wrote:
> hgovindh <hariprasad.govindharajan@intel.com> writes:
> 
>> Fix unaligned memory access when reading IPv6 header which
>> leads to segmentation fault by changing aligned memory read
>> to unaligned memory read.
>>
>> Bugzilla ID: 279
>> Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
>> Cc: maciej.czekaj@caviumnetworks.com
>> Cc: stable@dpdk.org
>> Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
>> ---
>>   examples/l3fwd/l3fwd_em.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
>> index fa8f82be6..f0c443dae 100644
>> --- a/examples/l3fwd/l3fwd_em.c
>> +++ b/examples/l3fwd/l3fwd_em.c
>> @@ -285,7 +285,7 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t portid, void *lookup_struct)
>>   	 * Get part of 5 tuple: dst IP address lower 96 bits
>>   	 * and src IP address higher 32 bits.
>>   	 */
>> -	key.xmm[1] = *(xmm_t *)data1;
>> +	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);
> 
> Nak.  Please use a generic unaligned load, rather than an intel specific
> one.  Otherwise, supported platforms like arm64 will have broken builds.
> 
> Additionally, which chip and compiler did you use to get this error?

I have reproduced this error on Intel Xeon E5-2699 and GCC 7.4 (Ubuntu 
18.04).

> 
>>   
>>   	/*
>>   	 * Get part of 5 tuple: dst port and src port
>
Herakliusz Lipiec July 25, 2019, 2:01 p.m. UTC | #5 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Aaron Conole
> Sent: Thursday, July 25, 2019 2:28 PM
> hgovindh <hariprasad.govindharajan@intel.com> writes:
> 
> > Fix unaligned memory access when reading IPv6 header which leads to
> > segmentation fault by changing aligned memory read to unaligned memory
> > read.
> >
> > Bugzilla ID: 279
> > Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
> > Cc: maciej.czekaj@caviumnetworks.com
> > Cc: stable@dpdk.org
> > Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
> > ---
> >  examples/l3fwd/l3fwd_em.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
> > index fa8f82be6..f0c443dae 100644
> > --- a/examples/l3fwd/l3fwd_em.c
> > +++ b/examples/l3fwd/l3fwd_em.c
> > @@ -285,7 +285,7 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t
> portid, void *lookup_struct)
> >  	 * Get part of 5 tuple: dst IP address lower 96 bits
> >  	 * and src IP address higher 32 bits.
> >  	 */
> > -	key.xmm[1] = *(xmm_t *)data1;
> > +	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);
> 
> Nak.  Please use a generic unaligned load, rather than an intel specific one.
> Otherwise, supported platforms like arm64 will have broken builds.
> 
> Additionally, which chip and compiler did you use to get this error?
This comes from Bugzilla, the compiler used there is GCC 8.2.0 and the CPU is Intel Core i5,
As far as I know this can also be reproduced on Intel Xeon, with GCC 8.3.0, 
in both cases its compiled with compiler optimizations disabled.
> 
> >
> >  	/*
> >  	 * Get part of 5 tuple: dst port and src port
Hariprasad Govindharajan July 25, 2019, 2:08 p.m. UTC | #6 
-----Original Message-----
From: Burakov, Anatoly 
Sent: Thursday, July 25, 2019 3:01 PM
To: Aaron Conole <aconole@redhat.com>; Govindharajan, Hariprasad <hariprasad.govindharajan@intel.com>
Cc: Remy Horton <remy.horton@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>; Ori Kam <orika@mellanox.com>; Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; Kantecki, Tomasz <tomasz.kantecki@intel.com>; dev@dpdk.org; stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: fix unaligned memory access

On 25-Jul-19 2:27 PM, Aaron Conole wrote:
> hgovindh <hariprasad.govindharajan@intel.com> writes:
> 
>> Fix unaligned memory access when reading IPv6 header which leads to 
>> segmentation fault by changing aligned memory read to unaligned 
>> memory read.
>>
>> Bugzilla ID: 279
>> Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
>> Cc: maciej.czekaj@caviumnetworks.com
>> Cc: stable@dpdk.org
>> Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
>> ---
>>   examples/l3fwd/l3fwd_em.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c 
>> index fa8f82be6..f0c443dae 100644
>> --- a/examples/l3fwd/l3fwd_em.c
>> +++ b/examples/l3fwd/l3fwd_em.c
>> @@ -285,7 +285,7 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t portid, void *lookup_struct)
>>   	 * Get part of 5 tuple: dst IP address lower 96 bits
>>   	 * and src IP address higher 32 bits.
>>   	 */
>> -	key.xmm[1] = *(xmm_t *)data1;
>> +	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);
> 
> Nak.  Please use a generic unaligned load, rather than an intel 
> specific one.  Otherwise, supported platforms like arm64 will have broken builds.
> 
> Additionally, which chip and compiler did you use to get this error?

I have reproduced this error on Intel Xeon E5-2699 and GCC 7.4 (Ubuntu 18.04).

I have reproduced this error on Intel(R) Xeon(R) CPU and GCC 8.3.0 (Ubuntu 16.04).
> 
>>   
>>   	/*
>>   	 * Get part of 5 tuple: dst port and src port
> 


--
Thanks,
G Hariprasad
Anatoly Burakov July 25, 2019, 2:23 p.m. UTC | #7 
On 25-Jul-19 3:01 PM, Lipiec, Herakliusz wrote:
> 
> 
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Aaron Conole
>> Sent: Thursday, July 25, 2019 2:28 PM
>> hgovindh <hariprasad.govindharajan@intel.com> writes:
>>
>>> Fix unaligned memory access when reading IPv6 header which leads to
>>> segmentation fault by changing aligned memory read to unaligned memory
>>> read.
>>>
>>> Bugzilla ID: 279
>>> Fixes: 64d3955de1de ("examples/l3fwd: fix ARM build")
>>> Cc: maciej.czekaj@caviumnetworks.com
>>> Cc: stable@dpdk.org
>>> Signed-off-by: hgovindh <hariprasad.govindharajan@intel.com>
>>> ---
>>>   examples/l3fwd/l3fwd_em.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
>>> index fa8f82be6..f0c443dae 100644
>>> --- a/examples/l3fwd/l3fwd_em.c
>>> +++ b/examples/l3fwd/l3fwd_em.c
>>> @@ -285,7 +285,7 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t
>> portid, void *lookup_struct)
>>>   	 * Get part of 5 tuple: dst IP address lower 96 bits
>>>   	 * and src IP address higher 32 bits.
>>>   	 */
>>> -	key.xmm[1] = *(xmm_t *)data1;
>>> +	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);
>>
>> Nak.  Please use a generic unaligned load, rather than an intel specific one.
>> Otherwise, supported platforms like arm64 will have broken builds.
>>
>> Additionally, which chip and compiler did you use to get this error?
> This comes from Bugzilla, the compiler used there is GCC 8.2.0 and the CPU is Intel Core i5,
> As far as I know this can also be reproduced on Intel Xeon, with GCC 8.3.0,
> in both cases its compiled with compiler optimizations disabled.

I have reproduced it with compiler optimizations enabled as well.

>>
>>>
>>>   	/*
>>>   	 * Get part of 5 tuple: dst port and src port
>
diff mbox series

Patch

diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
index fa8f82be6..f0c443dae 100644
--- a/examples/l3fwd/l3fwd_em.c
+++ b/examples/l3fwd/l3fwd_em.c
@@ -285,7 +285,7 @@  em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t portid, void *lookup_struct)
 	 * Get part of 5 tuple: dst IP address lower 96 bits
 	 * and src IP address higher 32 bits.
 	 */
-	key.xmm[1] = *(xmm_t *)data1;
+	key.xmm[1] = _mm_loadu_si128((xmm_t *)data1);
 
 	/*
 	 * Get part of 5 tuple: dst port and src port