Re: [Qemu-devel] [PATCH 01/12] slirp: remove QEMU_PACKED from structures with don't require it

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, on lun. 08 janv. 2018 14:28:53 -0300, wrote:
>  struct mbuf_ptr {
>       struct mbuf *mptr;
>       uint32_t dummy;
> -} QEMU_PACKED;
> +};
>  #else
>  struct mbuf_ptr {
>       struct mbuf *mptr;
> -} QEMU_PACKED;
> +};

> @@ -199,7 +199,7 @@ struct ipovly {
>       uint16_t        ih_len;                 /* protocol length */
>       struct  in_addr ih_src;         /* source internet address */
>       struct  in_addr ih_dst;         /* destination internet address */
> -} QEMU_PACKED;
> +};

Did you actually try to change these structures?  The presence of the
"dummy" field should have hinted that it's not a trivial structure :)

mbuf_ptr is used in struct ipovly which is to have the same size as the
ipv4 header.  One would have to untangle that before removing the packed
attribute.

> @@ -215,7 +215,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;
> +};

This one seems safe to me. I'd still rather see an actual test with
added holes in the structure to be sure :)

> @@ -225,7 +225,7 @@ struct ipq {
>  struct       ipasfrag {
>       struct qlink ipf_link;
>       struct ip ipf_ip;
> -} QEMU_PACKED;
> +};

Please see iptofrag and fragtoip in ip_input.c, they assume that ipf_ip
immediately follows ipf_link.  I guess using offsetof there should avoid
the issue.  Note however that slirp assumes that in a 32bit-aligned
ethernet frame it has enough room before the IP header to stuff the
ipf_link things.  We could add a build-time check that offsetof(ipf_ip)
<= 2 + ETH_HLEN

> @@ -136,7 +136,7 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>  struct ndpentry {
>      unsigned char   eth_addr[ETH_ALEN];     /* sender hardware address */
>      struct in6_addr ip_addr;                /* sender IP address       */
> -} QEMU_PACKED;
> +};

This one should be safe.

Samuel