From: Florian Obser Subject: Re: add source link-layer address option to rad(8) To: tech@openbsd.org Date: Wed, 15 May 2024 21:14:14 +0200 On 2024-05-11 12:34 -07, Ryan Vogt wrote: > On Fri, May 10, 2024 at 07:36:28PM +0000, Florian Obser wrote: >> On 2024-05-07 16:15 -07, Ryan Vogt wrote: > Now, despite having a perfectly good IPv6 router and a functional IPv6 > nameserver, a similar situation occurs in macOS as to what happens in > iOS: a few packets go over the WiFi connection, then nothing. The > culprit seems to be that macOS' deault route is in interface scope: > > $ netstat -rn > [...snip...] > Internet 6: > Destination Gateway Flags Netif > default fe80::...%en0 UGcIg en0 > [...snip...] > > That "I" flag is what I'm looking at. With the default route being in > interface scope, you can do things like: > > $ ping6 fdaa:...:1%en0 > > but not: > > $ ping6 fdaa:...:1 > Interesting, I have no idea what they are doing here. The netstat man page is not explaining what I is, it and it's not a thing FreeBSD has either. At least it's not documented in their netstat man page. This kinda maybe looks like a bug in macOS / iOS where they are not doing neighbor discovery? But regardless, rad(8) really should send a source link-layer address, otherwise every client has to do one more round trip. Very nice find! > Thank you also for pointing me to slaacd/frontend.c. It was really > helpful to look at the precise details there. > > But, I think there's a little hiccup in consistency between > rad/frontend.c and slaacd/frontend.c here. In rad/frontend.c, all of > the structs for the different RA options contain both the header (the > type and the length) and the body. For example: > > struct nd_opt_route_info (from ) > > which is used in rad/frontend.c, contains header fields > nd_opt_rti_type and nd_opt_rti_len, then the body of the option > (prefix len, etc.). > > struct nd_opt_route_info { > /* Header */ > u_int8_t nd_opt_rti_type; > u_int8_t nd_opt_rti_len; > /* Body */ > u_int8_t nd_opt_rti_prefixlen; > u_int8_t nd_opt_rti_flags; > u_int32_t nd_opt_rti_lifetime; > }; > > Similarly, > > struct nd_opt_pref64 (from rad/frontend.c) > > also contains the header (nd_opt_pref64_type and nd_opt_pref64_len) > and the body (nd_opt_p ref64_sltime_plc and nd_opt_pref64). > > struct nd_opt_pref64 { > /* Header */ > u_int8_t nd_opt_pref64_type; > u_int8_t nd_opt_pref64_len; > /* Body */ > u_int16_t nd_opt_pref64_sltime_plc; > u_int8_t nd_opt_pref64[12]; > }; > > I'll call that the rad/frontend.c style -- structs named nd_opt_[...] > which contain both the header and the body/data for each option. > > On the other hand, in slaacd/frontend.c, the header and the data are > two different structs. There's a struct nd_opt_hdr, but also a > struct ether_addr named nd_opt_source_link_addr. > > nd_opt_hdr nd_opt_hdr; > struct ether_addr nd_opt_source_link_addr; > > Here in slaacd/frontend.c, it's the variable name for the body alone > that follows the naming convention of nd_opt_[...]. I'll call that the > slaacd/frontend.c style -- two separate structs, this time with the > body variable named nd_opt_[...]. > > The question is: which style should the proposed patch follow? > > Knowing now that all the world is ethernet (no more arbitrary-length > body), I could define a struct in the rad/frontend.c style that holds > both the header and the body, just like the other structs in > rad/frontend.c. Call this Option 1: > > struct nd_opt_source_link_addr { > /* Header */ > u_int8_t nd_opt_source_link_addr_type; > u_int8_t nd_opt_source_link_addr_len; > /* Body */ > struct ether_addr nd_opt_source_link_addr_hw_addr; > }; > > Then, in rad/frontend.c's build_packet(), I have a: > > struct nd_opt_source_link_addr *ndopt_source_link_addr; > > alongside the: > > struct nd_opt_mtu *ndopt_mtu; > struct nd_opt_prefix_info *ndopt_prefix_info; > > That uses the same name for the source link-layer address option as > slaacd/frontend.c (specifically, "nd_opt_source_link_addr"), but > follows the rad/frontend.c convention of putting the entirety of the > option (header and body) into a single struct. > Yes, this is fine. And presumably even better. I had confused myself with slaacd/engine.c ra_parse(), where slaacd only deals with nd_opts that are defined in icmp6.h. The issue really is that nd_opt_source_link_addr (and nd_opt_pref64) are not defined in icmp6.h > >> This was an oversight by me to not implement it when rewriting >> rtadvd(8). I suspect it did not support this option either. > > Just for the purposes of people searching the archives for the > equivalent configuration option between rtadvd.conf(5) and > rad.conf(5), the option in OpenBSD 6.3's rtadvd(8) was: > > nolladdr Right, still on oversight on my end to not implement it :) > > > A few quick notes: > > 1. In build_packet(), I've placed the struct pointers > nd_opt_source_link_addr, nd_opt_mtu, and nd_opt_prefix_info in the > order they're presented in RFC 4861 section 4.2. sure, style(9) has size: When declaring variables in functions, declare them sorted by size (largest to smallest), I usually can't be arsed to work out which struct is larger, unless I know it off the top of my head. > > 2. I'm uncertain why we're doing a strcmp() against the name of the > interface while iterating over ifap, instead of comparing against > the interface index. But, since that's how it's done in > slaacd/frontend.c, where both the name and the index are also > available, I'm assuming there's a reason we use the name instead of > the index that I just don't know about. So, I switched to using a > strcmp() against the interface name instead of comparing against > the index. I was under the impression that an interface index could be recycled when you unplug a usb device and plug in a different one. But that does not seem to be true. if_index seems to be monotonically increasing. I think it's best to leave this for now as is and maybe change all "may" daemons to use if_index. This probably shows up in dhcpleased, slaacd and unwind as well. > > 3. Despite the assumption that all the world is ethernet, it feels > wrong to me to do the memcpy() into the struct ether_addr without > verifying that the address I'm copying is actually an ethernet > address of length ETHER_ADDR_LEN. If the check is extraneous and > unnecessary, I can easily remove it. Sure, that's what slaacd does as well. I was thinking along the lines of: there is no need to support anything else than ethernet, because you just won't encounter it on openbsd, lot's of things would need to change. It's still nice to check that we get what we expect. > > Here's a new version of the proposed patch: > Looks good, I have one comment inline > diff --git frontend.c frontend.c > index 6748e43732d..7610fc8d9b7 100644 > --- frontend.c > +++ frontend.c > @@ -56,6 +56,7 @@ > #include > > #include > +#include > #include > #include > > @@ -120,6 +121,12 @@ struct nd_opt_pref64 { > u_int8_t nd_opt_pref64[12]; > }; > > +struct nd_opt_source_link_addr { > + u_int8_t nd_opt_source_link_addr_type; > + u_int8_t nd_opt_source_link_addr_len; > + struct ether_addr nd_opt_source_link_addr_hw_addr; > +}; > + > TAILQ_HEAD(, ra_iface) ra_interfaces; > > __dead void frontend_shutdown(void); > @@ -1099,6 +1106,7 @@ void > build_packet(struct ra_iface *ra_iface) > { > struct nd_router_advert *ra; > + struct nd_opt_source_link_addr *ndopt_source_link_addr; > struct nd_opt_mtu *ndopt_mtu; > struct nd_opt_prefix_info *ndopt_pi; > struct ra_iface_conf *ra_iface_conf; > @@ -1110,6 +1118,8 @@ build_packet(struct ra_iface *ra_iface) > struct ra_rdnss_conf *ra_rdnss; > struct ra_dnssl_conf *ra_dnssl; > struct ra_pref64_conf *pref64; > + struct ifaddrs *ifap, *ifa; > + struct sockaddr_dl *sdl; > size_t len, label_len; > uint8_t *p, buf[RA_MAX_SIZE]; > char *label_start, *label_end; > @@ -1119,6 +1129,8 @@ build_packet(struct ra_iface *ra_iface) > ra_options_conf = &ra_iface_conf->ra_options; > > len = sizeof(*ra); > + if (ra_iface_conf->ra_options.source_link_addr) > + len += sizeof(*ndopt_source_link_addr); > if (ra_options_conf->mtu > 0) > len += sizeof(*ndopt_mtu); > len += sizeof(*ndopt_pi) * ra_iface->prefix_count; > @@ -1170,6 +1182,35 @@ build_packet(struct ra_iface *ra_iface) > ra->nd_ra_retransmit = htonl(ra_options_conf->retrans_timer); > p += sizeof(*ra); > > + if (ra_iface_conf->ra_options.source_link_addr) { > + ndopt_source_link_addr = (struct nd_opt_source_link_addr *)p; > + ndopt_source_link_addr->nd_opt_source_link_addr_type = > + ND_OPT_SOURCE_LINKADDR; > + ndopt_source_link_addr->nd_opt_source_link_addr_len = 1; > + if (getifaddrs(&ifap) != 0) { > + ifap = NULL; > + log_warn("getifaddrs"); > + } > + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { > + if (ifa->ifa_addr == NULL || > + ifa->ifa_addr->sa_family != AF_LINK) > + continue; > + if (strcmp(ra_iface->name, ifa->ifa_name) != 0) > + continue; > + sdl = (struct sockaddr_dl *)ifa->ifa_addr; > + if (sdl->sdl_type != IFT_ETHER || > + sdl->sdl_alen != ETHER_ADDR_LEN) > + continue; > + memcpy(&ndopt_source_link_addr-> > + nd_opt_source_link_addr_hw_addr, > + LLADDR(sdl), ETHER_ADDR_LEN); > + break; > + } > + if (ifap != NULL) > + freeifaddrs(ifap); > + p += sizeof(*ndopt_source_link_addr); if you fail to find a mac address, for example because you are racing against an interface being unplugged, you will (try to) send a source link addr option with a mac address of all zero. I think the easiest way to avoid that is doing something like this: if (ifap != NULL) { freeifaddrs(ifap); p += sizeof(*ndopt_source_link_addr); } else len -= sizeof(*ndopt_source_link_addr); > + } > + > if (ra_options_conf->mtu > 0) { > ndopt_mtu = (struct nd_opt_mtu *)p; > ndopt_mtu->nd_opt_mtu_type = ND_OPT_MTU; > diff --git parse.y parse.y > index 66cc0c0ab64..b9d369bde17 100644 > --- parse.y > +++ parse.y > @@ -122,6 +122,7 @@ typedef struct { > %token CONFIGURATION OTHER LIFETIME REACHABLE TIME RETRANS TIMER > %token AUTO PREFIX VALID PREFERENCE PREFERRED LIFETIME ONLINK AUTONOMOUS > %token ADDRESS_CONFIGURATION DNS NAMESERVER SEARCH MTU NAT64 HIGH MEDIUM LOW > +%token SOURCE LINK_LAYER > > %token STRING > %token NUMBER > @@ -226,6 +227,9 @@ ra_opt_block : DEFAULT ROUTER yesno { > | RETRANS TIMER NUMBER { > ra_options->retrans_timer = $3; > } > + | SOURCE LINK_LAYER ADDRESS yesno { > + ra_options->source_link_addr = $4; > + } > | MTU NUMBER { > ra_options->mtu = $2; > } > @@ -523,6 +527,7 @@ lookup(char *s) > {"interface", RA_IFACE}, > {"lifetime", LIFETIME}, > {"limit", LIMIT}, > + {"link-layer", LINK_LAYER}, > {"low", LOW}, > {"managed", MANAGED}, > {"medium", MEDIUM}, > @@ -539,6 +544,7 @@ lookup(char *s) > {"retrans", RETRANS}, > {"router", ROUTER}, > {"search", SEARCH}, > + {"source", SOURCE}, > {"time", TIME}, > {"timer", TIMER}, > {"valid", VALID}, > diff --git printconf.c printconf.c > index 184a5df2dc1..2edfb06edd4 100644 > --- printconf.c > +++ printconf.c > @@ -78,6 +78,8 @@ print_ra_options(const char *indent, const struct ra_options_conf *ra_options) > printf("%srouter lifetime %d\n", indent, ra_options->router_lifetime); > printf("%sreachable time %u\n", indent, ra_options->reachable_time); > printf("%sretrans timer %u\n", indent, ra_options->retrans_timer); > + printf("%ssource link-layer address %s\n", indent, > + yesno(ra_options->source_link_addr)); > if (ra_options->mtu > 0) > printf("%smtu %u\n", indent, ra_options->mtu); > > diff --git rad.c rad.c > index b283e58fbf1..9093932c563 100644 > --- rad.c > +++ rad.c > @@ -757,6 +757,7 @@ config_new_empty(void) > xconf->ra_options.router_lifetime = ADV_DEFAULT_LIFETIME; > xconf->ra_options.reachable_time = 0; > xconf->ra_options.retrans_timer = 0; > + xconf->ra_options.source_link_addr = 1; > xconf->ra_options.mtu = 0; > xconf->ra_options.rdns_lifetime = DEFAULT_RDNS_LIFETIME; > SIMPLEQ_INIT(&xconf->ra_options.ra_rdnss_list); > diff --git rad.conf.5 rad.conf.5 > index f82ccfd216e..a2589cda88c 100644 > --- rad.conf.5 > +++ rad.conf.5 > @@ -127,6 +127,10 @@ The default is medium. > .\" XXX > .\" .It Ic retrans timer Ar number > .\" XXX > +.It Ic source link-layer address Pq Ic yes Ns | Ns Ic no > +Add a source link-layer address option to router advertisement messages, to > +communicate the link-layer address of the sending interface. > +The default is yes. > .El > .Sh INTERFACES > A list of interfaces or interface groups to send advertisements on: > diff --git rad.h rad.h > index 787e78c7c4a..61187dba273 100644 > --- rad.h > +++ rad.h > @@ -97,6 +97,7 @@ struct ra_options_conf { > int router_lifetime; /* default router lifetime */ > uint32_t reachable_time; > uint32_t retrans_timer; > + int source_link_addr; /* source link-layer address */ > uint32_t mtu; > uint32_t rdns_lifetime; > SIMPLEQ_HEAD(, ra_rdnss_conf) ra_rdnss_list; > -- In my defence, I have been left unsupervised.