From: Florian Obser Subject: Re: add source link-layer address option to rad(8) To: Ryan Vogt Cc: tech@openbsd.org Date: Fri, 10 May 2024 21:36:28 +0200 Hi, On 2024-05-07 16:15 -07, Ryan Vogt wrote: > Hi tech@, > > This is my first time sending a proposed patch, so hello everyone. > This patch adds support to rad(8) for the source link-layer address > option in router advertisement messages (RFC 4861, section 4.2). awesome, we certainly need more people who are willing to roll around in IPv6. > > Motivation: if an RA message includes a DNS option (RDNSS, DNSSL, or > both), but the RA message doesn't include the source link-layer > address option, Apple clients (macOS, iOS, etc.) in an IPv6-only > environment will misbehave. I'm more than happy to share the details > of how they misbehave if anyone would like, but I'll leave it at > "misbehave" here for brevity. I'm interested. Might also be worthwhile to have it in the archives. > > Quick notes about the patch: > > 1. I avoided abbreviating "link-layer address" as "lladrr", > "linkaddr", etc., where possible, to avoid any confusion with link- > local addresses. So, while "linklayer_addr" is more verbose, I hope > it minimizes confusion. Please have a look at sbin/slaacd/frontend.c how it's handled there. We fetch the mac address in update_iface() and use it in send_solicitation(). - please use similar variable names / structs - you can assume that all the world is ethernet and that link-layer addresses are available. Otherwise something is really wrong and rad(8) is the least of our problems. > > 2. RFC 4861 section 4.2 states that the source link-layer address > option "MAY" be omitted, which might be read as meaning that it > should be included by default? That said, I didn't want to change > the default and current behaviour of rad(8) -- that is, not > including the option -- because excluding it does serve a purpose > (inbound load sharing, see RFC 4861). So, I made the default in > rad.conf(5) to exclude the source link-layer address option. I don't think you can get OpenBSD to support inbound load sharing like this. The default should be yes. This was an oversight by me to not implement it when rewriting rtadvd(8). I suspect it did not support this option either. > > 3. I used a "struct nd_opt_hdr" to hold the header for the source > link-layer address option, instead of making a new struct (e.g., > "struct nd_opt_src_linklayer_addr"). I did that because there's > nothing in the source link-layer address option except the type, > length, and arbitrary-sized extra data (the arbitrarily sized link- > layer address), just how a "struct nd_opt_hdr" is defined in > . I could define a new struct instead if it would > help readability or consistency. yeah, that's fine, just a bit overcomplicated, see my response to 1. > > I'd be grateful for any feedback. > some more comments inline > diff --git usr.sbin/rad/frontend.c usr.sbin/rad/frontend.c > index 6748e43732d..b8db63e02b6 100644 > --- usr.sbin/rad/frontend.c > +++ usr.sbin/rad/frontend.c > @@ -56,6 +56,7 @@ > #include > > #include > +#include > #include > #include > > @@ -128,6 +129,7 @@ void frontend_startup(void); > void icmp6_receive(int, short, void *); > void join_all_routers_mcast_group(struct ra_iface *); > void leave_all_routers_mcast_group(struct ra_iface *); > +struct sockaddr_dl *find_linklayer_sockaddr(struct ifaddrs *, int); > int get_link_state(char *); > int get_ifrdomain(char *); > void merge_ra_interface(char *, char *); > @@ -729,6 +731,23 @@ find_ra_iface_conf(struct ra_iface_conf_head *head, char *if_name) > return (NULL); > } > > +struct sockaddr_dl* > +find_linklayer_sockaddr(struct ifaddrs *ifap, int if_index) > +{ > + struct ifaddrs *ifa; > + > + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { > + if (ifa->ifa_addr == NULL || > + ifa->ifa_addr->sa_family != AF_LINK) > + continue; > + if (((struct sockaddr_dl *)ifa->ifa_addr)->sdl_index == > + if_index) > + return (struct sockaddr_dl *)ifa->ifa_addr; > + } > + log_warn("find_linklayer_sockaddr"); As said, you can assume that a link-layer address always exists and that it's ethernet. Please inline this into build packet and and copy the address into a struct ether_addr. then you don't need to hold on to ifap for such a long time. > + return (NULL); > +} > + > int > get_link_state(char *if_name) > { > @@ -1110,6 +1129,9 @@ 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 nd_opt_hdr *ndopt_srclinklayeraddr; > + struct ifaddrs *ifap; > + struct sockaddr_dl *sa_linklayer; > size_t len, label_len; > uint8_t *p, buf[RA_MAX_SIZE]; > char *label_start, *label_end; > @@ -1136,6 +1158,20 @@ build_packet(struct ra_iface *ra_iface) > entry) > len += sizeof(struct nd_opt_pref64); > > + ifap = NULL; > + sa_linklayer = NULL; > + if (ra_iface_conf->ra_options.src_linklayer_addr) { > + if (getifaddrs(&ifap) != 0) > + log_warn("getifaddrs"); > + else > + sa_linklayer = find_linklayer_sockaddr(ifap, > + ra_iface->if_index); > + if (sa_linklayer != NULL) > + /* round up to 8 byte boundary */ > + len += (sizeof(struct nd_opt_hdr) + > + sa_linklayer->sdl_alen + 7) & ~7; > + } > + > if (len > sizeof(ra_iface->data)) > fatalx("%s: packet too big", __func__); /* XXX send multiple */ > > @@ -1283,6 +1319,22 @@ build_packet(struct ra_iface *ra_iface) > p += sizeof(struct nd_opt_pref64); > } > > + if (sa_linklayer != NULL) { > + ndopt_srclinklayeraddr = (struct nd_opt_hdr *)p; > + ndopt_srclinklayeraddr->nd_opt_type = ND_OPT_SOURCE_LINKADDR; > + /* length expressed in units of 8 octets */ > + ndopt_srclinklayeraddr->nd_opt_len = > + ((sizeof(struct nd_opt_hdr) + sa_linklayer->sdl_alen + 7) & > + ~7) / 8; > + p += sizeof(struct nd_opt_hdr); > + memcpy(p, LLADDR(sa_linklayer), sa_linklayer->sdl_alen); > + p += sa_linklayer->sdl_alen; > + while (((uintptr_t)p) % 8 != 0) > + *p++ = '\0'; memset(3) ;) but this goes away once you use struct ether_addr > + } > + if (ifap != NULL) > + freeifaddrs(ifap); > + > if (len != ra_iface->datalen || memcmp(buf, ra_iface->data, len) > != 0) { > memcpy(ra_iface->data, buf, len); > diff --git usr.sbin/rad/parse.y usr.sbin/rad/parse.y > index 66cc0c0ab64..fb9f4628854 100644 > --- usr.sbin/rad/parse.y > +++ usr.sbin/rad/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->src_linklayer_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 usr.sbin/rad/printconf.c usr.sbin/rad/printconf.c > index 184a5df2dc1..4cf64fabd93 100644 > --- usr.sbin/rad/printconf.c > +++ usr.sbin/rad/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->src_linklayer_addr)); > if (ra_options->mtu > 0) > printf("%smtu %u\n", indent, ra_options->mtu); > > diff --git usr.sbin/rad/rad.conf.5 usr.sbin/rad/rad.conf.5 > index f82ccfd216e..11545c1c23f 100644 > --- usr.sbin/rad/rad.conf.5 > +++ usr.sbin/rad/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 no. > .El > .Sh INTERFACES > A list of interfaces or interface groups to send advertisements on: > diff --git usr.sbin/rad/rad.h usr.sbin/rad/rad.h > index 787e78c7c4a..509f91b208a 100644 > --- usr.sbin/rad/rad.h > +++ usr.sbin/rad/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 src_linklayer_addr; > uint32_t mtu; > uint32_t rdns_lifetime; > SIMPLEQ_HEAD(, ra_rdnss_conf) ra_rdnss_list; > -- In my defence, I have been left unsupervised.