fix DNS resolution in ping6 #13450

benpicco · 2020-02-22T19:14:58Z

Contribution description

Previously DNS resolution in ping6 would always fail.
This was due to a slew of minor bugs in sock_dns and gnrc_icmpv6_echo.
This PR fixes them, so DNS resolution is working properly now.

Testing procedure

If you are using a board with direct WiFi or Ethernet connection (e.g. esp8266 or esp32), it should suffice to add USEMODULE += sock_dns to examples/gnrc_networking.

On a 6LoWPAN device, the border router (examples/gnrc_border_router) doesn't seem to forward DNS information, so you have to resort to tests/gnrc_sock_dns to being able to set the DNS server manually by calling e.g. dns server 2620:119:35::35:.

You should now be able to ping a host by name:

2020-02-22 20:10:44,986 #  ping6 riot-os.org
2020-02-22 20:10:45,114 # 12 bytes from 2a01:4f8:151:64::11: icmp_seq=0 ttl=56 time=71.772 ms
2020-02-22 20:10:46,071 # 12 bytes from 2a01:4f8:151:64::11: icmp_seq=1 ttl=56 time=29.318 ms
2020-02-22 20:10:47,122 # 12 bytes from 2a01:4f8:151:64::11: icmp_seq=2 ttl=56 time=79.199 ms

Issues/PRs references

fixes #13447
follow-up on #13443

sys/net/application_layer/dns/dns.c

miri64

tests/gnrc_sock_udp is failing with this PR. On master it still works.

sys/net/application_layer/dns/dns.c

benpicco · 2020-02-24T12:25:22Z

I restored the test to use AF_UNSPEC and sock_dns_query() to return the size of the address again, updated the documentation to reflect that.

The test is passing now again on native.

miri64 · 2020-02-24T12:38:58Z

I've marked it as an API change due to the doc change. However, since the doc is just adopted to the behavior, I don't think we need two ACKs here.

miri64

Tested on native and samr21-xpro using tests/gnrc_sock_dns (don't have the set-up here to test with the border router and ping6 but I trust your testing there). One minor nit, to prevent errors introduced in future fixes. Other than that you may squash.

miri64 · 2020-02-24T12:43:18Z

sys/net/application_layer/dns/dns.c

+
+        sock_dns_hdr_t *hdr = (sock_dns_hdr_t*) buf;
+        memset(hdr, 0, sizeof(*hdr));
+        hdr->id = 0; /* random? */


I know this is just pulled down, but this should at least come from a variable now, that it is re-set on every retry.

random_bytes(&hdr->id, sizeof(hdr->id));?

miri64

Don't overshoot ;-)

sys/net/application_layer/dns/dns.c

We need to set `res` to 0 to signal success, otherwise we end up in the print usage case.

When using ping6 with an IP address, don't do a DNS lookup. Hostnames can't contain ':', so use that to tell them apart from plain IP addressees.

miri64 · 2020-02-24T13:27:23Z

Regarding the border router not forwarding the RDNSS option:

Did you set GNRC_IPV6_NIB_CONF_DNS=1?

Is sock_dns_server set correctly in _handle_rdnsso()?

RIOT/sys/net/gnrc/network_layer/ipv6/nib/nib.c

Lines 1366 to 1422 in 63f805c

    
           #if GNRC_IPV6_NIB_CONF_DNS 
        
           static uint32_t _handle_rdnsso(gnrc_netif_t *netif, const icmpv6_hdr_t *icmpv6, 
        
                                          const ndp_opt_rdnss_impl_t *rdnsso) 
        
           { 
        
               uint32_t ltime = UINT32_MAX; 
        
               const ipv6_addr_t *addr; 
        
               if ((rdnsso->len < NDP_OPT_RDNSS_MIN_LEN) || 
        
                   (icmpv6->type != ICMPV6_RTR_ADV)) { 
        
                   return ltime; 
        
               } 
        
               /* select first if unassigned, search possible address otherwise */ 
        
               addr = (sock_dns_server.port == 0) ? &rdnsso->addrs[0] : NULL; 
        
               if (addr == NULL) { 
        
                   unsigned addrs_num = (rdnsso->len - 1) / 2; 
        
                   for (unsigned i = 0; i < addrs_num; i++) { 
        
                       if (memcmp(sock_dns_server.addr.ipv6, 
        
                                  &rdnsso->addrs[i], 
        
                                  sizeof(rdnsso->addrs[i])) == 0) { 
        
                           addr = &rdnsso->addrs[i]; 
        
                           break; 
        
                       } 
        
                   } 
        
               } 
        
           #if SOCK_HAS_IPV6 
        
               ltime = byteorder_ntohl(rdnsso->ltime); 
        
               if (addr != NULL) { 
        
                   if (ltime > 0) { 
        
                       sock_dns_server.port = SOCK_DNS_PORT; 
        
                       sock_dns_server.family = AF_INET6; 
        
                       sock_dns_server.netif = netif->pid; 
        
                       memcpy(sock_dns_server.addr.ipv6, rdnsso->addrs, 
        
                              sizeof(sock_dns_server.addr.ipv6)); 
        
                       if (ltime < UINT32_MAX) { 
        
                           /* the valid lifetime is given in seconds, but our timers work 
        
                            * in milliseconds, so we have to scale down to the smallest 
        
                            * possible value (UINT32_MAX - 1). This is however alright 
        
                            * since we ask for a new router advertisement before this 
        
                            * timeout expires */ 
        
                           ltime = (ltime > (UINT32_MAX / MS_PER_SEC)) ? 
        
                                         (UINT32_MAX - 1) : ltime * MS_PER_SEC; 
        
                           _evtimer_add(&sock_dns_server, GNRC_IPV6_NIB_RDNSS_TIMEOUT, 
        
                                        &_rdnss_timeout, ltime); 
        
                       } 
        
                   } 
        
                   else { 
        
                       evtimer_del(&_nib_evtimer, &_rdnss_timeout.event); 
        
                       _handle_rdnss_timeout(&sock_dns_server); 
        
                   } 
        
               } 
        
           #else 
        
               (void)addr; 
        
           #endif 
        
               return ltime; 
        
           } 
        
           #endif

Is the option for the sent router advertisement constructed correctly in _build_ext_opts()?

RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c

Lines 137 to 156 in 63f805c

    
           #if GNRC_IPV6_NIB_CONF_DNS && SOCK_HAS_IPV6 
        
               uint32_t rdnss_ltime = _evtimer_lookup(&sock_dns_server, 
        
                                                      GNRC_IPV6_NIB_RDNSS_TIMEOUT); 
        
               if ((rdnss_ltime < UINT32_MAX) && 
        
                   (!ipv6_addr_is_link_local((ipv6_addr_t *)sock_dns_server.addr.ipv6))) { 
        
                   gnrc_pktsnip_t *rdnsso = gnrc_ndp_opt_rdnss_build( 
        
                           rdnss_ltime * MS_PER_SEC, 
        
                           (ipv6_addr_t *)&sock_dns_server.addr, 
        
                           1U, ext_opts 
        
                       ); 
        
                   if (rdnsso == NULL) { 
        
                       /* gnrc_ndp_opt_rdnss_build() only returns NULL when pktbuf is full 
        
                        * in this configuration */ 
        
                       DEBUG("nib: No space left in packet buffer. Not adding RDNSSO\n"); 
        
                       return NULL; 
        
                   } 
        
                   ext_opts = rdnsso; 
        
               } 
        
           #endif  /* GNRC_IPV6_NIB_CONF_DNS */

miri64

Re-ACK

Saving RAM is more important than saving a few cycles used by re-creating the request buffer in the error case. Also reduce the size of the buffer to 128 bytes. If we are just requesting the AAAA record it is unlikely for the reply to take up the maximum size of 512 bytes. We were already placing restrictions on the domain name length, those are now actually a bit more relaxed (112 bytes instead of 64)

The implementation already did that, now also reflect this in the documentation.

benpicco · 2020-02-24T13:31:21Z

Did you set GNRC_IPV6_NIB_CONF_DNS=1?

I didn't know such option exists, but it gets set automatically if gnrc_ipv6_nib_dns is used - for tests/gnrc_sock_dns this is the case.

I will investigate the 2. & 3. when I'm home.

miri64 · 2020-02-24T13:32:35Z

I didn't know such option exists, but it gets set automatically if gnrc_ipv6_nib_dns is used - for tests/gnrc_sock_dns this is the case.

I will investigate the 2. & 3. when I'm home.

but not for gnrc_border_router, right?

benpicco · 2020-02-24T21:44:11Z

When I add gnrc_ipv6_nib_dns to gnrc_border_router it will indeed forward the DNS server information!

benpicco · 2020-02-24T22:18:40Z

And with CFLAGS += -DTHREAD_STACKSIZE_MAIN=1024 the crashes on ATmega also stop.

benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Feb 22, 2020

benpicco requested review from cgundogan, haukepetersen, miri64 and PeterKietzmann as code owners February 22, 2020 19:14

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 22, 2020

benpicco commented Feb 22, 2020

View reviewed changes

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

benpicco requested a review from kaspar030 February 22, 2020 19:24

miri64 requested changes Feb 23, 2020

View reviewed changes

sys/net/application_layer/dns/dns.c Show resolved Hide resolved

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 23, 2020

miri64 requested changes Feb 24, 2020

View reviewed changes

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

miri64 added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Feb 24, 2020

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 24, 2020

miri64 approved these changes Feb 24, 2020

View reviewed changes

miri64 requested changes Feb 24, 2020

View reviewed changes

sys/net/application_layer/dns/dns.c Outdated Show resolved Hide resolved

benpicco added 3 commits February 24, 2020 14:19

sock_dns: fix off-by-one error in _parse_dns_reply()

e9a4093

shell/gnrc_icmpv6_echo: set success state if DNS query succeeded

a65daf7

We need to set `res` to 0 to signal success, otherwise we end up in the print usage case.

shell/gnrc_icmpv6_echo: don't do DNS lookup for plain IP

367f19d

When using ping6 with an IP address, don't do a DNS lookup. Hostnames can't contain ':', so use that to tell them apart from plain IP addressees.

benpicco force-pushed the dns-fixes branch from 2ff0af4 to 601cc3a Compare February 24, 2020 13:23

miri64 approved these changes Feb 24, 2020

View reviewed changes

benpicco and others added 2 commits February 24, 2020 14:29

sock_dns: make sock_dns_query() return the length of the address

fa44472

The implementation already did that, now also reflect this in the documentation.

benpicco force-pushed the dns-fixes branch from ec97e9b to fa44472 Compare February 24, 2020 13:29

benpicco removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 24, 2020

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 24, 2020

benpicco merged commit d044800 into RIOT-OS:master Feb 24, 2020

benpicco deleted the dns-fixes branch February 24, 2020 20:58

benpicco mentioned this pull request Mar 3, 2020

gnrc_dhcpv6_client: configure prefix as compression context on 6LBRs #13485

Merged

leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix DNS resolution in ping6 #13450

fix DNS resolution in ping6 #13450

benpicco commented Feb 22, 2020 •

edited

Loading

miri64 left a comment

benpicco commented Feb 24, 2020

miri64 commented Feb 24, 2020

miri64 left a comment

miri64 Feb 24, 2020

benpicco Feb 24, 2020

miri64 left a comment

miri64 commented Feb 24, 2020

miri64 left a comment

benpicco commented Feb 24, 2020

miri64 commented Feb 24, 2020

benpicco commented Feb 24, 2020

benpicco commented Feb 24, 2020 •

edited

Loading

fix DNS resolution in ping6 #13450

fix DNS resolution in ping6 #13450

Conversation

benpicco commented Feb 22, 2020 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

miri64 left a comment

Choose a reason for hiding this comment

benpicco commented Feb 24, 2020

miri64 commented Feb 24, 2020

miri64 left a comment

Choose a reason for hiding this comment

miri64 Feb 24, 2020

Choose a reason for hiding this comment

benpicco Feb 24, 2020

Choose a reason for hiding this comment

miri64 left a comment

Choose a reason for hiding this comment

miri64 commented Feb 24, 2020

miri64 left a comment

Choose a reason for hiding this comment

benpicco commented Feb 24, 2020

miri64 commented Feb 24, 2020

benpicco commented Feb 24, 2020

benpicco commented Feb 24, 2020 • edited Loading

benpicco commented Feb 22, 2020 •

edited

Loading

benpicco commented Feb 24, 2020 •

edited

Loading