Download raw body.
add source link-layer address option to rad(8)
Hi,
On 2024-05-07 16:15 -07, Ryan Vogt <rvogt.ca@gmail.com> 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
> <netinet/icmp6.h>. 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 <sys/uio.h>
>
> #include <net/if.h>
> +#include <net/if_dl.h>
> #include <net/if_types.h>
> #include <net/route.h>
>
> @@ -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 <v.string> STRING
> %token <v.number> 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.
add source link-layer address option to rad(8)