Re: [PATCH v2 1/8] netlink: Add function to extract mac addresses from arp table

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 12 Jun 2025 00:21:45 -0400
Jon Maloy <jmaloy@redhat.com> wrote:

> The solution to bug #120 requires the ability to translate from
> an IP address to its corresponding MAC address in cases where
> those are present in the ARP/NTP table.

Nit: NDP.

> We add this feature here.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>

Nit: David's address is david@gibson.dropbear.id.au (in case you need
to Cc: him specifically).

Nit: while bug #120 isn't fixed at once by one specific patch in this
series, I would rather be liberal with Link: tags, better a few than
nothing, should we ever need to look up commits in the future. That is,

Link: https://bugs.passt.top/show_bug.cgi?id=120

would fit nicely here and in a few other patches, I think.

> ---
>  netlink.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  netlink.h |  1 +
>  2 files changed, 59 insertions(+)
> 
> diff --git a/netlink.c b/netlink.c
> index ee9325a..6c55c4c 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -800,6 +800,64 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
>  	return status;
>  }
>  
> +/**
> + * nl_mac_get() - Get mac address corresponding to given IP address

Nit: MAC address

> + * @s:		Netlink socket
> + * @addr:	IPv4 or IPv6 address
> + * @mac:	Array to place the returned MAC address
> + *
> + * Return: 0 on success, negative error code on failure

Kind of. As far as I understand, this also returns 0 if the MAC address
is not found. Should we perhaps set @mac to all-zeroes in that case,
and say "0 if found or not in table, ..."?

> + */
> +int nl_mac_get(int s, const union inany_addr *addr, unsigned char *mac)
> +{
> +	struct nlmsghdr *nlh;
> +	char buf[NLBUFSIZ];
> +	const void *ip;

I guess you should be able to use inany_equals() and keep addresses as
inany_addr as much as possible.

> +	ssize_t status;
> +	uint32_t seq;
> +	int alen;
> +	struct {
> +		struct nlmsghdr nlh;
> +		struct ndmsg ndm;
> +	} req;

You can actually ask the kernel to do the filtering for you (as long as
NETLINK_GET_STRICT_CHK is available, >= 4.20), by passing a NDA_DST
attribute in the request. See e.g. 'strace ip neigh get ... dev ...'.

As far as I know, even if the device (ndm_ifindex) is mandatory in
ip-neighbour(8), the kernel doesn't actually need it.

By the way, I'm not sure if we want to give a preference to a
particular interface, in case we have different MAC addresses for the
same IP address on different interfaces.

I would say we don't care and we can just pick one because we don't
always have the inbound interface available anyway, but I haven't
really thought it through.

> +
> +	if (IN6_IS_ADDR_V4MAPPED(&addr->a6)) {
> +		ip = &addr->v4mapped.a4;
> +		alen = sizeof(struct in_addr);
> +		req.ndm.ndm_family = AF_INET;
> +	} else {
> +		ip = &addr->a6;
> +		alen = sizeof(struct in6_addr);
> +		req.ndm.ndm_family = AF_INET6;
> +	}
> +
> +	seq = nl_send(s, &req, RTM_GETNEIGH, NLM_F_DUMP, sizeof(req));
> +	nl_foreach_oftype(nlh, status, s, buf, seq, RTM_NEWNEIGH) {
> +		struct ndmsg *ndm = NLMSG_DATA(nlh);
> +		struct rtattr *attr = (struct rtattr *)(ndm + 1);
> +		int attrlen = nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*ndm));
> +		unsigned char *lladdr = NULL;
> +		void *neigh_ip = NULL;

cppcheck says:

netlink.c:839:18: style: Variable 'lladdr' can be declared as pointer to const [constVariablePointer]
  unsigned char *lladdr = NULL;
                 ^
netlink.c:840:9: style: Variable 'neigh_ip' can be declared as pointer to const [constVariablePointer]
  void *neigh_ip = NULL;
        ^

Try 'make cppcheck' (or running all the tests, that would be even
better).

> +
> +		for (; RTA_OK(attr, attrlen); attr = RTA_NEXT(attr, attrlen)) {
> +			if (attr->rta_type == NDA_DST)
> +				neigh_ip = RTA_DATA(attr);
> +			else if (attr->rta_type == NDA_LLADDR)
> +				lladdr = RTA_DATA(attr);
> +		}
> +
> +		if (!neigh_ip || !lladdr)
> +			continue;
> +
> +		if (!memcmp(neigh_ip, ip, alen)) {

...the filtering is still needed for kernel versions < 4.20 (we also do
it in other functions, such as nl_route_dup()), but in general it
should be unnecessary.

That's important to avoid huge netlink messages and substantial overhead
in case we have a lot of neighbours in the table.

> +			memcpy(mac, lladdr, ETH_ALEN);
> +			return 0;
> +		}
> +	}
> +
> +	return status;
> +}
> +
>  /**
>   * nl_addr_get_ll() - Get first IPv6 link-local address for a given interface
>   * @s:		Netlink socket
> diff --git a/netlink.h b/netlink.h
> index b51e99c..2f674d7 100644
> --- a/netlink.h
> +++ b/netlink.h
> @@ -17,6 +17,7 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  		 int s_dst, unsigned int ifi_dst, sa_family_t af);
>  int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
>  		void *addr, int *prefix_len, void *addr_l);
> +int nl_mac_get(int s, const union inany_addr *addr, unsigned char *mac);
>  int nl_addr_set(int s, unsigned int ifi, sa_family_t af,
>  		const void *addr, int prefix_len);
>  int nl_addr_get_ll(int s, unsigned int ifi, struct in6_addr *addr);

-- 
Stefano