Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
On 12 August 2018 at 04:11, Andrew Oates wrote: > Ping --- would you like me to resubmit the patch using CONFIG_BSD? Yes, that seems our best option. Could you please also include a comment that summarises the behaviour of the other host OSes and why we're using this ifdef? thanks -- PMM
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
On Tue, Aug 14, 2018 at 11:52 AM Peter Maydell wrote: > On 12 August 2018 at 04:11, Andrew Oates wrote: > > Ping --- would you like me to resubmit the patch using CONFIG_BSD? > > Yes, that seems our best option. Could you please also include > a comment that summarises the behaviour of the other host OSes > and why we're using this ifdef? > Will do, thanks! > > thanks > -- PMM >
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
Ping --- would you like me to resubmit the patch using CONFIG_BSD? Cheers, ~Andrew On Wed, Aug 1, 2018 at 10:39 AM Andrew Oates wrote: > > > > On Wed, Aug 1, 2018 at 6:10 AM Peter Maydell > wrote: > >> On 1 August 2018 at 00:25, Andrew Oates wrote: >> > Both CONFIG_BSD and not-CONFIG_LINUX work on macOS. I unfortunately >> don't >> > have access to any other BSDs to test them, though. >> >> Is there an easy way to test it? The QEMU makefiles have some >> runes for setting up a BSD VM... >> > > Ok, it turns out that SOCK_DGRAM+IPPROTO_ICMP isn't actually supported on > FreeBSD---tested in qemu (thanks for the tip!). > > I didn't actually boot up NetBSD or OpenBSD, but poking around the kernel > source I found for them it appears they have the same restriction: > https://github.com/freebsd/freebsd/blob/master/sys/netinet/in_proto.c > https://github.com/openbsd/src/blob/master/sys/netinet/in_proto.c > > http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/netinet/in_proto.c?rev=1.128&content-type=text/x-cvsweb-markup > > (search for SOCK_DGRAM and IPPROTO_ICMP in the above). > > So I don't think ICMP via SLIRP would work at all in the other BSDs, > meaning this patch is a no-op for them either way. If we _were_ to make > SLIRP+ICMP work in *BSD, we'd presumably want to do it with a > SOCK_RAW+IPPROTO_ICMP socket (which would require special permissions to > create), which would mean an included IP header---so I think that #ifdef > CONFIG_BSD here is defensible. > > I did some poking for Solaris and Haiku and didn't find much, though it > looks like the version of ping included w/ Haiku uses SOCK_RAW rather than > SOCK_DGRAM, so it may be in the same boat as non-OSX-BSDs ( > https://github.com/haiku/haiku/blob/master/src/bin/network/ping/ping.c#L205 > ). > > ~Andrew > > >> >> thanks >> -- PMM >> >
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
On Wed, Aug 1, 2018 at 6:10 AM Peter Maydell wrote: > On 1 August 2018 at 00:25, Andrew Oates wrote: > > Both CONFIG_BSD and not-CONFIG_LINUX work on macOS. I unfortunately > don't > > have access to any other BSDs to test them, though. > > Is there an easy way to test it? The QEMU makefiles have some > runes for setting up a BSD VM... > Ok, it turns out that SOCK_DGRAM+IPPROTO_ICMP isn't actually supported on FreeBSD---tested in qemu (thanks for the tip!). I didn't actually boot up NetBSD or OpenBSD, but poking around the kernel source I found for them it appears they have the same restriction: https://github.com/freebsd/freebsd/blob/master/sys/netinet/in_proto.c https://github.com/openbsd/src/blob/master/sys/netinet/in_proto.c http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/netinet/in_proto.c?rev=1.128&content-type=text/x-cvsweb-markup (search for SOCK_DGRAM and IPPROTO_ICMP in the above). So I don't think ICMP via SLIRP would work at all in the other BSDs, meaning this patch is a no-op for them either way. If we _were_ to make SLIRP+ICMP work in *BSD, we'd presumably want to do it with a SOCK_RAW+IPPROTO_ICMP socket (which would require special permissions to create), which would mean an included IP header---so I think that #ifdef CONFIG_BSD here is defensible. I did some poking for Solaris and Haiku and didn't find much, though it looks like the version of ping included w/ Haiku uses SOCK_RAW rather than SOCK_DGRAM, so it may be in the same boat as non-OSX-BSDs ( https://github.com/haiku/haiku/blob/master/src/bin/network/ping/ping.c#L205 ). ~Andrew > > thanks > -- PMM >
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
On 1 August 2018 at 00:25, Andrew Oates wrote: > Both CONFIG_BSD and not-CONFIG_LINUX work on macOS. I unfortunately don't > have access to any other BSDs to test them, though. Is there an easy way to test it? The QEMU makefiles have some runes for setting up a BSD VM... thanks -- PMM
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
On Tue, Jul 31, 2018 at 6:22 AM Peter Maydell wrote: > On 31 July 2018 at 02:16, Andrew Oates wrote: > > Yeah, I suspect (but haven't tested) that this applies to all BSDs. We > > could switch CONFIG_DARWIN to CONFIG_BSD (happy to resend the patch, just > > LMK). > > > > Agreed that platform-specific ifdefs are gross, but I don't see a better > way > > here :/ One option would be to look at the packet length and contents to > > try to determine if there's an IP header before the ICMP header, but that > > seems pretty iffy. Creating ICMP sockets often requires special > privileges > > or configuration (e.g. on Linux) so I don't think it could easily be > done at > > configure-time to test the host machine's configuration. > > Mmm. I guess using CONFIG_BSD, or perhaps even not-CONFIG_LINUX, > would be best. Is there an easy way to test this? > (Our other two supported host OSes are the Solarises and Haiku; > no idea if either of those support ICMP sockets or what their > behaviour is here...) > Both CONFIG_BSD and not-CONFIG_LINUX work on macOS. I unfortunately don't have access to any other BSDs to test them, though. ~Andrew > > thanks > -- PMM >
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
On 31 July 2018 at 02:16, Andrew Oates wrote: > Yeah, I suspect (but haven't tested) that this applies to all BSDs. We > could switch CONFIG_DARWIN to CONFIG_BSD (happy to resend the patch, just > LMK). > > Agreed that platform-specific ifdefs are gross, but I don't see a better way > here :/ One option would be to look at the packet length and contents to > try to determine if there's an IP header before the ICMP header, but that > seems pretty iffy. Creating ICMP sockets often requires special privileges > or configuration (e.g. on Linux) so I don't think it could easily be done at > configure-time to test the host machine's configuration. Mmm. I guess using CONFIG_BSD, or perhaps even not-CONFIG_LINUX, would be best. Is there an easy way to test this? (Our other two supported host OSes are the Solarises and Haiku; no idea if either of those support ICMP sockets or what their behaviour is here...) thanks -- PMM
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
Yeah, I suspect (but haven't tested) that this applies to all BSDs. We
could switch CONFIG_DARWIN to CONFIG_BSD (happy to resend the patch, just
LMK).
Agreed that platform-specific ifdefs are gross, but I don't see a better
way here :/ One option would be to look at the packet length and contents
to try to determine if there's an IP header before the ICMP header, but
that seems pretty iffy. Creating ICMP sockets often requires special
privileges or configuration (e.g. on Linux) so I don't think it could
easily be done at configure-time to test the host machine's configuration.
~Andrew
On Mon, Jul 30, 2018 at 6:38 AM Peter Maydell
wrote:
> On 29 July 2018 at 15:35, Samuel Thibault
> wrote:
> > From: Andrew Oates
> >
> > On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when
> > read from. On macOS, however, the socket acts like a SOCK_RAW socket
> > and includes the IP header as well.
> >
> > This change strips the extra IP header from the received packet on macOS
> > before sending it to the guest.
> >
> > Signed-off-by: Andrew Oates
> > Signed-off-by: Samuel Thibault
> > ---
> > slirp/ip_icmp.c | 16 +++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> > index 0b667a429a..6316427ed3 100644
> > --- a/slirp/ip_icmp.c
> > +++ b/slirp/ip_icmp.c
> > @@ -420,7 +420,21 @@ void icmp_receive(struct socket *so)
> > icp = mtod(m, struct icmp *);
> >
> > id = icp->icmp_id;
> > -len = qemu_recv(so->s, icp, m->m_len, 0);
> > +len = qemu_recv(so->s, icp, M_ROOM(m), 0);
> > +#ifdef CONFIG_DARWIN
> > +if (len >= sizeof(struct ip)) {
> > +/* Skip the IP header that OS X (unlike Linux) includes. */
> > +struct ip *inner_ip = mtod(m, struct ip *);
> > +int inner_hlen = inner_ip->ip_hl << 2;
> > +if (inner_hlen > len) {
> > +len = -1;
> > +errno = -EINVAL;
> > +} else {
> > +len -= inner_hlen;
> > +memmove(icp, (unsigned char *)icp + inner_hlen, len);
> > +}
> > +}
> > +#endif
>
> I think it's generally preferable to avoid per-OS ifdefs -- is
> this really OSX specific and not (for instance) also applicable
> to the other BSDs? Is there some other (configure or runtime) check
> we could do to identify whether this is required?
>
> For instance the FreeBSD manpage for icmp(4)
>
> https://www.freebsd.org/cgi/man.cgi?query=icmp&apropos=0&sektion=0&manpath=FreeBSD+11.2-RELEASE&arch=default&format=html
>
> says "incoming packets are received with the IP header and
> options intact" and I would be unsurprised to find that
> all the BSDs behave the same way here.
>
> thanks
> -- PMM
>
Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts
On 29 July 2018 at 15:35, Samuel Thibault wrote:
> From: Andrew Oates
>
> On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when
> read from. On macOS, however, the socket acts like a SOCK_RAW socket
> and includes the IP header as well.
>
> This change strips the extra IP header from the received packet on macOS
> before sending it to the guest.
>
> Signed-off-by: Andrew Oates
> Signed-off-by: Samuel Thibault
> ---
> slirp/ip_icmp.c | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> index 0b667a429a..6316427ed3 100644
> --- a/slirp/ip_icmp.c
> +++ b/slirp/ip_icmp.c
> @@ -420,7 +420,21 @@ void icmp_receive(struct socket *so)
> icp = mtod(m, struct icmp *);
>
> id = icp->icmp_id;
> -len = qemu_recv(so->s, icp, m->m_len, 0);
> +len = qemu_recv(so->s, icp, M_ROOM(m), 0);
> +#ifdef CONFIG_DARWIN
> +if (len >= sizeof(struct ip)) {
> +/* Skip the IP header that OS X (unlike Linux) includes. */
> +struct ip *inner_ip = mtod(m, struct ip *);
> +int inner_hlen = inner_ip->ip_hl << 2;
> +if (inner_hlen > len) {
> +len = -1;
> +errno = -EINVAL;
> +} else {
> +len -= inner_hlen;
> +memmove(icp, (unsigned char *)icp + inner_hlen, len);
> +}
> +}
> +#endif
I think it's generally preferable to avoid per-OS ifdefs -- is
this really OSX specific and not (for instance) also applicable
to the other BSDs? Is there some other (configure or runtime) check
we could do to identify whether this is required?
For instance the FreeBSD manpage for icmp(4)
https://www.freebsd.org/cgi/man.cgi?query=icmp&apropos=0&sektion=0&manpath=FreeBSD+11.2-RELEASE&arch=default&format=html
says "incoming packets are received with the IP header and
options intact" and I would be unsurprised to find that
all the BSDs behave the same way here.
thanks
-- PMM
