[ovs-dev] [PATCH] flow: Fix using pointer to member of packed struct icmp6_hdr.

Ilya Maximets i.maximets at ovn.org
Thu Oct 10 09:41:45 UTC 2019


On 09.10.2019 20:18, Ben Pfaff wrote:
> On Wed, Oct 09, 2019 at 06:54:29PM +0200, Ilya Maximets wrote:
>> On 08.10.2019 18:55, William Tu wrote:
>>> On Tue, Oct 01, 2019 at 08:04:00PM +0300, Ilya Maximets wrote:
>>>> OVS has no structure definition for ICMPv6 header with additional
>>>> data. More precisely, it has, but this structure named as
>>>> 'icmp6_error_header' and only suitable to store error related
>>>> extended information.  'flow_compose_l4' stores additional
>>>> information in reserved bits by using system defined structure
>>>> 'icmp6_hdr', which is marked as 'packed' and this leads to
>>>> build failure with gcc >= 9:
>>>>
>>>>     lib/flow.c:3041:34: error:
>>>>       taking address of packed member of 'struct icmp6_hdr' may result
>>>>       in an unaligned pointer value [-Werror=address-of-packed-member]
>>>>
>>>>           uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
>>>>                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> Fix that by renaming 'icmp6_error_header' to 'icmp6_data_header'
>>>> and allowing it to store not only errors, but any type of additional
>>>> information by analogue with 'struct icmp6_hdr'.
>>>> All the usages of 'struct icmp6_hdr' replaced with this new structure.
>>>> Removed redundant conversions between network and host representations.
>>>> Now fields are always in be.
>>>>
>>>> This also, probably, makes flow_compose_l4 more robust by avoiding
>>>> possible unaligned accesses to 32 bit value.
>>>>
>>>> Fixes: 9b2b84973db7 ("Support for match & set ICMPv6 reserved and options type fields")
>>>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>>>
>>> Looks good to me! Thanks.
>>> Tested on travis and cirrus.
>>>
>>> Acked-by: William Tu <u9012063 at gmail.com>
>>
>> Thanks, William!
>>
>> Ben, could you, please, take a look at this too?
>>
>> I just want to be sure that this change is aligned with
>> expectations from the network header structures in OVS.
> 
> It seems like a positive change.  We used to have periodic problems with
> misaligned accesses on RISC architectures and moving to the 16aligned_*
> types, while annoying in some ways, has avoided those.  This change is
> helpful because it updates a struct that somehow had been missed
> previously.
> 
> Acked-by: Ben Pfaff <blp at ovn.org>
> 

Thanks! I applied this to master and backported to 2.12 with
a slight addition for OVN code.
I'll send a patch for OVN repo with these changes.

Best regards, Ilya Maximets.


More information about the dev mailing list