On Mon, May 20, 2024 at 12:14:40PM +0000, Florian Obser wrote:
> getifaddrs(3) does a non trivial amount of work.
> 
> Call it once and pass a pointer to the head of the list around when
> reconfiguring interfaces.

There were three patches related to rad(8) on tech@ recently:

1. send RTM_CHGADDRATTR for vltime / pltime changes
2. rad(8): reduce calls to getifaddrs(3)
3. rad(8): track vltime / pltime for "auto prefix"es

After I wrote so much about the third patch, I thought I should go
back and run rad(8) with just the first two patches applied (the
RTM_CHGADDRATTR patch already in -current, and the getifaddrs patch
manually applied).

That's been running perfectly for me for the last three days in a
small dual-stack setup, so this patch below is working great for me.

> OK?
> 
> diff --git frontend.c frontend.c
> index ca8719a4a1b..a49c576071d 100644
> --- frontend.c
> +++ frontend.c
> @@ -102,6 +102,7 @@ struct ra_iface {
>  	TAILQ_ENTRY(ra_iface)		 entry;
>  	struct icmp6_ev			*icmp6ev;
>  	struct ra_prefix_conf_head	 prefixes;
> +	struct ether_addr		 hw_addr;
>  	char				 name[IF_NAMESIZE];
>  	char				 conf_name[IF_NAMESIZE];
>  	uint32_t			 if_index;
> @@ -135,9 +136,8 @@ 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 *);
> -int			 get_link_state(char *);
>  int			 get_ifrdomain(char *);
> -void			 merge_ra_interface(char *, char *);
> +void			 merge_ra_interface(char *, char *, struct ifaddrs *);
>  void			 merge_ra_interfaces(void);
>  struct ra_iface		*find_ra_iface_by_id(uint32_t);
>  struct ra_iface		*find_ra_iface_by_name(char *);
> @@ -153,8 +153,7 @@ void			 add_new_prefix_to_ra_iface(struct ra_iface *r,
>  void			 free_ra_iface(struct ra_iface *);
>  int			 in6_mask2prefixlen(struct in6_addr *);
>  void			 get_interface_prefixes(struct ra_iface *,
> -			     struct ra_prefix_conf *);
> -int			 interface_has_linklocal_address(char *);
> +			     struct ra_prefix_conf *, struct ifaddrs *);
>  void			 build_packet(struct ra_iface *);
>  void			 build_leaving_packet(struct ra_iface *);
>  void			 ra_output(struct ra_iface *, struct sockaddr_in6 *);
> @@ -736,30 +735,6 @@ find_ra_iface_conf(struct ra_iface_conf_head *head, char *if_name)
>  	return (NULL);
>  }
>  
> -int
> -get_link_state(char *if_name)
> -{
> -	struct ifaddrs	*ifap, *ifa;
> -	int		 ls = LINK_STATE_UNKNOWN;
> -
> -	if (getifaddrs(&ifap) != 0) {
> -		log_warn("getifaddrs");
> -		return LINK_STATE_UNKNOWN;
> -	}
> -	for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> -		if (ifa->ifa_addr == NULL ||
> -		    ifa->ifa_addr->sa_family != AF_LINK)
> -			continue;
> -		if (strcmp(if_name, ifa->ifa_name) != 0)
> -			continue;
> -
> -		ls = ((struct if_data*)ifa->ifa_data)->ifi_link_state;
> -		break;
> -	}
> -	freeifaddrs(ifap);
> -	return ls;
> -}
> -
>  int
>  get_ifrdomain(char *if_name)
>  {
> @@ -774,27 +749,75 @@ get_ifrdomain(char *if_name)
>  }
>  
>  void
> -merge_ra_interface(char *name, char *conf_name)
> +merge_ra_interface(char *if_name, char *conf_name, struct ifaddrs *ifap)
>  {
>  	struct ra_iface		*ra_iface;
> +	struct ifaddrs		*ifa;
> +	struct sockaddr_in6	*sin6;
> +	struct in6_ifreq	 ifr6;
> +	struct sockaddr_dl	*sdl;
> +	struct ether_addr	 hw_addr;
>  	uint32_t		 if_index;
> -	int			 link_state, has_linklocal, ifrdomain;
> +	int			 link_state = LINK_STATE_UNKNOWN;
> +	int			 has_linklocal = 0, ifrdomain;
> +	int			 has_hw_addr = 0;
> +
> +	for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> +		if (ifa->ifa_addr == NULL)
> +			continue;
> +		if (ifa->ifa_addr->sa_family != AF_LINK &&
> +		    ifa->ifa_addr->sa_family != AF_INET6)
> +			continue;
> +		if (strcmp(if_name, ifa->ifa_name) != 0)
> +			continue;
> +
> +		if (ifa->ifa_addr->sa_family == AF_LINK) {
> +			link_state =
> +			    ((struct if_data*)ifa->ifa_data)->ifi_link_state;
> +			sdl = (struct sockaddr_dl *)ifa->ifa_addr;
> +			if (sdl->sdl_type == IFT_ETHER &&
> +			    sdl->sdl_alen == ETHER_ADDR_LEN) {
> +				has_hw_addr = 1;
> +				memcpy(&hw_addr, LLADDR(sdl), ETHER_ADDR_LEN);
> +			}
> +		} else if (ifa->ifa_addr->sa_family == AF_INET6) {
> +			sin6 = (struct sockaddr_in6 *)ifa->ifa_addr;
>  
> -	link_state = get_link_state(name);
> -	has_linklocal = interface_has_linklocal_address(name);
> -	ifrdomain = get_ifrdomain(name);
> +			if (!IN6_IS_ADDR_LINKLOCAL(&sin6->sin6_addr))
> +				continue;
> +
> +			memset(&ifr6, 0, sizeof(ifr6));
> +			strlcpy(ifr6.ifr_name, if_name, sizeof(ifr6.ifr_name));
> +			memcpy(&ifr6.ifr_addr, sin6, sizeof(ifr6.ifr_addr));
> +			if (ioctl(ioctlsock, SIOCGIFAFLAG_IN6,
> +			    (caddr_t)&ifr6) == -1) {
> +				log_warn("SIOCGIFAFLAG_IN6");
> +				continue;
> +			}
>  
> -	if ((ra_iface = find_ra_iface_by_name(name)) != NULL) {
> +			if (ifr6.ifr_ifru.ifru_flags6 & (IN6_IFF_TENTATIVE |
> +			    IN6_IFF_DUPLICATED))
> +				continue;
> +			has_linklocal = 1;
> +		}
> +	}
> +
> +	ifrdomain = get_ifrdomain(if_name);
> +
> +	if ((ra_iface = find_ra_iface_by_name(if_name)) != NULL) {
>  		ra_iface->link_state = link_state;
>  		if (!LINK_STATE_IS_UP(link_state)) {
> -			log_debug("%s down, removing", name);
> +			log_debug("%s down, removing", if_name);
>  			ra_iface->removed = 1;
>  		} else if (!has_linklocal) {
>  			log_debug("%s has no IPv6 link-local address, "
> -			    "removing", name);
> +			    "removing", if_name);
>  			ra_iface->removed = 1;
>  		} else if (ifrdomain == -1) {
> -			log_debug("can't get rdomain for %s, removing", name);
> +			log_debug("can't get rdomain for %s, removing", if_name);
> +			ra_iface->removed = 1;
> +		} else if (!has_hw_addr) {
> +			log_debug("%s has no mac address, removing", if_name);
>  			ra_iface->removed = 1;
>  		} else if (ra_iface->rdomain != ifrdomain) {
>  			leave_all_routers_mcast_group(ra_iface);
> @@ -804,37 +827,49 @@ merge_ra_interface(char *name, char *conf_name)
>  			join_all_routers_mcast_group(ra_iface);
>  			ra_iface->removed = 0;
>  		} else {
> -			log_debug("keeping interface %s", name);
> +			log_debug("keeping interface %s", if_name);
>  			ra_iface->removed = 0;
>  		}
> +		memcpy(&ra_iface->hw_addr, &hw_addr, sizeof(hw_addr));
>  		return;
>  	}
>  
>  	if (!LINK_STATE_IS_UP(link_state)) {
> -		log_debug("%s down, ignoring", name);
> +		log_debug("%s down, ignoring", if_name);
>  		return;
>  	}
>  
>  	if (!has_linklocal) {
> -		log_debug("%s has no IPv6 link-local address, ignoring", name);
> +		log_debug("%s has no IPv6 link-local address, ignoring",
> +		    if_name);
>  		return;
>  	}
>  
> -	log_debug("new interface %s", name);
> -	if ((if_index = if_nametoindex(name)) == 0)
> +	if (ifrdomain == -1) {
> +		log_debug("can't get rdomain for %s, ignoring", if_name);
>  		return;
> +	}
>  
> -	log_debug("adding interface %s", name);
> +	if (!has_hw_addr) {
> +		log_debug("%s has no mac address, ignoring", if_name);
> +		return;
> +	}
> +
> +	log_debug("new interface %s", if_name);
> +	if ((if_index = if_nametoindex(if_name)) == 0)
> +		return;
> +
> +	log_debug("adding interface %s", if_name);
>  	if ((ra_iface = calloc(1, sizeof(*ra_iface))) == NULL)
>  		fatal("%s", __func__);
>  
> -	strlcpy(ra_iface->name, name, sizeof(ra_iface->name));
> +	strlcpy(ra_iface->name, if_name, sizeof(ra_iface->name));
>  	strlcpy(ra_iface->conf_name, conf_name,
>  	    sizeof(ra_iface->conf_name));
>  
>  	ra_iface->if_index = if_index;
>  	ra_iface->rdomain = ifrdomain;
> -
> +	memcpy(&ra_iface->hw_addr, &hw_addr, sizeof(hw_addr));
>  	SIMPLEQ_INIT(&ra_iface->prefixes);
>  
>  	ra_iface->icmp6ev = get_icmp6ev_by_rdomain(ifrdomain);
> @@ -850,9 +885,15 @@ merge_ra_interfaces(void)
>  	struct ra_iface		*ra_iface;
>  	struct ifgroupreq	 ifgr;
>  	struct ifg_req		*ifg;
> +	struct ifaddrs		*ifap;
>  	char			*conf_name;
>  	unsigned int		 len;
>  
> +	if (getifaddrs(&ifap) != 0) {
> +		log_warn("getifaddrs");
> +		return;
> +	}
> +
>  	TAILQ_FOREACH(ra_iface, &ra_interfaces, entry)
>  		ra_iface->removed = 1;
>  
> @@ -861,7 +902,7 @@ merge_ra_interfaces(void)
>  
>  		/* check if network interface or group */
>  		if (isdigit((unsigned char)conf_name[strlen(conf_name) - 1])) {
> -			merge_ra_interface(conf_name, conf_name);
> +			merge_ra_interface(conf_name, conf_name, ifap);
>  		} else {
>  			log_debug("interface group %s", conf_name);
>  
> @@ -888,7 +929,7 @@ merge_ra_interfaces(void)
>  			    ifg++) {
>  				len -= sizeof(struct ifg_req);
>  				merge_ra_interface(ifg->ifgrq_member,
> -				    conf_name);
> +				    conf_name, ifap);
>  			}
>  			free(ifgr.ifgr_groups);
>  		}
> @@ -925,10 +966,11 @@ merge_ra_interfaces(void)
>  
>  		if (ra_iface_conf->autoprefix)
>  			get_interface_prefixes(ra_iface,
> -			    ra_iface_conf->autoprefix);
> +			    ra_iface_conf->autoprefix, ifap);
>  
>  		build_packet(ra_iface);
>  	}
> +	freeifaddrs(ifap);
>  }
>  
>  void
> @@ -972,62 +1014,15 @@ in6_mask2prefixlen(struct in6_addr *in6)
>  	return (plen);
>  }
>  
> -int
> -interface_has_linklocal_address(char *name)
> -{
> -	struct ifaddrs		*ifap, *ifa;
> -	struct sockaddr_in6	*sin6;
> -	struct in6_ifreq	 ifr6;
> -	int			 ret = 0;
> -
> -	if (getifaddrs(&ifap) != 0)
> -		fatal("getifaddrs");
> -
> -	for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
> -		if (strcmp(name, ifa->ifa_name) != 0)
> -			continue;
> -		if (ifa->ifa_addr == NULL ||
> -		    ifa->ifa_addr->sa_family != AF_INET6)
> -			continue;
> -
> -		sin6 = (struct sockaddr_in6 *)ifa->ifa_addr;
> -
> -		if (!IN6_IS_ADDR_LINKLOCAL(&sin6->sin6_addr))
> -			continue;
> -
> -		memset(&ifr6, 0, sizeof(ifr6));
> -		strlcpy(ifr6.ifr_name, name, sizeof(ifr6.ifr_name));
> -		memcpy(&ifr6.ifr_addr, sin6, sizeof(ifr6.ifr_addr));
> -		if (ioctl(ioctlsock, SIOCGIFAFLAG_IN6, (caddr_t)&ifr6) == -1) {
> -			log_warn("SIOCGIFAFLAG_IN6");
> -			continue;
> -		}
> -
> -		if (ifr6.ifr_ifru.ifru_flags6 & (IN6_IFF_TENTATIVE |
> -		    IN6_IFF_DUPLICATED))
> -			continue;
> -
> -		ret = 1;
> -		break;
> -	}
> -	freeifaddrs(ifap);
> -	return (ret);
> -}
> -
>  void
>  get_interface_prefixes(struct ra_iface *ra_iface, struct ra_prefix_conf
> -    *autoprefix)
> +    *autoprefix, struct ifaddrs *ifap)
>  {
>  	struct in6_ifreq	 ifr6;
> -	struct ifaddrs		*ifap, *ifa;
> +	struct ifaddrs		*ifa;
>  	struct sockaddr_in6	*sin6;
>  	int			 prefixlen;
>  
> -	log_debug("%s: %s", __func__, ra_iface->name);
> -
> -	if (getifaddrs(&ifap) != 0)
> -		fatal("getifaddrs");
> -
>  	for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
>  		if (strcmp(ra_iface->name, ifa->ifa_name) != 0)
>  			continue;
> @@ -1058,7 +1053,6 @@ get_interface_prefixes(struct ra_iface *ra_iface, struct ra_prefix_conf
>  		add_new_prefix_to_ra_iface(ra_iface, &sin6->sin6_addr,
>  		    prefixlen, autoprefix);
>  	}
> -	freeifaddrs(ifap);
>  }
>  
>  struct ra_prefix_conf*
> @@ -1118,8 +1112,6 @@ 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;
> @@ -1187,30 +1179,9 @@ build_packet(struct ra_iface *ra_iface)
>  		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);
> -		} else
> -			len -= sizeof(*ndopt_source_link_addr);
> +		memcpy(&ndopt_source_link_addr->nd_opt_source_link_addr_hw_addr,
> +		    &ra_iface->hw_addr, ETHER_ADDR_LEN);
> +		p += sizeof(*ndopt_source_link_addr);
>  	}
>  
>  	if (ra_options_conf->mtu > 0) {
> 
> -- 
> In my defence, I have been left unsupervised.