Re: [ovs-dev] [PATCH 1/5] odp-execute: Fix unaligned eth_addr pointers.

[Ben Pfaff]

On Thu, May 25, 2017 at 01:52:16PM -0700, Joe Stringer wrote:
> On 25 May 2017 at 13:35, Ben Pfaff  wrote:
> > On Thu, May 25, 2017 at 01:05:11PM -0700, Joe Stringer wrote:
> >> On 24 May 2017 at 20:02, Ben Pfaff  wrote:
> >> > On Tue, May 23, 2017 at 04:02:12PM -0700, Joe Stringer wrote:
> >> >> Clang 4.0 complains:
> >> >>
> >> >> ../lib/odp-execute.c:61:37: error: taking address of packed member 
> >> >> 'eth_dst' of
> >> >> class or structure 'eth_header' may result in an unaligned pointer value
> >> >>   [-Werror,-Waddress-of-packed-member]
> >> >> ether_addr_copy_masked(>eth_src, key->eth_src, 
> >> >> mask->eth_src);
> >> >> ^~~
> >> >> ../lib/odp-execute.c:62:37: error: taking address of packed member 
> >> >> 'eth_dst' of
> >> >> class or structure 'eth_header' may result in an unaligned pointer value
> >> >>   [-Werror,-Waddress-of-packed-member]
> >> >> ether_addr_copy_masked(>eth_dst, key->eth_dst, 
> >> >> mask->eth_dst);
> >> >>
> >> >> Ethernet source addresses are 48 bits offset into the Ethernet header,
> >> >> so taking a pointer for this is not guaranteed to be valid on all
> >> >> architectures. Fix this by referencing the memory direct from the
> >> >> Ethernet header pointer.
> >> >>
> >> >> Signed-off-by: Joe Stringer 
> >> >
> >> > I don't understand--why does Clang think that there's something packed
> >> > here?  I don't see any packed annotation on struct eth_header (and I
> >> > don't think it needs one).
> >>
> >> https://github.com/openvswitch/ovs/blob/master/lib/packets.h#L398
> >>
> >> I believe that this is the wire-formatted version that we use for
> >> assembling PDUs for protocols such as STP, so I think it needs to be
> >> properly packed.
> >
> > Oh, oops, somehow I was blind to the difference between eth_header and
> > eth_addr.
> >
> > But I don't actually see a need to pack this structure.  There's nothing
> > in it that would cause a compile to insert padding.  It has all 16-bit
> > members (including nested members), and you never get padding (on normal
> > architectures) when all the members of a struct have the same size;
> > otherwise arrays wouldn't work reasonably.
> >
> > I'll send some patches.
> 
> Ok thanks, I'll abandon this series and look for your patches.

I sent my patch (just one, I guess):
https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332864.html
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] odp-execute: Fix unaligned eth_addr pointers.

[Joe Stringer]

On 25 May 2017 at 13:35, Ben Pfaff  wrote:
> On Thu, May 25, 2017 at 01:05:11PM -0700, Joe Stringer wrote:
>> On 24 May 2017 at 20:02, Ben Pfaff  wrote:
>> > On Tue, May 23, 2017 at 04:02:12PM -0700, Joe Stringer wrote:
>> >> Clang 4.0 complains:
>> >>
>> >> ../lib/odp-execute.c:61:37: error: taking address of packed member 
>> >> 'eth_dst' of
>> >> class or structure 'eth_header' may result in an unaligned pointer value
>> >>   [-Werror,-Waddress-of-packed-member]
>> >> ether_addr_copy_masked(>eth_src, key->eth_src, 
>> >> mask->eth_src);
>> >> ^~~
>> >> ../lib/odp-execute.c:62:37: error: taking address of packed member 
>> >> 'eth_dst' of
>> >> class or structure 'eth_header' may result in an unaligned pointer value
>> >>   [-Werror,-Waddress-of-packed-member]
>> >> ether_addr_copy_masked(>eth_dst, key->eth_dst, 
>> >> mask->eth_dst);
>> >>
>> >> Ethernet source addresses are 48 bits offset into the Ethernet header,
>> >> so taking a pointer for this is not guaranteed to be valid on all
>> >> architectures. Fix this by referencing the memory direct from the
>> >> Ethernet header pointer.
>> >>
>> >> Signed-off-by: Joe Stringer 
>> >
>> > I don't understand--why does Clang think that there's something packed
>> > here?  I don't see any packed annotation on struct eth_header (and I
>> > don't think it needs one).
>>
>> https://github.com/openvswitch/ovs/blob/master/lib/packets.h#L398
>>
>> I believe that this is the wire-formatted version that we use for
>> assembling PDUs for protocols such as STP, so I think it needs to be
>> properly packed.
>
> Oh, oops, somehow I was blind to the difference between eth_header and
> eth_addr.
>
> But I don't actually see a need to pack this structure.  There's nothing
> in it that would cause a compile to insert padding.  It has all 16-bit
> members (including nested members), and you never get padding (on normal
> architectures) when all the members of a struct have the same size;
> otherwise arrays wouldn't work reasonably.
>
> I'll send some patches.

Ok thanks, I'll abandon this series and look for your patches.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] odp-execute: Fix unaligned eth_addr pointers.

[Ben Pfaff]

On Thu, May 25, 2017 at 01:05:11PM -0700, Joe Stringer wrote:
> On 24 May 2017 at 20:02, Ben Pfaff  wrote:
> > On Tue, May 23, 2017 at 04:02:12PM -0700, Joe Stringer wrote:
> >> Clang 4.0 complains:
> >>
> >> ../lib/odp-execute.c:61:37: error: taking address of packed member 
> >> 'eth_dst' of
> >> class or structure 'eth_header' may result in an unaligned pointer value
> >>   [-Werror,-Waddress-of-packed-member]
> >> ether_addr_copy_masked(>eth_src, key->eth_src, 
> >> mask->eth_src);
> >> ^~~
> >> ../lib/odp-execute.c:62:37: error: taking address of packed member 
> >> 'eth_dst' of
> >> class or structure 'eth_header' may result in an unaligned pointer value
> >>   [-Werror,-Waddress-of-packed-member]
> >> ether_addr_copy_masked(>eth_dst, key->eth_dst, 
> >> mask->eth_dst);
> >>
> >> Ethernet source addresses are 48 bits offset into the Ethernet header,
> >> so taking a pointer for this is not guaranteed to be valid on all
> >> architectures. Fix this by referencing the memory direct from the
> >> Ethernet header pointer.
> >>
> >> Signed-off-by: Joe Stringer 
> >
> > I don't understand--why does Clang think that there's something packed
> > here?  I don't see any packed annotation on struct eth_header (and I
> > don't think it needs one).
> 
> https://github.com/openvswitch/ovs/blob/master/lib/packets.h#L398
> 
> I believe that this is the wire-formatted version that we use for
> assembling PDUs for protocols such as STP, so I think it needs to be
> properly packed.

Oh, oops, somehow I was blind to the difference between eth_header and
eth_addr.

But I don't actually see a need to pack this structure.  There's nothing
in it that would cause a compile to insert padding.  It has all 16-bit
members (including nested members), and you never get padding (on normal
architectures) when all the members of a struct have the same size;
otherwise arrays wouldn't work reasonably.

I'll send some patches.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] odp-execute: Fix unaligned eth_addr pointers.

[Joe Stringer]

On 24 May 2017 at 20:02, Ben Pfaff  wrote:
> On Tue, May 23, 2017 at 04:02:12PM -0700, Joe Stringer wrote:
>> Clang 4.0 complains:
>>
>> ../lib/odp-execute.c:61:37: error: taking address of packed member 'eth_dst' 
>> of
>> class or structure 'eth_header' may result in an unaligned pointer value
>>   [-Werror,-Waddress-of-packed-member]
>> ether_addr_copy_masked(>eth_src, key->eth_src, 
>> mask->eth_src);
>> ^~~
>> ../lib/odp-execute.c:62:37: error: taking address of packed member 'eth_dst' 
>> of
>> class or structure 'eth_header' may result in an unaligned pointer value
>>   [-Werror,-Waddress-of-packed-member]
>> ether_addr_copy_masked(>eth_dst, key->eth_dst, 
>> mask->eth_dst);
>>
>> Ethernet source addresses are 48 bits offset into the Ethernet header,
>> so taking a pointer for this is not guaranteed to be valid on all
>> architectures. Fix this by referencing the memory direct from the
>> Ethernet header pointer.
>>
>> Signed-off-by: Joe Stringer 
>
> I don't understand--why does Clang think that there's something packed
> here?  I don't see any packed annotation on struct eth_header (and I
> don't think it needs one).

https://github.com/openvswitch/ovs/blob/master/lib/packets.h#L398

I believe that this is the wire-formatted version that we use for
assembling PDUs for protocols such as STP, so I think it needs to be
properly packed.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/5] odp-execute: Fix unaligned eth_addr pointers.

[Ben Pfaff]

On Tue, May 23, 2017 at 04:02:12PM -0700, Joe Stringer wrote:
> Clang 4.0 complains:
> 
> ../lib/odp-execute.c:61:37: error: taking address of packed member 'eth_dst' 
> of
> class or structure 'eth_header' may result in an unaligned pointer value
>   [-Werror,-Waddress-of-packed-member]
> ether_addr_copy_masked(>eth_src, key->eth_src, mask->eth_src);
> ^~~
> ../lib/odp-execute.c:62:37: error: taking address of packed member 'eth_dst' 
> of
> class or structure 'eth_header' may result in an unaligned pointer value
>   [-Werror,-Waddress-of-packed-member]
> ether_addr_copy_masked(>eth_dst, key->eth_dst, mask->eth_dst);
> 
> Ethernet source addresses are 48 bits offset into the Ethernet header,
> so taking a pointer for this is not guaranteed to be valid on all
> architectures. Fix this by referencing the memory direct from the
> Ethernet header pointer.
> 
> Signed-off-by: Joe Stringer 

I don't understand--why does Clang think that there's something packed
here?  I don't see any packed annotation on struct eth_header (and I
don't think it needs one).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/5] odp-execute: Fix unaligned eth_addr pointers.

[Joe Stringer]

Clang 4.0 complains:

../lib/odp-execute.c:61:37: error: taking address of packed member 'eth_dst' of
class or structure 'eth_header' may result in an unaligned pointer value
  [-Werror,-Waddress-of-packed-member]
ether_addr_copy_masked(>eth_src, key->eth_src, mask->eth_src);
^~~
../lib/odp-execute.c:62:37: error: taking address of packed member 'eth_dst' of
class or structure 'eth_header' may result in an unaligned pointer value
  [-Werror,-Waddress-of-packed-member]
ether_addr_copy_masked(>eth_dst, key->eth_dst, mask->eth_dst);

Ethernet source addresses are 48 bits offset into the Ethernet header,
so taking a pointer for this is not guaranteed to be valid on all
architectures. Fix this by referencing the memory direct from the
Ethernet header pointer.

Signed-off-by: Joe Stringer 
---
 lib/odp-execute.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 08bc50a46a2e..f0dadf56c422 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -48,6 +48,21 @@ ether_addr_copy_masked(struct eth_addr *dst, const struct 
eth_addr src,
 }
 
 static void
+ether_addrs_copy_masked(struct eth_header *hdr,
+const struct eth_addr src, const struct eth_addr smask,
+const struct eth_addr dst, const struct eth_addr dmask)
+{
+int i;
+
+for (i = 0; i < ARRAY_SIZE(src.be16); i++) {
+hdr->eth_src.be16[i] = src.be16[i]
+ | (hdr->eth_src.be16[i] & ~smask.be16[i]);
+hdr->eth_dst.be16[i] = dst.be16[i]
+ | (hdr->eth_dst.be16[i] & ~dmask.be16[i]);
+}
+}
+
+static void
 odp_eth_set_addrs(struct dp_packet *packet, const struct ovs_key_ethernet *key,
   const struct ovs_key_ethernet *mask)
 {
@@ -58,8 +73,8 @@ odp_eth_set_addrs(struct dp_packet *packet, const struct 
ovs_key_ethernet *key,
 eh->eth_src = key->eth_src;
 eh->eth_dst = key->eth_dst;
 } else {
-ether_addr_copy_masked(>eth_src, key->eth_src, mask->eth_src);
-ether_addr_copy_masked(>eth_dst, key->eth_dst, mask->eth_dst);
+ether_addrs_copy_masked(eh, key->eth_src, mask->eth_src,
+key->eth_dst, mask->eth_dst);
 }
 }
 }
-- 
2.12.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev