[Qemu-devel] [PULL 0/1] slirp fix for qemu 3.0

[Qemu-devel] [PULL 0/1] slirp fix for qemu 3.0

[Samuel Thibault]

The following changes since commit 18a398f6a39df4b08ff86ac0d38384193ca5f4cc:

  Update version for v3.0.0-rc2 release (2018-07-24 22:06:31 +0100)

are available in the Git repository at:

  https://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to 176e8672834d2038e744d7f230c3d75ecfec66aa:

  slirp: fix ICMP handling on macOS hosts (2018-07-29 16:28:58 +0200)

----------------------------------------------------------------
slirp fixes

Andrew Oates (1):
  slirp: fix ICMP handling on macOS hosts

----------------------------------------------------------------
Andrew Oates (1):
      slirp: fix ICMP handling on macOS hosts

 slirp/ip_icmp.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

[Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts

[Samuel Thibault]

From: Andrew Oates <aoates@google.com>

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 <aoates@google.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 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
     icp->icmp_id = id;
 
     m->m_data -= hlen;
-- 
2.18.0

Re: [Qemu-devel] [PULL 1/1] slirp: fix ICMP handling on macOS hosts

[Peter Maydell]

On 29 July 2018 at 15:35, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> From: Andrew Oates <aoates@google.com>
>
> 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 <aoates@google.com>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  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

[Andrew Oates]

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 <peter.maydell@linaro.org>
wrote:

> On 29 July 2018 at 15:35, Samuel Thibault <samuel.thibault@ens-lyon.org>
> wrote:
> > From: Andrew Oates <aoates@google.com>
> >
> > 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 <aoates@google.com>
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > ---
> >  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

[Peter Maydell]

On 31 July 2018 at 02:16, Andrew Oates <aoates@google.com> 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

[Andrew Oates]

On Tue, Jul 31, 2018 at 6:22 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 31 July 2018 at 02:16, Andrew Oates <aoates@google.com> 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

[Peter Maydell]

On 1 August 2018 at 00:25, Andrew Oates <aoates@google.com> 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

[Andrew Oates]

On Wed, Aug 1, 2018 at 6:10 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 1 August 2018 at 00:25, Andrew Oates <aoates@google.com> 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

[Andrew Oates]

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 <aoates@google.com> wrote:

>
>
>
> On Wed, Aug 1, 2018 at 6:10 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On 1 August 2018 at 00:25, Andrew Oates <aoates@google.com> 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

[Peter Maydell]

On 12 August 2018 at 04:11, Andrew Oates <aoates@google.com> 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

[Andrew Oates]

On Tue, Aug 14, 2018 at 11:52 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 12 August 2018 at 04:11, Andrew Oates <aoates@google.com> 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
>