[Qemu-devel] [PATCH 0/2] slirp: fix -Waddress-of-packed-member warnings

[Qemu-devel] [PATCH 0/2] slirp: fix -Waddress-of-packed-member warnings

[Peter Maydell]

These patches fix the clang -Waddress-of-packed-member warnings
in the slirp code. For slirp, the structs marked as packed
are already naturally packed/aligned, so adding the 'packed'
attribute doesn't change their layout but means the compiler
has to assume they might be misaligned. Instead, for the structs
which map to on-the-wire packet formats, we add build time
assertions that they have the sizes we think they should.

The 'struct ipq' and 'struct ipasfrag' aren't on-the-wire
packet representations, so it's not clear why they were
marked packed in the first place. (They contain host pointers
and so have a size that depends on the host.) Here we just
assert the only thing that's documented as a layout restriction:
that struct ipq's frag_link and struct ipasfrag's ipf_link
are at the same offset.

Together, this fixes all the -Waddress-of-packed-member warnings
in slirp, which I think was the only set remaining without
patches at least being on-list. (Some of the block/ warning fixes
are still working their way through the relevant trees.)

thanks
-- PMM

Peter Maydell (2):
  slirp: Avoid marking naturally packed structs as QEMU_PACKED
  slirp: Don't mark struct ipq or struct ipasfrag as packed

 slirp/ip.h       |  7 +++++--
 slirp/ip6.h      | 12 ++++++++++--
 slirp/ip6_icmp.h | 20 +++++++++++++++-----
 3 files changed, 30 insertions(+), 9 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] slirp: Avoid marking naturally packed structs as QEMU_PACKED

[Peter Maydell]

Various ipv6 structs in the slirp headers are marked QEMU_PACKED,
but they are actually naturally aligned and will have no padding
in them. Instead of marking them with the 'packed' attribute,
assert at compile time that they are the size we expect. This
allows us to take the address of fields within the structs
without risking undefined behaviour, and suppresses clang
-Waddress-of-packed-member warnings.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/ip6.h      | 12 ++++++++++--
 slirp/ip6_icmp.h | 20 +++++++++++++++-----
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/slirp/ip6.h b/slirp/ip6.h
index 14e9c787357..1e3e329ce6a 100644
--- a/slirp/ip6.h
+++ b/slirp/ip6.h
@@ -133,7 +133,7 @@ struct ip6 {
     uint8_t     ip_nh;               /* next header */
     uint8_t     ip_hl;               /* hop limit */
     struct in6_addr ip_src, ip_dst;  /* source and dest address */
-} QEMU_PACKED;
+};
 
 /*
  * IPv6 pseudo-header used by upper-layer protocols
@@ -145,7 +145,15 @@ struct ip6_pseudohdr {
     uint16_t    ih_zero_hi;       /* zero */
     uint8_t     ih_zero_lo;       /* zero */
     uint8_t     ih_nh;            /* next header */
-} QEMU_PACKED;
+};
 
+/*
+ * We don't want to mark these ip6 structs as packed as they are naturally
+ * correctly aligned; instead assert that there is no stray padding.
+ * If we marked the struct as packed then we would be unable to take
+ * the address of any of the fields in it.
+ */
+QEMU_BUILD_BUG_ON(sizeof(struct ip6) != 40);
+QEMU_BUILD_BUG_ON(sizeof(struct ip6_pseudohdr) != 40);
 
 #endif
diff --git a/slirp/ip6_icmp.h b/slirp/ip6_icmp.h
index 32b0914055a..2ad2b75e676 100644
--- a/slirp/ip6_icmp.h
+++ b/slirp/ip6_icmp.h
@@ -48,12 +48,16 @@ struct ndp_ra {     /* Router Advertisement Message */
     uint16_t lifetime;      /* Router Lifetime */
     uint32_t reach_time;    /* Reachable Time */
     uint32_t retrans_time;  /* Retrans Timer */
-} QEMU_PACKED;
+};
+
+QEMU_BUILD_BUG_ON(sizeof(struct ndp_ra) != 12);
 
 struct ndp_ns {     /* Neighbor Solicitation Message */
     uint32_t reserved;
     struct in6_addr target; /* Target Address */
-} QEMU_PACKED;
+};
+
+QEMU_BUILD_BUG_ON(sizeof(struct ndp_ns) != 20);
 
 struct ndp_na {     /* Neighbor Advertisement Message */
 #if G_BYTE_ORDER == G_BIG_ENDIAN
@@ -72,13 +76,17 @@ struct ndp_na {     /* Neighbor Advertisement Message */
         reserved_lo:24;
 #endif
     struct in6_addr target; /* Target Address */
-} QEMU_PACKED;
+};
+
+QEMU_BUILD_BUG_ON(sizeof(struct ndp_na) != 20);
 
 struct ndp_redirect {
     uint32_t reserved;
     struct in6_addr target; /* Target Address */
     struct in6_addr dest;   /* Destination Address */
-} QEMU_PACKED;
+};
+
+QEMU_BUILD_BUG_ON(sizeof(struct ndp_redirect) != 36);
 
 /*
  * Structure of an icmpv6 header.
@@ -103,7 +111,9 @@ struct icmp6 {
 #define icmp6_nns icmp6_body.ndp_ns
 #define icmp6_nna icmp6_body.ndp_na
 #define icmp6_redirect icmp6_body.ndp_redirect
-} QEMU_PACKED;
+};
+
+QEMU_BUILD_BUG_ON(sizeof(struct icmp6) != 40);
 
 #define ICMP6_MINLEN    4
 #define ICMP6_ERROR_MINLEN  8
-- 
2.20.1

[Qemu-devel] [PATCH 2/2] slirp: Don't mark struct ipq or struct ipasfrag as packed

[Peter Maydell]

There is no reason to mark the struct ipq and struct ipasfrag as
packed: they are naturally aligned anyway, and are not representing
any on-the-wire packet format.  Indeed they vary in size depending on
the size of pointers on the host system, because the 'struct qlink'
members include 'void *' fields.

Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member
warnings and probably lets the compiler generate better code too.

The only thing we do care about in the layout of the struct is
that the frag_link matches up with the ipf_link of the struct
ipasfrag, as documented in the comment on that struct; assert
at build time that this is the case.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/ip.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/slirp/ip.h b/slirp/ip.h
index 243b6c8b249..20614f3b53e 100644
--- a/slirp/ip.h
+++ b/slirp/ip.h
@@ -217,7 +217,7 @@ struct ipq {
 	uint8_t	ipq_p;			/* protocol of this fragment */
 	uint16_t	ipq_id;			/* sequence id for reassembly */
 	struct	in_addr ipq_src,ipq_dst;
-} QEMU_PACKED;
+};
 
 /*
  * Ip header, when holding a fragment.
@@ -227,7 +227,10 @@ struct ipq {
 struct	ipasfrag {
 	struct qlink ipf_link;
 	struct ip ipf_ip;
-} QEMU_PACKED;
+};
+
+QEMU_BUILD_BUG_ON(offsetof(struct ipq, frag_link) !=
+                  offsetof(struct ipasfrag, ipf_link));
 
 #define ipf_off      ipf_ip.ip_off
 #define ipf_tos      ipf_ip.ip_tos
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/2] slirp: Don't mark struct ipq or struct ipasfrag as packed

[Daniel P. Berrangé]

On Tue, Jan 22, 2019 at 06:18:22PM +0000, Peter Maydell wrote:
> There is no reason to mark the struct ipq and struct ipasfrag as
> packed: they are naturally aligned anyway, and are not representing
> any on-the-wire packet format.  Indeed they vary in size depending on
> the size of pointers on the host system, because the 'struct qlink'
> members include 'void *' fields.
> 
> Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member
> warnings and probably lets the compiler generate better code too.
> 
> The only thing we do care about in the layout of the struct is
> that the frag_link matches up with the ipf_link of the struct
> ipasfrag, as documented in the comment on that struct; assert
> at build time that this is the case.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  slirp/ip.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/slirp/ip.h b/slirp/ip.h
> index 243b6c8b249..20614f3b53e 100644
> --- a/slirp/ip.h
> +++ b/slirp/ip.h

Just here there's a misleading comment

  * size 28 bytes

that should be purged

> @@ -217,7 +217,7 @@ struct ipq {
>  	uint8_t	ipq_p;			/* protocol of this fragment */
>  	uint16_t	ipq_id;			/* sequence id for reassembly */
>  	struct	in_addr ipq_src,ipq_dst;
> -} QEMU_PACKED;
> +};
>  
>  /*
>   * Ip header, when holding a fragment.
> @@ -227,7 +227,10 @@ struct ipq {
>  struct	ipasfrag {
>  	struct qlink ipf_link;
>  	struct ip ipf_ip;
> -} QEMU_PACKED;
> +};
> +
> +QEMU_BUILD_BUG_ON(offsetof(struct ipq, frag_link) !=
> +                  offsetof(struct ipasfrag, ipf_link));
>  
>  #define ipf_off      ipf_ip.ip_off
>  #define ipf_tos      ipf_ip.ip_tos


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 2/2] slirp: Don't mark struct ipq or struct ipasfrag as packed

[Peter Maydell]

On Tue, 22 Jan 2019 at 18:55, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Jan 22, 2019 at 06:18:22PM +0000, Peter Maydell wrote:
> > There is no reason to mark the struct ipq and struct ipasfrag as
> > packed: they are naturally aligned anyway, and are not representing
> > any on-the-wire packet format.  Indeed they vary in size depending on
> > the size of pointers on the host system, because the 'struct qlink'
> > members include 'void *' fields.
> >
> > Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member
> > warnings and probably lets the compiler generate better code too.
> >
> > The only thing we do care about in the layout of the struct is
> > that the frag_link matches up with the ipf_link of the struct
> > ipasfrag, as documented in the comment on that struct; assert
> > at build time that this is the case.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  slirp/ip.h | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/slirp/ip.h b/slirp/ip.h
> > index 243b6c8b249..20614f3b53e 100644
> > --- a/slirp/ip.h
> > +++ b/slirp/ip.h
>
> Just here there's a misleading comment
>
>   * size 28 bytes
>
> that should be purged

I noticed that one but left it alone on the basis that I
wasn't sure what to replace it with, and as it is it
provides the useful information of "somebody at some point
thought the size of this struct mattered but they were
badly confused, so tread with caution" :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] slirp: Avoid marking naturally packed structs as QEMU_PACKED

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On 1/22/19 12:18 PM, Peter Maydell wrote:
> Various ipv6 structs in the slirp headers are marked QEMU_PACKED,
> but they are actually naturally aligned and will have no padding
> in them. Instead of marking them with the 'packed' attribute,
> assert at compile time that they are the size we expect. This
> allows us to take the address of fields within the structs
> without risking undefined behaviour, and suppresses clang
> -Waddress-of-packed-member warnings.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  slirp/ip6.h      | 12 ++++++++++--
>  slirp/ip6_icmp.h | 20 +++++++++++++++-----
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 

> +/*
> + * We don't want to mark these ip6 structs as packed as they are naturally
> + * correctly aligned; instead assert that there is no stray padding.
> + * If we marked the struct as packed then we would be unable to take
> + * the address of any of the fields in it.
> + */
> +QEMU_BUILD_BUG_ON(sizeof(struct ip6) != 40);
> +QEMU_BUILD_BUG_ON(sizeof(struct ip6_pseudohdr) != 40);

Nice.  (And IIRC, the IPv6 designers tried to pick natural alignments on
purpose)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/2] slirp: Don't mark struct ipq or struct ipasfrag as packed

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On 1/22/19 12:18 PM, Peter Maydell wrote:
> There is no reason to mark the struct ipq and struct ipasfrag as
> packed: they are naturally aligned anyway, and are not representing
> any on-the-wire packet format.  Indeed they vary in size depending on
> the size of pointers on the host system, because the 'struct qlink'
> members include 'void *' fields.
> 
> Dropping the 'packed' annotation fixes clang -Waddress-of-packed-member
> warnings and probably lets the compiler generate better code too.
> 
> The only thing we do care about in the layout of the struct is
> that the frag_link matches up with the ipf_link of the struct
> ipasfrag, as documented in the comment on that struct; assert
> at build time that this is the case.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  slirp/ip.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/2] slirp: fix -Waddress-of-packed-member warnings

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On 1/22/19 12:18 PM, Peter Maydell wrote:
> These patches fix the clang -Waddress-of-packed-member warnings
> in the slirp code. For slirp, the structs marked as packed
> are already naturally packed/aligned, so adding the 'packed'
> attribute doesn't change their layout but means the compiler
> has to assume they might be misaligned. Instead, for the structs
> which map to on-the-wire packet formats, we add build time
> assertions that they have the sizes we think they should.
> 
> The 'struct ipq' and 'struct ipasfrag' aren't on-the-wire
> packet representations, so it's not clear why they were
> marked packed in the first place. (They contain host pointers
> and so have a size that depends on the host.) Here we just
> assert the only thing that's documented as a layout restriction:
> that struct ipq's frag_link and struct ipasfrag's ipf_link
> are at the same offset.
> 
> Together, this fixes all the -Waddress-of-packed-member warnings
> in slirp, which I think was the only set remaining without
> patches at least being on-list. (Some of the block/ warning fixes
> are still working their way through the relevant trees.)

How does this series play with the ongoing effort to make libslirp an
independent project?  I'm assuming your use of QEMU_BUILD_BUG_ON() will
have to be translated into whatever libslirp-specific mechanism we want
to split off into that project.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/2] slirp: fix -Waddress-of-packed-member warnings

[Samuel Thibault]

Hello,

Peter Maydell, le mar. 22 janv. 2019 18:18:20 +0000, a ecrit:
> These patches fix the clang -Waddress-of-packed-member warnings
> in the slirp code.

Applied to my tree, thanks!

Eric Blake, le mar. 22 janv. 2019 13:27:32 -0600, a ecrit:
> How does this series play with the ongoing effort to make libslirp an
> independent project?  I'm assuming your use of QEMU_BUILD_BUG_ON() will
> have to be translated into whatever libslirp-specific mechanism we want
> to split off into that project.

Indeed, but I guess libslirp will want to have such kind of macro
anyway, so I don't see a reason to hold this change.

Samuel