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 <address@hidden>
wrote:

> On 29 July 2018 at 15:35, Samuel Thibault <address@hidden>
> wrote:
> > From: Andrew Oates <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Samuel Thibault <address@hidden>
> > ---
> >  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
>