| Message ID | 20180108172904.8772-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | add HOST_SUPPORTS_UNALIGNED_ACCESS, reduce slirp warnings | expand |
On 08.01.2018 18:28, Philippe Mathieu-Daudé wrote:
> theses structures are not serialized and often store host pointers.
Patch looks fine at a quick glance... did you check that migration still
works (while there is network traffic going on in the guest)?
Thomas
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
diff --git a/slirp/ip.h b/slirp/ip.h index 1df6723357..e29ccd3c9f 100644 --- a/slirp/ip.h +++ b/slirp/ip.h @@ -179,11 +179,11 @@ struct ip_timestamp { struct mbuf_ptr { struct mbuf *mptr; uint32_t dummy; -} QEMU_PACKED; +}; #else struct mbuf_ptr { struct mbuf *mptr; -} QEMU_PACKED; +}; #endif struct qlink { void *next, *prev; @@ -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; +}; /* * Ip reassembly queue structure. Each fragment @@ -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; +}; /* * Ip header, when holding a fragment. @@ -225,7 +225,7 @@ struct ipq { struct ipasfrag { struct qlink ipf_link; struct ip ipf_ip; -} QEMU_PACKED; +}; #define ipf_off ipf_ip.ip_off #define ipf_tos ipf_ip.ip_tos diff --git a/slirp/slirp.h b/slirp/slirp.h index 898ec9516d..9f29b52610 100644 --- a/slirp/slirp.h +++ b/slirp/slirp.h @@ -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; +}; #define NDP_TABLE_SIZE 16
theses structures are not serialized and often store host pointers. slirp/ip_input.c:159:43: warning: taking address of packed member 'ip_link' of class or structure 'ipq' may result in an unaligned pointer value [-Waddress-of-packed-member] for (l = slirp->ipq.ip_link.next; l != &slirp->ipq.ip_link; ^~~~~~~~~~~~~~~~~~ Reported-by: John Arbuckle <programmingkidx@gmail.com> Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- slirp/ip.h | 10 +++++----- slirp/slirp.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-)