| Message ID | 20170523230216.29696-2-joe@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
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(&eh->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(&eh->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 <joe@ovn.org> 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).
On 24 May 2017 at 20:02, Ben Pfaff <blp@ovn.org> 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(&eh->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(&eh->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 <joe@ovn.org> > > 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.
On Thu, May 25, 2017 at 01:05:11PM -0700, Joe Stringer wrote: > On 24 May 2017 at 20:02, Ben Pfaff <blp@ovn.org> 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(&eh->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(&eh->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 <joe@ovn.org> > > > > 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.
On 25 May 2017 at 13:35, Ben Pfaff <blp@ovn.org> wrote: > On Thu, May 25, 2017 at 01:05:11PM -0700, Joe Stringer wrote: >> On 24 May 2017 at 20:02, Ben Pfaff <blp@ovn.org> 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(&eh->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(&eh->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 <joe@ovn.org> >> > >> > 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.
On Thu, May 25, 2017 at 01:52:16PM -0700, Joe Stringer wrote: > On 25 May 2017 at 13:35, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, May 25, 2017 at 01:05:11PM -0700, Joe Stringer wrote: > >> On 24 May 2017 at 20:02, Ben Pfaff <blp@ovn.org> 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(&eh->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(&eh->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 <joe@ovn.org> > >> > > >> > 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
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(&eh->eth_src, key->eth_src, mask->eth_src); - ether_addr_copy_masked(&eh->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); } } }
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(&eh->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(&eh->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 <joe@ovn.org> --- lib/odp-execute.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)