[Qemu-devel] [PATCH 0/2] Fix spurious RDNSS announce

[Qemu-devel] [PATCH 0/2] Fix spurious RDNSS announce

[Samuel Thibault]

Hello,

These two patches fix sending the IPv6 RDNSS announce only when
it can actually work, fixing the bug reported by Cole Robinson
<crobinso@redhat.com>.  Could somebody review them before I can send
a pull update?

Samuel Thibault (2):
  slirp: Make RA build more flexible
  slirp: Send RDNSS in RA only if host has an IPv6 DNS server

 slirp/ip6_icmp.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/2] slirp: Make RA build more flexible

[Samuel Thibault]

Do not hardcode the RA size at all, use a pl_size variable which
accounts the accumulated size, and fill rip->ip_pl at the end.

This will allow to make some blocks optional.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 slirp/ip6_icmp.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index 298a48dd25..d0f5cc1456 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -143,17 +143,10 @@ void ndp_send_ra(Slirp *slirp)
     /* Build IPv6 packet */
     struct mbuf *t = m_get(slirp);
     struct ip6 *rip = mtod(t, struct ip6 *);
+    size_t pl_size = 0;
     rip->ip_src = (struct in6_addr)LINKLOCAL_ADDR;
     rip->ip_dst = (struct in6_addr)ALLNODES_MULTICAST;
     rip->ip_nh = IPPROTO_ICMPV6;
-    rip->ip_pl = htons(ICMP6_NDP_RA_MINLEN
-                        + NDPOPT_LINKLAYER_LEN
-                        + NDPOPT_PREFIXINFO_LEN
-#ifndef _WIN32
-                        + NDPOPT_RDNSS_LEN
-#endif
-                        );
-    t->m_len = sizeof(struct ip6) + ntohs(rip->ip_pl);
 
     /* Build ICMPv6 packet */
     t->m_data += sizeof(struct ip6);
@@ -171,6 +164,7 @@ void ndp_send_ra(Slirp *slirp)
     ricmp->icmp6_nra.reach_time = htonl(NDP_AdvReachableTime);
     ricmp->icmp6_nra.retrans_time = htonl(NDP_AdvRetransTime);
     t->m_data += ICMP6_NDP_RA_MINLEN;
+    pl_size += ICMP6_NDP_RA_MINLEN;
 
     /* Source link-layer address (NDP option) */
     struct ndpopt *opt = mtod(t, struct ndpopt *);
@@ -178,6 +172,7 @@ void ndp_send_ra(Slirp *slirp)
     opt->ndpopt_len = NDPOPT_LINKLAYER_LEN / 8;
     in6_compute_ethaddr(rip->ip_src, opt->ndpopt_linklayer);
     t->m_data += NDPOPT_LINKLAYER_LEN;
+    pl_size += NDPOPT_LINKLAYER_LEN;
 
     /* Prefix information (NDP option) */
     struct ndpopt *opt2 = mtod(t, struct ndpopt *);
@@ -192,6 +187,7 @@ void ndp_send_ra(Slirp *slirp)
     opt2->ndpopt_prefixinfo.reserved2 = 0;
     opt2->ndpopt_prefixinfo.prefix = slirp->vprefix_addr6;
     t->m_data += NDPOPT_PREFIXINFO_LEN;
+    pl_size += NDPOPT_PREFIXINFO_LEN;
 
 #ifndef _WIN32
     /* Prefix information (NDP option) */
@@ -203,16 +199,14 @@ void ndp_send_ra(Slirp *slirp)
     opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
     opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
     t->m_data += NDPOPT_RDNSS_LEN;
+    pl_size += NDPOPT_RDNSS_LEN;
 #endif
 
+    rip->ip_pl = htons(pl_size);
+    t->m_data -= sizeof(struct ip6) + pl_size;
+    t->m_len = sizeof(struct ip6) + pl_size;
+
     /* ICMPv6 Checksum */
-#ifndef _WIN32
-    t->m_data -= NDPOPT_RDNSS_LEN;
-#endif
-    t->m_data -= NDPOPT_PREFIXINFO_LEN;
-    t->m_data -= NDPOPT_LINKLAYER_LEN;
-    t->m_data -= ICMP6_NDP_RA_MINLEN;
-    t->m_data -= sizeof(struct ip6);
     ricmp->icmp6_cksum = ip6_cksum(t);
 
     ip6_output(NULL, t, 0);
-- 
2.11.0

[Qemu-devel] [PATCH 2/2] slirp: Send RDNSS in RA only if host has an IPv6 DNS server

[Samuel Thibault]

Previously we would always send an RDNSS option in the RA, making the guest
try to resolve DNS through IPv6, even if the host does not actually have
and IPv6 DNS server available.

This makes the RDNSS option enabled only when an IPv6 DNS server is
available.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 slirp/ip6_icmp.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index d0f5cc1456..00183e5945 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -189,18 +189,22 @@ void ndp_send_ra(Slirp *slirp)
     t->m_data += NDPOPT_PREFIXINFO_LEN;
     pl_size += NDPOPT_PREFIXINFO_LEN;
 
-#ifndef _WIN32
     /* Prefix information (NDP option) */
-    /* disabled for windows for now, until get_dns6_addr is implemented */
-    struct ndpopt *opt3 = mtod(t, struct ndpopt *);
-    opt3->ndpopt_type = NDPOPT_RDNSS;
-    opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
-    opt3->ndpopt_rdnss.reserved = 0;
-    opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
-    opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
-    t->m_data += NDPOPT_RDNSS_LEN;
-    pl_size += NDPOPT_RDNSS_LEN;
-#endif
+    {
+        struct in6_addr addr;
+        uint32_t scope_id;
+        if (get_dns6_addr(&addr, &scope_id) >= 0) {
+            /* Host system does have an IPv6 DNS server, announce our proxy.  */
+            struct ndpopt *opt3 = mtod(t, struct ndpopt *);
+            opt3->ndpopt_type = NDPOPT_RDNSS;
+            opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
+            opt3->ndpopt_rdnss.reserved = 0;
+            opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
+            opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
+            t->m_data += NDPOPT_RDNSS_LEN;
+            pl_size += NDPOPT_RDNSS_LEN;
+        }
+    }
 
     rip->ip_pl = htons(pl_size);
     t->m_data -= sizeof(struct ip6) + pl_size;
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/2] slirp: Make RA build more flexible

[Philippe Mathieu-Daudé]

On 03/26/2017 03:46 PM, Samuel Thibault wrote:
> Do not hardcode the RA size at all, use a pl_size variable which
> accounts the accumulated size, and fill rip->ip_pl at the end.
>
> This will allow to make some blocks optional.
>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  slirp/ip6_icmp.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
> index 298a48dd25..d0f5cc1456 100644
> --- a/slirp/ip6_icmp.c
> +++ b/slirp/ip6_icmp.c
> @@ -143,17 +143,10 @@ void ndp_send_ra(Slirp *slirp)
>      /* Build IPv6 packet */
>      struct mbuf *t = m_get(slirp);
>      struct ip6 *rip = mtod(t, struct ip6 *);
> +    size_t pl_size = 0;
>      rip->ip_src = (struct in6_addr)LINKLOCAL_ADDR;
>      rip->ip_dst = (struct in6_addr)ALLNODES_MULTICAST;
>      rip->ip_nh = IPPROTO_ICMPV6;
> -    rip->ip_pl = htons(ICMP6_NDP_RA_MINLEN
> -                        + NDPOPT_LINKLAYER_LEN
> -                        + NDPOPT_PREFIXINFO_LEN
> -#ifndef _WIN32
> -                        + NDPOPT_RDNSS_LEN
> -#endif
> -                        );
> -    t->m_len = sizeof(struct ip6) + ntohs(rip->ip_pl);
>
>      /* Build ICMPv6 packet */
>      t->m_data += sizeof(struct ip6);
> @@ -171,6 +164,7 @@ void ndp_send_ra(Slirp *slirp)
>      ricmp->icmp6_nra.reach_time = htonl(NDP_AdvReachableTime);
>      ricmp->icmp6_nra.retrans_time = htonl(NDP_AdvRetransTime);
>      t->m_data += ICMP6_NDP_RA_MINLEN;
> +    pl_size += ICMP6_NDP_RA_MINLEN;
>
>      /* Source link-layer address (NDP option) */
>      struct ndpopt *opt = mtod(t, struct ndpopt *);
> @@ -178,6 +172,7 @@ void ndp_send_ra(Slirp *slirp)
>      opt->ndpopt_len = NDPOPT_LINKLAYER_LEN / 8;
>      in6_compute_ethaddr(rip->ip_src, opt->ndpopt_linklayer);
>      t->m_data += NDPOPT_LINKLAYER_LEN;
> +    pl_size += NDPOPT_LINKLAYER_LEN;
>
>      /* Prefix information (NDP option) */
>      struct ndpopt *opt2 = mtod(t, struct ndpopt *);
> @@ -192,6 +187,7 @@ void ndp_send_ra(Slirp *slirp)
>      opt2->ndpopt_prefixinfo.reserved2 = 0;
>      opt2->ndpopt_prefixinfo.prefix = slirp->vprefix_addr6;
>      t->m_data += NDPOPT_PREFIXINFO_LEN;
> +    pl_size += NDPOPT_PREFIXINFO_LEN;
>
>  #ifndef _WIN32
>      /* Prefix information (NDP option) */
> @@ -203,16 +199,14 @@ void ndp_send_ra(Slirp *slirp)
>      opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
>      opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
>      t->m_data += NDPOPT_RDNSS_LEN;
> +    pl_size += NDPOPT_RDNSS_LEN;
>  #endif
>
> +    rip->ip_pl = htons(pl_size);
> +    t->m_data -= sizeof(struct ip6) + pl_size;
> +    t->m_len = sizeof(struct ip6) + pl_size;
> +
>      /* ICMPv6 Checksum */
> -#ifndef _WIN32
> -    t->m_data -= NDPOPT_RDNSS_LEN;
> -#endif
> -    t->m_data -= NDPOPT_PREFIXINFO_LEN;
> -    t->m_data -= NDPOPT_LINKLAYER_LEN;
> -    t->m_data -= ICMP6_NDP_RA_MINLEN;
> -    t->m_data -= sizeof(struct ip6);
>      ricmp->icmp6_cksum = ip6_cksum(t);
>
>      ip6_output(NULL, t, 0);
>

Re: [Qemu-devel] [PATCH 2/2] slirp: Send RDNSS in RA only if host has an IPv6 DNS server

[Philippe Mathieu-Daudé]

Hi Samuel,

On 03/26/2017 03:46 PM, Samuel Thibault wrote:
> Previously we would always send an RDNSS option in the RA, making the guest
> try to resolve DNS through IPv6, even if the host does not actually have
> and IPv6 DNS server available.
>
> This makes the RDNSS option enabled only when an IPv6 DNS server is
> available.
>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  slirp/ip6_icmp.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
> index d0f5cc1456..00183e5945 100644
> --- a/slirp/ip6_icmp.c
> +++ b/slirp/ip6_icmp.c
> @@ -189,18 +189,22 @@ void ndp_send_ra(Slirp *slirp)
>      t->m_data += NDPOPT_PREFIXINFO_LEN;
>      pl_size += NDPOPT_PREFIXINFO_LEN;
>
> -#ifndef _WIN32
>      /* Prefix information (NDP option) */
> -    /* disabled for windows for now, until get_dns6_addr is implemented */
> -    struct ndpopt *opt3 = mtod(t, struct ndpopt *);
> -    opt3->ndpopt_type = NDPOPT_RDNSS;
> -    opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
> -    opt3->ndpopt_rdnss.reserved = 0;
> -    opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
> -    opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
> -    t->m_data += NDPOPT_RDNSS_LEN;
> -    pl_size += NDPOPT_RDNSS_LEN;
> -#endif
> +    {

Why don't declare at function begining and remove this { } ?
Else reading the file one might ask himself "what if () condition was 
missed here?".
Except this indentation issue the patch is Ok for me.

> +        struct in6_addr addr;
> +        uint32_t scope_id;
> +        if (get_dns6_addr(&addr, &scope_id) >= 0) {
> +            /* Host system does have an IPv6 DNS server, announce our proxy.  */
> +            struct ndpopt *opt3 = mtod(t, struct ndpopt *);
> +            opt3->ndpopt_type = NDPOPT_RDNSS;
> +            opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
> +            opt3->ndpopt_rdnss.reserved = 0;
> +            opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
> +            opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
> +            t->m_data += NDPOPT_RDNSS_LEN;
> +            pl_size += NDPOPT_RDNSS_LEN;
> +        }
> +    }
>
>      rip->ip_pl = htons(pl_size);
>      t->m_data -= sizeof(struct ip6) + pl_size;
>

Phil.

Re: [Qemu-devel] [PATCH 2/2] slirp: Send RDNSS in RA only if host has an IPv6 DNS server

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, on lun. 27 mars 2017 11:56:00 -0300, wrote:
> Why don't declare at function begining and remove this { } ?

Oh, right, now I can.  While working on the code I still had ifdef
WIN32, so it'd lead to an unused variable warning.  But now that the
ifdef is gone, we can just put the variable at the beginning of the
function indeed.

Thanks,
Samuel