Re: [Qemu-devel] [PATCHv7 3/9] slirp: Adding IPv6 UDP support

[Thomas Huth <thuth@redhat.com>]

On 08.02.2016 11:28, Samuel Thibault wrote:
> From: Guillaume Subiron <maethor@subiron.org>
> 
> This adds the sin6 case in the fhost and lhost unions and related macros.
> It adds udp6_input() and udp6_output().
> It adds the IPv6 case in sorecvfrom().
> Finally, udp_input() is called by ip6_input().
> 
> Signed-off-by: Guillaume Subiron <maethor@subiron.org>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
...
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 32b1ba3..b79ddec 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -541,7 +541,12 @@ sorecvfrom(struct socket *so)
>  	                   (struct sockaddr_in *) &daddr,
>  	                   so->so_iptos);
>  	        break;
> +	    case AF_INET6:
> +	        udp6_output(so, m, (struct sockaddr_in6 *) &saddr,
> +	                    (struct sockaddr_in6 *) &daddr);
> +	        break;
>  	    default:
> +	        g_assert_not_reached();

Could this be triggered by the guest? If so, I'd like to suggest to use
qemu_log_mask(LOG_GUEST_ERROR, ...) instead, since a guest should not be
able to terminate QEMU like this.

>  	        break;
>  	    }
>  	  } /* rx error */
...
> diff --git a/slirp/udp6.c b/slirp/udp6.c
> new file mode 100644
> index 0000000..63d6a8c
> --- /dev/null
> +++ b/slirp/udp6.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright (c) 2013
> + * Guillaume Subiron
> + *
> + * Please read the file COPYRIGHT for the
> + * terms and conditions of the copyright.
> + */
> +
> +#include "slirp.h"
> +#include "udp.h"
> +
> +void udp6_input(struct mbuf *m)
> +{
> +    Slirp *slirp = m->slirp;
> +    struct ip6 *ip, save_ip;
> +    struct udphdr *uh;
> +    int hlen = sizeof(struct ip6);
> +    int len;
> +    struct socket *so;
> +    struct sockaddr_in6 lhost;
> +
> +    DEBUG_CALL("udp6_input");
> +    DEBUG_ARG("m = %lx", (long)m);
> +
> +    if (slirp->restricted) {
> +        goto bad;
> +    }
> +
> +    ip = mtod(m, struct ip6 *);
> +    m->m_len -= hlen;
> +    m->m_data += hlen;
> +    uh = mtod(m, struct udphdr *);
> +    m->m_len += hlen;
> +    m->m_data -= hlen;
> +
> +    if (ip6_cksum(m)) {
> +        goto bad;
> +    }
> +
> +    len = ntohs((uint16_t)uh->uh_ulen);
> +
> +    /*
> +     * Make mbuf data length reflect UDP length.
> +     * If not enough data to reflect UDP length, drop.
> +     */
> +    if (ntohs(ip->ip_pl) != len) {
> +        if (len > ntohs(ip->ip_pl)) {
> +            goto bad;
> +        }
> +        m_adj(m, len - ntohs(ip->ip_pl));
> +        ip->ip_pl = htons(len);
> +    }
> +
> +    /* TODO handle DHCP/BOOTP */
> +    /* TODO handle TFTP */
> +
> +    /* Locate pcb for datagram. */
> +    lhost.sin6_family = AF_INET6;
> +    lhost.sin6_addr = ip->ip_src;
> +    lhost.sin6_port = uh->uh_sport;
> +
> +    so = solookup(&slirp->udp_last_so, &slirp->udb,
> +                  (struct sockaddr_storage *) &lhost, NULL);
> +
> +    if (so == NULL) {
> +        /* If there's no socket for this packet, create one. */
> +        so = socreate(slirp);
> +        if (!so) {
> +            goto bad;
> +        }
> +        if (udp_attach(so, AF_INET6) == -1) {
> +            DEBUG_MISC((dfd, " udp6_attach errno = %d-%s\n",
> +                        errno, strerror(errno)));
> +            sofree(so);
> +            goto bad;
> +        }
> +
> +        /* Setup fields */
> +        so->so_lfamily = AF_INET6;
> +        so->so_laddr6 = ip->ip_src;
> +        so->so_lport6 = uh->uh_sport;
> +    }
> +
> +    so->so_ffamily = AF_INET6;
> +    so->so_faddr6 = ip->ip_dst; /* XXX */
> +    so->so_fport6 = uh->uh_dport; /* XXX */

Why use the XXXs here? Some additional words in the comments would be
nice...

> +    hlen += sizeof(struct udphdr);
> +    m->m_len -= hlen;
> +    m->m_data += hlen;
> +
> +    /*
> +     * Now we sendto() the packet.
> +     */
> +    if (sosendto(so, m) == -1) {
> +        m->m_len += hlen;
> +        m->m_data -= hlen;
> +        *ip = save_ip;

It's getting late already and maybe I should stop reviewing ... but ...
using save_ip here looks bogus to me. Is this right, or just a
copy-n-paste error from the udpv4 code? Where is save_ip initialized?

> +        DEBUG_MISC((dfd, "udp tx errno = %d-%s\n", errno, strerror(errno)));
> +        /* TODO: ICMPv6 error */
> +        /*icmp_error(m, ICMP_UNREACH,ICMP_UNREACH_NET, 0,strerror(errno));*/
> +        goto bad;
> +    }
> +
> +    m_free(so->so_m);   /* used for ICMP if error on sorecvfrom */
> +
> +    /* restore the orig mbuf packet */
> +    m->m_len += hlen;
> +    m->m_data -= hlen;
> +    *ip = save_ip;

dito.

> +    so->so_m = m;
> +
> +    return;
> +bad:
> +    m_free(m);
> +}
> +
> +int udp6_output(struct socket *so, struct mbuf *m,
> +        struct sockaddr_in6 *saddr, struct sockaddr_in6 *daddr)
> +{
> +    struct ip6 *ip;
> +    struct udphdr *uh;
> +
> +    DEBUG_CALL("udp6_output");
> +    DEBUG_ARG("so = %lx", (long)so);
> +    DEBUG_ARG("m = %lx", (long)m);
> +
> +    /* adjust for header */
> +    m->m_data -= sizeof(struct udphdr);
> +    m->m_len += sizeof(struct udphdr);
> +    uh = mtod(m, struct udphdr *);
> +    m->m_data -= sizeof(struct ip6);
> +    m->m_len += sizeof(struct ip6);
> +    ip = mtod(m, struct ip6 *);
> +
> +    /* Build IP header */
> +    ip->ip_pl = htons(m->m_len - sizeof(struct ip6));
> +    ip->ip_nh = IPPROTO_UDP;
> +    ip->ip_src = saddr->sin6_addr;
> +    ip->ip_dst = daddr->sin6_addr;
> +
> +    /* Build UDP header */
> +    uh->uh_sport = saddr->sin6_port;
> +    uh->uh_dport = daddr->sin6_port;
> +    uh->uh_ulen = ip->ip_pl;
> +    uh->uh_sum = 0;
> +    uh->uh_sum = ip6_cksum(m);

I think you're missing the check for uh_sum = 0.

According to RFC768:

"If the computed  checksum  is zero,  it is transmitted  as all ones"

And according to RFC2460:

"whenever originating a UDP packet, an IPv6 node must compute a UDP
 checksum over the packet and the pseudo-header, and, if that
 computation yields a result of zero, it must be changed to hex
 FFFF for placement in the UDP header."

This is already done in udp.c, so you should also do this in udp6.c, I
think.

> +    return ip6_output(so, m, 0);
> +}
> 

 Thomas