Re: unwind(8): only use available address families
On Tue, Jan 26, 2021 at 05:53:26PM +0100, Klemens Nanni wrote: > On Tue, Jan 26, 2021 at 05:22:42PM +0100, Florian Obser wrote: > > On Mon, Jan 25, 2021 at 07:05:40PM +0100, Florian Obser wrote: > > > Unwind / libunbound goes pretty badly off the rails when an address > > > family is not available, it still tries to talk to nameservers with an > > > unreachable address family. > > > I don't think it's libunbound's place to figure this out. It can't > > > sensibly do a getifaddrs on every query... > > > So let's help it out a bit. > > > > > > OK? > > > > This is better. > > > > - 100% less ioctl, leading to tighter pledge after clue-bat from > > claudio while working in asr. > > - also handle RTM_DESYNC, pointed out by deraadt > Cool, it works as intended and is stable (after the other fix) for me. > > > I was also toying with the idea of counting arivals and departure of > > IP addresses using the routing socket, but getting the account right > > seems complicated. I don't think a call to getifaddrs will be > > triggered that often. > I think that's fair. > > Two things inline, with that > OK kn > > > + for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) { > > + if (ifa->ifa_addr == NULL) > > + continue; > > + switch(ifa->ifa_addr->sa_family) { > > + case AF_LINK: > Can you add the same comment here as well as you did in asr? > /* AF_LINK comes before inet / inet6 on an interface */ Sure, thanks. > > > + ifa_data = (struct if_data *)ifa->ifa_data; > > + ifa_rtable = ifa_data->ifi_rdomain; > > + break; > > + case AF_INET: > > + if (ifa_rtable != rtable) > > + continue; > > > > rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_PROPOSAL) > > - | ROUTE_FILTER(RTM_IFANNOUNCE); > > + | ROUTE_FILTER(RTM_IFANNOUNCE) | ROUTE_FILTER(RTM_NEWADDR) > > + | ROUTE_FILTER(RTM_DELADDR); > Looks like you missed `RTM_DESYNC' here. Nope, see rtsock.c: 534 /* filter messages that the process does not want */ 535 rtm = mtod(m, struct rt_msghdr *); 536 /* but RTM_DESYNC can't be filtered */ 537 if (rtm->rtm_type != RTM_DESYNC) { > > > if (setsockopt(frontend_routesock, AF_ROUTE, ROUTE_MSGFILTER, > > &rtfilter, sizeof(rtfilter)) == -1) > > fatal("setsockopt(ROUTE_MSGFILTER)"); > -- I'm not entirely sure you are real.
Re: unwind(8): only use available address families
On Tue, Jan 26, 2021 at 05:22:42PM +0100, Florian Obser wrote: > On Mon, Jan 25, 2021 at 07:05:40PM +0100, Florian Obser wrote: > > Unwind / libunbound goes pretty badly off the rails when an address > > family is not available, it still tries to talk to nameservers with an > > unreachable address family. > > I don't think it's libunbound's place to figure this out. It can't > > sensibly do a getifaddrs on every query... > > So let's help it out a bit. > > > > OK? > > This is better. > > - 100% less ioctl, leading to tighter pledge after clue-bat from > claudio while working in asr. > - also handle RTM_DESYNC, pointed out by deraadt Cool, it works as intended and is stable (after the other fix) for me. > I was also toying with the idea of counting arivals and departure of > IP addresses using the routing socket, but getting the account right > seems complicated. I don't think a call to getifaddrs will be > triggered that often. I think that's fair. Two things inline, with that OK kn > + for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) { > + if (ifa->ifa_addr == NULL) > + continue; > + switch(ifa->ifa_addr->sa_family) { > + case AF_LINK: Can you add the same comment here as well as you did in asr? /* AF_LINK comes before inet / inet6 on an interface */ > + ifa_data = (struct if_data *)ifa->ifa_data; > + ifa_rtable = ifa_data->ifi_rdomain; > + break; > + case AF_INET: > + if (ifa_rtable != rtable) > + continue; > rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_PROPOSAL) > - | ROUTE_FILTER(RTM_IFANNOUNCE); > + | ROUTE_FILTER(RTM_IFANNOUNCE) | ROUTE_FILTER(RTM_NEWADDR) > + | ROUTE_FILTER(RTM_DELADDR); Looks like you missed `RTM_DESYNC' here. > if (setsockopt(frontend_routesock, AF_ROUTE, ROUTE_MSGFILTER, > &rtfilter, sizeof(rtfilter)) == -1) > fatal("setsockopt(ROUTE_MSGFILTER)");
Re: unwind(8): only use available address families
On Mon, Jan 25, 2021 at 07:05:40PM +0100, Florian Obser wrote: > Unwind / libunbound goes pretty badly off the rails when an address > family is not available, it still tries to talk to nameservers with an > unreachable address family. > I don't think it's libunbound's place to figure this out. It can't > sensibly do a getifaddrs on every query... > So let's help it out a bit. > > OK? This is better. - 100% less ioctl, leading to tighter pledge after clue-bat from claudio while working in asr. - also handle RTM_DESYNC, pointed out by deraadt I was also toying with the idea of counting arivals and departure of IP addresses using the routing socket, but getting the account right seems complicated. I don't think a call to getifaddrs will be triggered that often. diff --git frontend.c frontend.c index 50dab6c70ca..b25fd76fe2e 100644 --- frontend.c +++ frontend.c @@ -32,6 +32,7 @@ #include #include +#include #include #include #include @@ -150,6 +151,7 @@ void parse_blocklist(int); int bl_cmp(struct bl_node *, struct bl_node *); voidfree_bl(void); int pending_query_cnt(void); +voidcheck_available_af(void); struct uw_conf *frontend_conf; static struct imsgev *iev_main; @@ -212,7 +214,7 @@ frontend(int debug, int verbose) setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid)) fatal("can't drop privileges"); - if (pledge("stdio unix recvfd", NULL) == -1) + if (pledge("stdio dns unix recvfd", NULL) == -1) fatal("pledge"); event_init(); @@ -660,6 +662,7 @@ frontend_startup(void) event_add(&ev_route, NULL); frontend_imsg_compose_main(IMSG_STARTUP_DONE, 0, NULL, 0); + check_available_af(); } void @@ -1362,6 +1365,11 @@ handle_route_message(struct rt_msghdr *rtm, struct sockaddr **rti_info) frontend_imsg_compose_resolver(IMSG_REPLACE_DNS, 0, &rdns_proposal, sizeof(rdns_proposal)); break; + case RTM_NEWADDR: + case RTM_DELADDR: + case RTM_DESYNC: + check_available_af(); + break; default: break; } @@ -1765,3 +1773,66 @@ tcp_timeout(int fd, short events, void *arg) { free_pending_query(arg); } + +void +check_available_af() +{ + static int available_af = HAVE_IPV4 | HAVE_IPV6; + static int rtable = -1; + struct ifaddrs *ifap, *ifa; + struct if_data *ifa_data; + struct sockaddr_in *sin4; + struct sockaddr_in6 *sin6; + int new_available_af = 0, ifa_rtable = -1; + + if (rtable == -1) + rtable = getrtable(); + + if (getifaddrs(&ifap) != 0) { + log_warn("getifaddrs"); + return; + } + + for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) { + if (ifa->ifa_addr == NULL) + continue; + switch(ifa->ifa_addr->sa_family) { + case AF_LINK: + ifa_data = (struct if_data *)ifa->ifa_data; + ifa_rtable = ifa_data->ifi_rdomain; + break; + case AF_INET: + if (ifa_rtable != rtable) + continue; + + sin4 = (struct sockaddr_in *)ifa->ifa_addr; + if ((ntohl(sin4->sin_addr.s_addr) >> 24) == + IN_LOOPBACKNET) + continue; + new_available_af |= HAVE_IPV4; + break; + case AF_INET6: + if (ifa_rtable != rtable) + continue; + + sin6 = (struct sockaddr_in6 *)ifa->ifa_addr; + if (IN6_IS_ADDR_LOOPBACK(&sin6->sin6_addr) || + IN6_IS_ADDR_LINKLOCAL(&sin6->sin6_addr) || + IN6_IS_ADDR_MC_LINKLOCAL(&sin6->sin6_addr) || + IN6_IS_ADDR_MC_INTFACELOCAL(&sin6->sin6_addr)) + continue; + new_available_af |= HAVE_IPV6; + break; + default: + break; + } + if (new_available_af == (HAVE_IPV4 | HAVE_IPV6)) + break; + } + freeifaddrs(ifap); + if (new_available_af != available_af) { + available_af = new_available_af; + frontend_imsg_compose_resolver(IMSG_CHANGE_AFS, 0, + &available_af, sizeof(available_af)); + } +} diff --git frontend.h frontend.h index cd6c21875af..0c18c7524dc 100644 --- frontend.h +++ frontend.h @@ -17,6 +17,9 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWAR
Re: unwind(8): only use available address families
On Mon, Jan 25, 2021 at 08:56:36PM +0100, Klemens Nanni wrote: > On Mon, Jan 25, 2021 at 07:05:40PM +0100, Florian Obser wrote: > > Unwind / libunbound goes pretty badly off the rails when an address > > family is not available, it still tries to talk to nameservers with an > > unreachable address family. > > I don't think it's libunbound's place to figure this out. It can't > > sensibly do a getifaddrs on every query... > > So let's help it out a bit. > Cool, this is pretty much what I had in mind after your reply to the > other thread. > > Have you tested this by adding some IPv4 address? unwind dies on me: > > $ ifconfig all inet | grep -w inet > inet 127.0.0.1 netmask 0xff00 > $ doas ifconfig tun0 1.2.3.4 > > check_available_af 3 - 2 > fatal in resolver: new_resolver: invalid resolver state: dead > frontend exiting > waiting for children to terminate > terminating > This has been fixed, and it was indeed unreleated to this diff. We also need to restart dead resolvers when we restart all resolvers because of a config change in libunbound. (e.g. changing of log level or new trust anchors). -- I'm not entirely sure you are real.
Re: unwind(8): only use available address families
On 25 January 2021 20:56:36 CET, Klemens Nanni wrote: >On Mon, Jan 25, 2021 at 07:05:40PM +0100, Florian Obser wrote: >> Unwind / libunbound goes pretty badly off the rails when an address >> family is not available, it still tries to talk to nameservers with >an >> unreachable address family. >> I don't think it's libunbound's place to figure this out. It can't >> sensibly do a getifaddrs on every query... >> So let's help it out a bit. >Cool, this is pretty much what I had in mind after your reply to the >other thread. > >Have you tested this by adding some IPv4 address? unwind dies on me: Yes, that's actually the only thing I tried, flipping v4 on and off. > > $ ifconfig all inet | grep -w inet > inet 127.0.0.1 netmask 0xff00 > $ doas ifconfig tun0 1.2.3.4 > > check_available_af 3 - 2 > fatal in resolver: new_resolver: invalid resolver state: dead Pretty sure this isn't a bug in this diff, it just gets exposed more easily. I also see that I left too much debug code in there... I'll have a look tomorrow. > frontend exiting > waiting for children to terminate > terminating -- Sent from a mobile device. Please excuse poor formating.
Re: unwind(8): only use available address families
On Mon, Jan 25, 2021 at 07:05:40PM +0100, Florian Obser wrote: > Unwind / libunbound goes pretty badly off the rails when an address > family is not available, it still tries to talk to nameservers with an > unreachable address family. > I don't think it's libunbound's place to figure this out. It can't > sensibly do a getifaddrs on every query... > So let's help it out a bit. Cool, this is pretty much what I had in mind after your reply to the other thread. Have you tested this by adding some IPv4 address? unwind dies on me: $ ifconfig all inet | grep -w inet inet 127.0.0.1 netmask 0xff00 $ doas ifconfig tun0 1.2.3.4 check_available_af 3 - 2 fatal in resolver: new_resolver: invalid resolver state: dead frontend exiting waiting for children to terminate terminating
unwind(8): only use available address families
Unwind / libunbound goes pretty badly off the rails when an address family is not available, it still tries to talk to nameservers with an unreachable address family. I don't think it's libunbound's place to figure this out. It can't sensibly do a getifaddrs on every query... So let's help it out a bit. OK? (I'm not 100% sure where to place check_available_af(). Maybe this should go to the main process.) diff --git frontend.c frontend.c index 50dab6c70ca..c5e5da91b9a 100644 --- frontend.c +++ frontend.c @@ -20,6 +20,7 @@ */ #include +#include #include #include #include @@ -32,6 +33,7 @@ #include #include +#include #include #include #include @@ -150,6 +152,8 @@ void parse_blocklist(int); int bl_cmp(struct bl_node *, struct bl_node *); voidfree_bl(void); int pending_query_cnt(void); +int get_ifrdomain(char *); +voidcheck_available_af(void); struct uw_conf *frontend_conf; static struct imsgev *iev_main; @@ -158,6 +162,7 @@ struct event ev_route; int udp4sock = -1, udp6sock = -1; int tcp4sock = -1, tcp6sock = -1; int ta_fd = -1; +int ioctlsock; static struct trust_anchor_head trust_anchors, new_trust_anchors; @@ -212,7 +217,10 @@ frontend(int debug, int verbose) setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid)) fatal("can't drop privileges"); - if (pledge("stdio unix recvfd", NULL) == -1) + if ((ioctlsock = socket(AF_INET6, SOCK_DGRAM, 0)) == -1) + fatal("socket"); + + if (pledge("stdio unix recvfd route", NULL) == -1) fatal("pledge"); event_init(); @@ -660,6 +668,7 @@ frontend_startup(void) event_add(&ev_route, NULL); frontend_imsg_compose_main(IMSG_STARTUP_DONE, 0, NULL, 0); + check_available_af(); } void @@ -1362,6 +1371,10 @@ handle_route_message(struct rt_msghdr *rtm, struct sockaddr **rti_info) frontend_imsg_compose_resolver(IMSG_REPLACE_DNS, 0, &rdns_proposal, sizeof(rdns_proposal)); break; + case RTM_NEWADDR: + case RTM_DELADDR: + check_available_af(); + break; default: break; } @@ -1765,3 +1778,72 @@ tcp_timeout(int fd, short events, void *arg) { free_pending_query(arg); } + +int +get_ifrdomain(char *if_name) +{ + struct ifreq ifr; + + strlcpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name)); + if (ioctl(ioctlsock, SIOCGIFRDOMAIN, (caddr_t)&ifr) == -1) { + log_warn("SIOCGIFRDOMAIN"); + return -1; + } + return ifr.ifr_rdomainid; +} + +void +check_available_af() +{ + static int available_af = HAVE_IPV4 | HAVE_IPV6; + static int rtable = -1; + struct ifaddrs *ifap, *ifa; + struct sockaddr_in *sin4; + struct sockaddr_in6 *sin6; + int new_available_af = 0; + + if (rtable == -1) + rtable = getrtable(); + + if (getifaddrs(&ifap) != 0) { + log_warn("getifaddrs"); + return; + } + + for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) { + if (get_ifrdomain(ifa->ifa_name) != getrtable()) + continue; + if (ifa->ifa_addr == NULL) + continue; + switch(ifa->ifa_addr->sa_family) { + case AF_INET: + sin4 = (struct sockaddr_in *)ifa->ifa_addr; + if ((ntohl(sin4->sin_addr.s_addr) >> 24) == + IN_LOOPBACKNET) + continue; + new_available_af |= HAVE_IPV4; + break; + case AF_INET6: + sin6 = (struct sockaddr_in6 *)ifa->ifa_addr; + if (IN6_IS_ADDR_LOOPBACK(&sin6->sin6_addr) || + IN6_IS_ADDR_LINKLOCAL(&sin6->sin6_addr) || + IN6_IS_ADDR_MC_LINKLOCAL(&sin6->sin6_addr) || + IN6_IS_ADDR_MC_INTFACELOCAL(&sin6->sin6_addr)) + continue; + new_available_af |= HAVE_IPV6; + break; + default: + break; + } + if (new_available_af == (HAVE_IPV4 | HAVE_IPV6)) + break; + } + freeifaddrs(ifap); + if (new_available_af != available_af) { + log_debug("%s %d - %d", __func__, new_available_af, + available_af); + available_af = new_available_af; + frontend_imsg_compose_resolver(IMSG_CHANGE_AFS, 0,