[PATCH] rtnl: Add neighbor discovery utilities

[PATCH] rtnl: Add neighbor discovery utilities

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 9969 bytes --]

Add a few variants of RTNL utilities to look up addresses in the
kernel's neighbour table entries and to add new entries.  This is
basically the ARP (for IPv4) and NDP (for IPv6) tables, that map IP
addresses to hardware addresses.
---
There are three variants of each method, I currently only have a use
case for the ipv4 and ipv6 variants so I'm happy dropping the struct
l_rtnl_address variant.

 ell/ell.sym  |   6 ++
 ell/rtnl.c   | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/rtnl.h   |  36 ++++++++++
 ell/useful.h |   4 ++
 4 files changed, 240 insertions(+)

diff --git a/ell/ell.sym b/ell/ell.sym
index b29d98d..8760f2b 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -663,6 +663,12 @@ global:
 	l_rtnl_ifaddr_extract;
 	l_rtnl_ifaddr_add;
 	l_rtnl_ifaddr_delete;
+	l_rtnl_neighbour_get_ipv4_hwaddr;
+	l_rtnl_neighbour_get_ipv6_hwaddr;
+	l_rtnl_neighbour_get_addr_hwaddr;
+	l_rtnl_neighbour_set_ipv4_hwaddr;
+	l_rtnl_neighbour_set_ipv6_hwaddr;
+	l_rtnl_neighbour_set_addr_hwaddr;
 /* icmp6 */
 	l_icmp6_client_new;
 	l_icmp6_client_free;
diff --git a/ell/rtnl.c b/ell/rtnl.c
index 1061105..4d81c48 100644
--- a/ell/rtnl.c
+++ b/ell/rtnl.c
@@ -27,6 +27,9 @@
 #define _GNU_SOURCE
 #include <linux/if.h>
 #include <linux/icmpv6.h>
+#include <linux/neighbour.h>
+#include <linux/if_ether.h>
+#include <net/if_arp.h>
 #include <sys/socket.h>
 #include <arpa/inet.h>
 #include <errno.h>
@@ -34,6 +37,7 @@
 #include "useful.h"
 #include "netlink.h"
 #include "log.h"
+#include "util.h"
 #include "rtnl.h"
 #include "private.h"
 
@@ -1311,3 +1315,193 @@ LIB_EXPORT uint32_t l_rtnl_route_delete(struct l_netlink *rtnl, int ifindex,
 	return _rtnl_route_change(rtnl, RTM_DELROUTE, ifindex, rt,
 						cb, user_data, destroy);
 }
+
+struct rtnl_neighbour_get_data {
+	l_rtnl_neighbour_get_cb_t cb;
+	void *user_data;
+	l_netlink_destroy_func_t destroy;
+};
+
+static void rtnl_neighbour_get_cb(int error, uint16_t type, const void *data,
+					uint32_t len, void *user_data)
+{
+	struct rtnl_neighbour_get_data *cb_data = user_data;
+	const struct ndmsg *ndmsg = data;
+	struct rtattr *attr;
+	const uint8_t *hwaddr = NULL;
+
+	if (error != 0)
+		goto done;
+
+	if (type != RTM_NEWNEIGH || len < NLMSG_ALIGN(sizeof(*ndmsg))) {
+		error = -EIO;
+		goto done;
+	}
+
+	if (!(ndmsg->ndm_state & (NUD_PERMANENT | NUD_NOARP | NUD_REACHABLE))) {
+		error = -ENOENT;
+		goto done;
+	}
+
+	attr = (void *) ndmsg + NLMSG_ALIGN(sizeof(*ndmsg));
+	len -= NLMSG_ALIGN(sizeof(*ndmsg));
+
+	for (; RTA_OK(attr, len); attr = RTA_NEXT(attr, len))
+		switch (attr->rta_type) {
+		case NDA_LLADDR:
+			hwaddr = RTA_DATA(attr);
+			break;
+		}
+
+	if (!hwaddr)
+		error = -EIO;
+
+done:
+	if (cb_data->cb) {
+		cb_data->cb(error, hwaddr, cb_data->user_data);
+		cb_data->cb = NULL;
+	}
+}
+
+static void rtnl_neighbour_get_destroy_cb(void *user_data)
+{
+	struct rtnl_neighbour_get_data *cb_data = user_data;
+
+	if (cb_data->destroy)
+		cb_data->destroy(cb_data->user_data);
+
+	l_free(cb_data);
+}
+
+static uint32_t rtnl_neighbour_get_hwaddr(struct l_netlink *rtnl,
+					int ifindex, uint8_t family,
+					const void *addr,
+					l_rtnl_neighbour_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	size_t bufsize = NLMSG_ALIGN(sizeof(struct ndmsg)) +
+			RTA_SPACE(16); /* NDA_DST */
+	uint8_t buf[bufsize];
+	struct ndmsg *ndmsg = (struct ndmsg *) buf;
+	void *rta_buf = (void *) ndmsg + NLMSG_ALIGN(sizeof(struct ndmsg));
+	__auto_type cb_data = struct_alloc(rtnl_neighbour_get_data,
+						cb, user_data, destroy);
+	uint32_t ret;
+
+	memset(buf, 0, bufsize);
+	ndmsg->ndm_family = family;
+	ndmsg->ndm_ifindex = ifindex;
+	ndmsg->ndm_flags = 0;
+
+	rta_buf += rta_add_address(rta_buf, NDA_DST, family, addr, addr);
+
+	ret = l_netlink_send(rtnl, RTM_GETNEIGH, 0, ndmsg,
+				rta_buf - (void *) ndmsg,
+				rtnl_neighbour_get_cb, cb_data,
+				rtnl_neighbour_get_destroy_cb);
+	if (ret)
+		return ret;
+
+	l_free(cb_data);
+	return 0;
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbour_get_ipv4_hwaddr(struct l_netlink *rtnl,
+					int ifindex, uint32_t addr,
+					l_rtnl_neighbour_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	return rtnl_neighbour_get_hwaddr(rtnl, ifindex, AF_INET, &addr,
+						cb, user_data, destroy);
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbour_get_ipv6_hwaddr(struct l_netlink *rtnl,
+					int ifindex, const uint8_t *addr,
+					l_rtnl_neighbour_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	return rtnl_neighbour_get_hwaddr(rtnl, ifindex, AF_INET6, addr,
+						cb, user_data, destroy);
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbour_get_addr_hwaddr(struct l_netlink *rtnl,
+					int ifindex,
+					const struct l_rtnl_address *addr,
+					l_rtnl_neighbour_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	const uint8_t *ip = addr->family == AF_INET ?
+		(const void *) &addr->in_addr : (const void *) &addr->in6_addr;
+
+	return rtnl_neighbour_get_hwaddr(rtnl, ifindex, addr->family, ip,
+						cb, user_data, destroy);
+}
+
+static uint32_t rtnl_neighbour_set_hwaddr(struct l_netlink *rtnl,
+					int ifindex, uint8_t family,
+					const void *addr, const uint8_t *hwaddr,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	size_t bufsize = NLMSG_ALIGN(sizeof(struct ndmsg)) +
+			RTA_SPACE(16) +      /* NDA_DST */
+			RTA_SPACE(ETH_ALEN); /* NDA_LLADDR */
+	uint8_t buf[bufsize];
+	struct ndmsg *ndmsg = (struct ndmsg *) buf;
+	void *rta_buf = (void *) ndmsg + NLMSG_ALIGN(sizeof(struct ndmsg));
+
+	memset(buf, 0, bufsize);
+	ndmsg->ndm_family = family;
+	ndmsg->ndm_ifindex = ifindex;
+	ndmsg->ndm_flags = 0;
+	ndmsg->ndm_state = NUD_REACHABLE;
+
+	rta_buf += rta_add_address(rta_buf, NDA_DST, family, addr, addr);
+	rta_buf += rta_add_data(rta_buf, NDA_LLADDR, hwaddr, ETH_ALEN);
+
+	return l_netlink_send(rtnl, RTM_NEWNEIGH, NLM_F_CREATE | NLM_F_REPLACE,
+				ndmsg, rta_buf - (void *) ndmsg,
+				cb, user_data, destroy);
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbour_set_ipv4_hwaddr(struct l_netlink *rtnl,
+					int ifindex, uint32_t addr,
+					const uint8_t *hwaddr,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	return rtnl_neighbour_set_hwaddr(rtnl, ifindex, AF_INET, &addr,
+						hwaddr, cb, user_data, destroy);
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbour_set_ipv6_hwaddr(struct l_netlink *rtnl,
+					int ifindex, const uint8_t *addr,
+					const uint8_t *hwaddr,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	return rtnl_neighbour_set_hwaddr(rtnl, ifindex, AF_INET6, addr,
+						hwaddr, cb, user_data, destroy);
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbour_set_addr_hwaddr(struct l_netlink *rtnl,
+					int ifindex,
+					const struct l_rtnl_address *addr,
+					const uint8_t *hwaddr,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	const uint8_t *ip = addr->family == AF_INET ?
+		(const void *) &addr->in_addr : (const void *) &addr->in6_addr;
+
+	return rtnl_neighbour_set_hwaddr(rtnl, ifindex, addr->family, ip,
+						hwaddr, cb, user_data, destroy);
+}
diff --git a/ell/rtnl.h b/ell/rtnl.h
index 9ea24f5..834cd14 100644
--- a/ell/rtnl.h
+++ b/ell/rtnl.h
@@ -33,6 +33,9 @@ extern "C" {
 struct l_rtnl_address;
 struct l_rtnl_route;
 
+typedef void (*l_rtnl_neighbour_get_cb_t) (int error, const uint8_t *hwaddr,
+						void *user_data);
+
 struct l_rtnl_address *l_rtnl_address_new(const char *ip, uint8_t prefix_len);
 struct l_rtnl_address *l_rtnl_address_clone(const struct l_rtnl_address *orig);
 void l_rtnl_address_free(struct l_rtnl_address *addr);
@@ -191,6 +194,39 @@ uint32_t l_rtnl_route_delete(struct l_netlink *rtnl, int ifindex,
 					void *user_data,
 					l_netlink_destroy_func_t destroy);
 
+uint32_t l_rtnl_neighbour_get_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
+					uint32_t addr,
+					l_rtnl_neighbour_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+uint32_t l_rtnl_neighbour_get_ipv6_hwaddr(struct l_netlink *rtnl, int ifindex,
+					const uint8_t *addr,
+					l_rtnl_neighbour_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+uint32_t l_rtnl_neighbour_get_addr_hwaddr(struct l_netlink *rtnl, int ifindex,
+					const struct l_rtnl_address *addr,
+					l_rtnl_neighbour_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+uint32_t l_rtnl_neighbour_set_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
+					uint32_t addr, const uint8_t *hwaddr,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+uint32_t l_rtnl_neighbour_set_ipv6_hwaddr(struct l_netlink *rtnl, int ifindex,
+					const uint8_t *addr,
+					const uint8_t *hwaddr,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+uint32_t l_rtnl_neighbour_set_addr_hwaddr(struct l_netlink *rtnl, int ifindex,
+					const struct l_rtnl_address *addr,
+					const uint8_t *hwaddr,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/ell/useful.h b/ell/useful.h
index d696bc3..b4783ce 100644
--- a/ell/useful.h
+++ b/ell/useful.h
@@ -83,3 +83,7 @@ static inline int secure_select(int select_left, int l, int r)
 
 	return r ^ ((l ^ r) & mask);
 }
+
+#define struct_alloc(structname, ...) \
+	(struct structname *) l_memdup(&(struct structname) { __VA_ARGS__ }, \
+					sizeof(struct structname))
-- 
2.30.2

Re: [PATCH] rtnl: Add neighbor discovery utilities

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3388 bytes --]

Hi Andrew,

On 8/24/21 7:40 PM, Andrew Zaborowski wrote:
> Add a few variants of RTNL utilities to look up addresses in the
> kernel's neighbour table entries and to add new entries.  This is
> basically the ARP (for IPv4) and NDP (for IPv6) tables, that map IP
> addresses to hardware addresses.
> ---
> There are three variants of each method, I currently only have a use
> case for the ipv4 and ipv6 variants so I'm happy dropping the struct
> l_rtnl_address variant.
> 
>   ell/ell.sym  |   6 ++
>   ell/rtnl.c   | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/rtnl.h   |  36 ++++++++++
>   ell/useful.h |   4 ++
>   4 files changed, 240 insertions(+)
> 

<snip>

> diff --git a/ell/rtnl.h b/ell/rtnl.h
> index 9ea24f5..834cd14 100644
> --- a/ell/rtnl.h
> +++ b/ell/rtnl.h
> @@ -33,6 +33,9 @@ extern "C" {
>   struct l_rtnl_address;
>   struct l_rtnl_route;
>   
> +typedef void (*l_rtnl_neighbour_get_cb_t) (int error, const uint8_t *hwaddr,

You probably want a length as well, no?

> +						void *user_data);
> +
>   struct l_rtnl_address *l_rtnl_address_new(const char *ip, uint8_t prefix_len);
>   struct l_rtnl_address *l_rtnl_address_clone(const struct l_rtnl_address *orig);
>   void l_rtnl_address_free(struct l_rtnl_address *addr);
> @@ -191,6 +194,39 @@ uint32_t l_rtnl_route_delete(struct l_netlink *rtnl, int ifindex,
>   					void *user_data,
>   					l_netlink_destroy_func_t destroy);
>   
> +uint32_t l_rtnl_neighbour_get_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
> +					uint32_t addr,

We do not use uint32_t to represent an ipv4 anywhere in the ell API.

> +					l_rtnl_neighbour_get_cb_t cb,
> +					void *user_data,
> +					l_netlink_destroy_func_t destroy);
> +uint32_t l_rtnl_neighbour_get_ipv6_hwaddr(struct l_netlink *rtnl, int ifindex,
> +					const uint8_t *addr,

similarly to above, ipv6 is never represented like this.

I was hoping to avoid using in_addr / in6_addr in the public API, but maybe that 
would be the lesser evil option than 'const uint8_t *'.  Or just use 
l_rtnl_address for everything.

> +					l_rtnl_neighbour_get_cb_t cb,
> +					void *user_data,
> +					l_netlink_destroy_func_t destroy);
> +uint32_t l_rtnl_neighbour_get_addr_hwaddr(struct l_netlink *rtnl, int ifindex,
> +					const struct l_rtnl_address *addr,
> +					l_rtnl_neighbour_get_cb_t cb,
> +					void *user_data,
> +					l_netlink_destroy_func_t destroy);
> +uint32_t l_rtnl_neighbour_set_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
> +					uint32_t addr, const uint8_t *hwaddr,

You're making an assumption that hwaddr is always 6?  That will not really be 
true long term.  Might as well take care of this now.

> +					l_netlink_command_func_t cb,
> +					void *user_data,
> +					l_netlink_destroy_func_t destroy);
> +uint32_t l_rtnl_neighbour_set_ipv6_hwaddr(struct l_netlink *rtnl, int ifindex,
> +					const uint8_t *addr,
> +					const uint8_t *hwaddr,
> +					l_netlink_command_func_t cb,
> +					void *user_data,
> +					l_netlink_destroy_func_t destroy);
> +uint32_t l_rtnl_neighbour_set_addr_hwaddr(struct l_netlink *rtnl, int ifindex,
> +					const struct l_rtnl_address *addr,
> +					const uint8_t *hwaddr,
> +					l_netlink_command_func_t cb,
> +					void *user_data,
> +					l_netlink_destroy_func_t destroy);
> +

Regards,
-Denis

Re: [PATCH] rtnl: Add neighbor discovery utilities

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3911 bytes --]

Hi Denis,

On Wed, 25 Aug 2021 at 21:09, Denis Kenzior <denkenz@gmail.com> wrote:
> On 8/24/21 7:40 PM, Andrew Zaborowski wrote:
> > Add a few variants of RTNL utilities to look up addresses in the
> > kernel's neighbour table entries and to add new entries.  This is
> > basically the ARP (for IPv4) and NDP (for IPv6) tables, that map IP
> > addresses to hardware addresses.
> > ---
> > There are three variants of each method, I currently only have a use
> > case for the ipv4 and ipv6 variants so I'm happy dropping the struct
> > l_rtnl_address variant.
> >
> >   ell/ell.sym  |   6 ++
> >   ell/rtnl.c   | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   ell/rtnl.h   |  36 ++++++++++
> >   ell/useful.h |   4 ++
> >   4 files changed, 240 insertions(+)
> >
>
> <snip>
>
> > diff --git a/ell/rtnl.h b/ell/rtnl.h
> > index 9ea24f5..834cd14 100644
> > --- a/ell/rtnl.h
> > +++ b/ell/rtnl.h
> > @@ -33,6 +33,9 @@ extern "C" {
> >   struct l_rtnl_address;
> >   struct l_rtnl_route;
> >
> > +typedef void (*l_rtnl_neighbour_get_cb_t) (int error, const uint8_t *hwaddr,
>
> You probably want a length as well, no?
>
> > +                                             void *user_data);
> > +
> >   struct l_rtnl_address *l_rtnl_address_new(const char *ip, uint8_t prefix_len);
> >   struct l_rtnl_address *l_rtnl_address_clone(const struct l_rtnl_address *orig);
> >   void l_rtnl_address_free(struct l_rtnl_address *addr);
> > @@ -191,6 +194,39 @@ uint32_t l_rtnl_route_delete(struct l_netlink *rtnl, int ifindex,
> >                                       void *user_data,
> >                                       l_netlink_destroy_func_t destroy);
> >
> > +uint32_t l_rtnl_neighbour_get_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
> > +                                     uint32_t addr,
>
> We do not use uint32_t to represent an ipv4 anywhere in the ell API.
>
> > +                                     l_rtnl_neighbour_get_cb_t cb,
> > +                                     void *user_data,
> > +                                     l_netlink_destroy_func_t destroy);
> > +uint32_t l_rtnl_neighbour_get_ipv6_hwaddr(struct l_netlink *rtnl, int ifindex,
> > +                                     const uint8_t *addr,
>
> similarly to above, ipv6 is never represented like this.
>
> I was hoping to avoid using in_addr / in6_addr in the public API, but maybe that
> would be the lesser evil option than 'const uint8_t *'.  Or just use
> l_rtnl_address for everything.

My preference would be for in_addr / in6_addr, l_rtnl_address seems to
be overkill for a wrapper for a 32-bit value.

The only evil aspect of these that I can see is uint32_t's endianness
ambiguity (something similar to kernel's __be32 would have been
better) but the simplicity is a big advantage.

>
> > +                                     l_rtnl_neighbour_get_cb_t cb,
> > +                                     void *user_data,
> > +                                     l_netlink_destroy_func_t destroy);
> > +uint32_t l_rtnl_neighbour_get_addr_hwaddr(struct l_netlink *rtnl, int ifindex,
> > +                                     const struct l_rtnl_address *addr,
> > +                                     l_rtnl_neighbour_get_cb_t cb,
> > +                                     void *user_data,
> > +                                     l_netlink_destroy_func_t destroy);
> > +uint32_t l_rtnl_neighbour_set_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
> > +                                     uint32_t addr, const uint8_t *hwaddr,
>
> You're making an assumption that hwaddr is always 6?  That will not really be
> true long term.  Might as well take care of this now.

I am, but if the address is not 6 bytes the user will know it based on
the ifindex too.  We can be more explicit and include a length though.

Best regards

Re: [PATCH] rtnl: Add neighbor discovery utilities

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

Hi Andrew,

>> I was hoping to avoid using in_addr / in6_addr in the public API, but maybe that
>> would be the lesser evil option than 'const uint8_t *'.  Or just use
>> l_rtnl_address for everything.
> 
> My preference would be for in_addr / in6_addr, l_rtnl_address seems to
> be overkill for a wrapper for a 32-bit value.

I wouldn't actually mind seeing more context on how you're using these.  In 
theory you would need the rtnl address anyway, unless you're using this within 
dhcp server classes.  In which case you can provide a private API as well.

> 
> The only evil aspect of these that I can see is uint32_t's endianness
> ambiguity (something similar to kernel's __be32 would have been
> better) but the simplicity is a big advantage.
> 

Use of struct in_addr would imply network order is being used.  Same as 
inet_ntoa/inet_ntop, etc.

>>
>>> +                                     l_rtnl_neighbour_get_cb_t cb,
>>> +                                     void *user_data,
>>> +                                     l_netlink_destroy_func_t destroy);
>>> +uint32_t l_rtnl_neighbour_get_addr_hwaddr(struct l_netlink *rtnl, int ifindex,
>>> +                                     const struct l_rtnl_address *addr,
>>> +                                     l_rtnl_neighbour_get_cb_t cb,
>>> +                                     void *user_data,
>>> +                                     l_netlink_destroy_func_t destroy);
>>> +uint32_t l_rtnl_neighbour_set_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
>>> +                                     uint32_t addr, const uint8_t *hwaddr,
>>
>> You're making an assumption that hwaddr is always 6?  That will not really be
>> true long term.  Might as well take care of this now.
> 
> I am, but if the address is not 6 bytes the user will know it based on
> the ifindex too.  We can be more explicit and include a length though.

User might, but API implementation doesn't.  How does the implementation of this 
function know that ifindex is for a device that uses 8 byte hardware address? 
ifindex is just an int as far as it is concerned.  So unless you're going to 
introduce a netdev abstraction, hardware length would be needed here at some point.

Regards,
-Denis

Re: [PATCH] rtnl: Add neighbor discovery utilities

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]

On Thu, 26 Aug 2021 at 00:51, Denis Kenzior <denkenz@gmail.com> wrote:
> >> I was hoping to avoid using in_addr / in6_addr in the public API, but maybe that
> >> would be the lesser evil option than 'const uint8_t *'.  Or just use
> >> l_rtnl_address for everything.
> >
> > My preference would be for in_addr / in6_addr, l_rtnl_address seems to
> > be overkill for a wrapper for a 32-bit value.
>
> I wouldn't actually mind seeing more context on how you're using these.  In
> theory you would need the rtnl address anyway, unless you're using this within
> dhcp server classes.  In which case you can provide a private API as well.

This would be used in IWD like in
https://github.com/balrog-kun/iwd/commit/2da9ad4bb20a85f53fc459b5ec00dd90adbfe01f#diff-38b3dd2a52d30079b126664f14e4c72daab4887eb5d84aa07c684076db1f5e0cR278-R292

We only build an l_rtnl_address for our own IPs in netconfig.c.

>
> >
> > The only evil aspect of these that I can see is uint32_t's endianness
> > ambiguity (something similar to kernel's __be32 would have been
> > better) but the simplicity is a big advantage.
> >
>
> Use of struct in_addr would imply network order is being used.  Same as
> inet_ntoa/inet_ntop, etc.

Right, and same as __be32.  __be32 has the advantage that it's not a
struct so, 1. you know its size and contents and can pass its pointer
directly to, say, an attribute builder without looking up internal
structures, 2. don't need to convert to/from it by assigning cryptic
field names, 3. can pass it as parameter.

>
> >>
> >>> +                                     l_rtnl_neighbour_get_cb_t cb,
> >>> +                                     void *user_data,
> >>> +                                     l_netlink_destroy_func_t destroy);
> >>> +uint32_t l_rtnl_neighbour_get_addr_hwaddr(struct l_netlink *rtnl, int ifindex,
> >>> +                                     const struct l_rtnl_address *addr,
> >>> +                                     l_rtnl_neighbour_get_cb_t cb,
> >>> +                                     void *user_data,
> >>> +                                     l_netlink_destroy_func_t destroy);
> >>> +uint32_t l_rtnl_neighbour_set_ipv4_hwaddr(struct l_netlink *rtnl, int ifindex,
> >>> +                                     uint32_t addr, const uint8_t *hwaddr,
> >>
> >> You're making an assumption that hwaddr is always 6?  That will not really be
> >> true long term.  Might as well take care of this now.
> >
> > I am, but if the address is not 6 bytes the user will know it based on
> > the ifindex too.  We can be more explicit and include a length though.
>
> User might, but API implementation doesn't.  How does the implementation of this
> function know that ifindex is for a device that uses 8 byte hardware address?
> ifindex is just an int as far as it is concerned.  So unless you're going to
> introduce a netdev abstraction, hardware length would be needed here at some point.

True, we don't have netdev data directly in ell and would need to
either get it from the user or query it from the kernel if that is
ever a concern.

Best regards

Re: [PATCH] rtnl: Add neighbor discovery utilities

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

Hi Andrew,

>> Use of struct in_addr would imply network order is being used.  Same as
>> inet_ntoa/inet_ntop, etc.
> 
> Right, and same as __be32.  __be32 has the advantage that it's not a
> struct so, 1. you know its size and contents and can pass its pointer
> directly to, say, an attribute builder without looking up internal
> structures, 2. don't need to convert to/from it by assigning cryptic
> field names, 3. can pass it as parameter.

Some of what you say isn't really correct.  Look at the signature of inet_ntoa 
for example.

You're arguing for use of __be32 because you store ipv4 addresses as uints.  I 
get that.  But that doesn't help you with ipv6 and there is already a well 
defined structure to represent an ipv4 address.

Anyhow, given how you're planning to use this, I'd just adopt a signature 
similar to inet_ntop.  Something like:

l_rtnl_neighbor_set_hwaddr(rtnl, ifindex, int family, const void *, const 
uint8_t *hwaddr, size_t hwaddr_len, cb, user_data, destroy)

Also, US spelling of 'neighbor' would be preferred.

Regards,
-Denis

Re: [PATCH] rtnl: Add neighbor discovery utilities

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Hi Denis,

On Thu, 26 Aug 2021 at 15:59, Denis Kenzior <denkenz@gmail.com> wrote:
> >> Use of struct in_addr would imply network order is being used.  Same as
> >> inet_ntoa/inet_ntop, etc.
> >
> > Right, and same as __be32.  __be32 has the advantage that it's not a
> > struct so, 1. you know its size and contents and can pass its pointer
> > directly to, say, an attribute builder without looking up internal
> > structures, 2. don't need to convert to/from it by assigning cryptic
> > field names, 3. can pass it as parameter.
>
> Some of what you say isn't really correct.  Look at the signature of inet_ntoa
> for example.
>
> You're arguing for use of __be32 because you store ipv4 addresses as uints.  I
> get that.  But that doesn't help you with ipv6 and there is already a well
> defined structure to represent an ipv4 address.

I'd argue that an array for ipv6 is still more practical than the same
array wrapped in a struct (basically same thing as with IPv4 in_addr)
but ok.

>
> Anyhow, given how you're planning to use this, I'd just adopt a signature
> similar to inet_ntop.  Something like:
>
> l_rtnl_neighbor_set_hwaddr(rtnl, ifindex, int family, const void *, const
> uint8_t *hwaddr, size_t hwaddr_len, cb, user_data, destroy)

Ok.

>
> Also, US spelling of 'neighbor' would be preferred.

The reason I chose neighbour is because that is the name of the kernel
API but ok.

Best regards

Re: [PATCH] rtnl: Add neighbor discovery utilities

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

Hi Andrew,

On 8/26/21 5:47 PM, Andrew Zaborowski wrote:
> Add a few variants of RTNL utilities to look up addresses in the
> kernel's neighbour table entries and to add new entries.  This is
> basically the ARP (for IPv4) and NDP (for IPv6) tables, that map IP
> addresses to hardware addresses.
> ---
>   ell/ell.sym  |   2 +
>   ell/rtnl.c   | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/rtnl.h   |  17 +++++++
>   ell/useful.h |   4 ++
>   4 files changed, 150 insertions(+)
> 

Applied, thanks.

Regards,
-Denis

[PATCH] rtnl: Add neighbor discovery utilities

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 6487 bytes --]

Add a few variants of RTNL utilities to look up addresses in the
kernel's neighbour table entries and to add new entries.  This is
basically the ARP (for IPv4) and NDP (for IPv6) tables, that map IP
addresses to hardware addresses.
---
 ell/ell.sym  |   2 +
 ell/rtnl.c   | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/rtnl.h   |  17 +++++++
 ell/useful.h |   4 ++
 4 files changed, 150 insertions(+)

diff --git a/ell/ell.sym b/ell/ell.sym
index b29d98d..9fcf334 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -663,6 +663,8 @@ global:
 	l_rtnl_ifaddr_extract;
 	l_rtnl_ifaddr_add;
 	l_rtnl_ifaddr_delete;
+	l_rtnl_neighbor_get_hwaddr;
+	l_rtnl_neighbor_set_hwaddr;
 /* icmp6 */
 	l_icmp6_client_new;
 	l_icmp6_client_free;
diff --git a/ell/rtnl.c b/ell/rtnl.c
index 1061105..4a1a248 100644
--- a/ell/rtnl.c
+++ b/ell/rtnl.c
@@ -27,6 +27,9 @@
 #define _GNU_SOURCE
 #include <linux/if.h>
 #include <linux/icmpv6.h>
+#include <linux/neighbour.h>
+#include <linux/if_ether.h>
+#include <net/if_arp.h>
 #include <sys/socket.h>
 #include <arpa/inet.h>
 #include <errno.h>
@@ -34,6 +37,7 @@
 #include "useful.h"
 #include "netlink.h"
 #include "log.h"
+#include "util.h"
 #include "rtnl.h"
 #include "private.h"
 
@@ -1311,3 +1315,126 @@ LIB_EXPORT uint32_t l_rtnl_route_delete(struct l_netlink *rtnl, int ifindex,
 	return _rtnl_route_change(rtnl, RTM_DELROUTE, ifindex, rt,
 						cb, user_data, destroy);
 }
+
+struct rtnl_neighbor_get_data {
+	l_rtnl_neighbor_get_cb_t cb;
+	void *user_data;
+	l_netlink_destroy_func_t destroy;
+};
+
+static void rtnl_neighbor_get_cb(int error, uint16_t type, const void *data,
+					uint32_t len, void *user_data)
+{
+	struct rtnl_neighbor_get_data *cb_data = user_data;
+	const struct ndmsg *ndmsg = data;
+	struct rtattr *attr;
+	const uint8_t *hwaddr = NULL;
+	size_t hwaddr_len = 0;
+
+	if (error != 0)
+		goto done;
+
+	if (type != RTM_NEWNEIGH || len < NLMSG_ALIGN(sizeof(*ndmsg))) {
+		error = -EIO;
+		goto done;
+	}
+
+	if (!(ndmsg->ndm_state & (NUD_PERMANENT | NUD_NOARP | NUD_REACHABLE))) {
+		error = -ENOENT;
+		goto done;
+	}
+
+	attr = (void *) ndmsg + NLMSG_ALIGN(sizeof(*ndmsg));
+	len -= NLMSG_ALIGN(sizeof(*ndmsg));
+
+	for (; RTA_OK(attr, len); attr = RTA_NEXT(attr, len))
+		switch (attr->rta_type) {
+		case NDA_LLADDR:
+			hwaddr = RTA_DATA(attr);
+			hwaddr_len = RTA_PAYLOAD(attr);
+			break;
+		}
+
+	if (!hwaddr)
+		error = -EIO;
+
+done:
+	if (cb_data->cb) {
+		cb_data->cb(error, hwaddr, hwaddr_len, cb_data->user_data);
+		cb_data->cb = NULL;
+	}
+}
+
+static void rtnl_neighbor_get_destroy_cb(void *user_data)
+{
+	struct rtnl_neighbor_get_data *cb_data = user_data;
+
+	if (cb_data->destroy)
+		cb_data->destroy(cb_data->user_data);
+
+	l_free(cb_data);
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbor_get_hwaddr(struct l_netlink *rtnl,
+					int ifindex, int family,
+					const void *ip,
+					l_rtnl_neighbor_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	size_t bufsize = NLMSG_ALIGN(sizeof(struct ndmsg)) +
+			RTA_SPACE(16); /* NDA_DST */
+	uint8_t buf[bufsize];
+	struct ndmsg *ndmsg = (struct ndmsg *) buf;
+	void *rta_buf = (void *) ndmsg + NLMSG_ALIGN(sizeof(struct ndmsg));
+	__auto_type cb_data = struct_alloc(rtnl_neighbor_get_data,
+						cb, user_data, destroy);
+	uint32_t ret;
+
+	memset(buf, 0, bufsize);
+	ndmsg->ndm_family = family;
+	ndmsg->ndm_ifindex = ifindex;
+	ndmsg->ndm_flags = 0;
+
+	rta_buf += rta_add_address(rta_buf, NDA_DST, family, ip, ip);
+
+	ret = l_netlink_send(rtnl, RTM_GETNEIGH, 0, ndmsg,
+				rta_buf - (void *) ndmsg,
+				rtnl_neighbor_get_cb, cb_data,
+				rtnl_neighbor_get_destroy_cb);
+	if (ret)
+		return ret;
+
+	l_free(cb_data);
+	return 0;
+}
+
+LIB_EXPORT uint32_t l_rtnl_neighbor_set_hwaddr(struct l_netlink *rtnl,
+					int ifindex, int family,
+					const void *ip,
+					const uint8_t *hwaddr,
+					size_t hwaddr_len,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy)
+{
+	size_t bufsize = NLMSG_ALIGN(sizeof(struct ndmsg)) +
+			RTA_SPACE(16) +        /* NDA_DST */
+			RTA_SPACE(hwaddr_len); /* NDA_LLADDR */
+	uint8_t buf[bufsize];
+	struct ndmsg *ndmsg = (struct ndmsg *) buf;
+	void *rta_buf = (void *) ndmsg + NLMSG_ALIGN(sizeof(struct ndmsg));
+
+	memset(buf, 0, bufsize);
+	ndmsg->ndm_family = family;
+	ndmsg->ndm_ifindex = ifindex;
+	ndmsg->ndm_flags = 0;
+	ndmsg->ndm_state = NUD_REACHABLE;
+
+	rta_buf += rta_add_address(rta_buf, NDA_DST, family, ip, ip);
+	rta_buf += rta_add_data(rta_buf, NDA_LLADDR, hwaddr, hwaddr_len);
+
+	return l_netlink_send(rtnl, RTM_NEWNEIGH, NLM_F_CREATE | NLM_F_REPLACE,
+				ndmsg, rta_buf - (void *) ndmsg,
+				cb, user_data, destroy);
+}
diff --git a/ell/rtnl.h b/ell/rtnl.h
index 9ea24f5..85b1370 100644
--- a/ell/rtnl.h
+++ b/ell/rtnl.h
@@ -33,6 +33,10 @@ extern "C" {
 struct l_rtnl_address;
 struct l_rtnl_route;
 
+typedef void (*l_rtnl_neighbor_get_cb_t) (int error, const uint8_t *hwaddr,
+						size_t hwaddr_len,
+						void *user_data);
+
 struct l_rtnl_address *l_rtnl_address_new(const char *ip, uint8_t prefix_len);
 struct l_rtnl_address *l_rtnl_address_clone(const struct l_rtnl_address *orig);
 void l_rtnl_address_free(struct l_rtnl_address *addr);
@@ -191,6 +195,19 @@ uint32_t l_rtnl_route_delete(struct l_netlink *rtnl, int ifindex,
 					void *user_data,
 					l_netlink_destroy_func_t destroy);
 
+uint32_t l_rtnl_neighbor_get_hwaddr(struct l_netlink *rtnl, int ifindex,
+					int family, const void *ip,
+					l_rtnl_neighbor_get_cb_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+uint32_t l_rtnl_neighbor_set_hwaddr(struct l_netlink *rtnl, int ifindex,
+					int family, const void *ip,
+					const uint8_t *hwaddr,
+					size_t hwaddr_len,
+					l_netlink_command_func_t cb,
+					void *user_data,
+					l_netlink_destroy_func_t destroy);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/ell/useful.h b/ell/useful.h
index d696bc3..b4783ce 100644
--- a/ell/useful.h
+++ b/ell/useful.h
@@ -83,3 +83,7 @@ static inline int secure_select(int select_left, int l, int r)
 
 	return r ^ ((l ^ r) & mask);
 }
+
+#define struct_alloc(structname, ...) \
+	(struct structname *) l_memdup(&(struct structname) { __VA_ARGS__ }, \
+					sizeof(struct structname))
-- 
2.30.2