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

Ben Pfaff blp at ovn.org
Wed Oct 9 18:18:55 UTC 2019


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>


More information about the dev mailing list