[PATCH 0/2] Fix equence collision with neighbour notification, and one nit

[PATCH 0/2] Fix equence collision with neighbour notification, and one nit

[Stefano Brivio]

Patch 1/2 fixes https://bugs.passt.top/show_bug.cgi?id=203, while
patch 2/2 is a cosmetic follow-up.

Stefano Brivio (2):
  netlink: Use regular request/response netlink socket for initial
    neighbour sync
  netlink: Fix comments to variables for netlink sockets and sequence

 netlink.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH 1/2] netlink: Use regular request/response netlink socket for initial neighbour sync

[Stefano Brivio]

...instead of the one dedicated to the neighbour monitor, because, if
neighbour notifications start coming in before or while we send the
initial request to read out the neighbour tables, messages and
sequence numbers will collide.

For example, if nl_neigh_sync() sends a RTM_GETNEIGH request with
sequence 20, we expect a corresponding reply with sequence 20. But
given that we already used the same socket to subscribe to
notifications, and notifications don't correspond to any specific
request we sent, we might now get a message with sequence 0.

The collision between messages wouldn't actually matter, as we'll
handle anyway any RTM_NEWNEIGH message in the same fashion, but we
need to validate sequence numbers for robustness, and that will fail.

At the same time, we have to subscribe to neighbour notifications
before calling nl_neigh_sync(), because we'll have a race condition
otherwise, as we might miss neighbours that were added before the
notifier is registered.

Use the regular nl_sock for nl_neigh_sync().

Drop the interface index from the request: we won't get any entry
otherwise, because the Linux kernel (as of version 7.0) is unable to
filter on it. Results are now filtered by interface index as we read
them.

Passing along an interface index used to work when nl_neigh_sync()
used the notifier socket, because NETLINK_GET_STRICT_CHK is not set
on it, meaning that results weren't filtered at all (interface and
IP version passed in the request were ignored altogether).

To reproduce the issue fixed here:

* detach a network and user namespace:

  [terminal 0]
  $ unshare -rUn
  # echo $$
  1543307

* attach pasta to it:

  [terminal 1]
  $ ./pasta -f --config-net 1543307 -I enp9s0

* enter that namespace from yet another terminal:

  [terminal 2]
  $ nsenter --preserve-credentials -U -n -t 1543307

* start flooding the MAC address table of this namespace:

  [terminal 1]
  # for i in $(seq 10 99); do for j in $(seq 10 99); do for k in $(seq 10 99); do ip ne add dev enp9s0 10.$i.$j.$k lladdr 00:11:22:$i:$j:$k; done; done; done

* and now start another instance of pasta in this namespace:

  [terminal 2]
  # ./pasta -d --config-net

which will eventually result in pasta exiting with a message like:

  0.0253: netlink: Unexpected sequence number (0 != 34)

Reported-by: Paul Holzinger <pholzing@redhat.com>
Link: https://bugs.passt.top/show_bug.cgi?id=203
Fixes: 3c469013cfaa ("netlink: add subscription on changes in NDP/ARP table")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 netlink.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/netlink.c b/netlink.c
index c3c830e..0863734 100644
--- a/netlink.c
+++ b/netlink.c
@@ -1206,24 +1206,23 @@ static void nl_neigh_msg_read(const struct ctx *c, struct nlmsghdr *nh)
  * @proto:	Protocol, AF_INET or AF_INET6
  * @ifi:	Interface index
  */
-static void nl_neigh_sync(const struct ctx *c, int proto, int ifi)
+static void nl_neigh_sync(const struct ctx *c, int proto)
 {
 	struct {
 		struct nlmsghdr nlh;
 		struct ndmsg    ndm;
 	} req = {
-		.ndm.ndm_family  = proto,
-		.ndm.ndm_ifindex = ifi,
+		.ndm.ndm_family = proto,
 	};
 	struct nlmsghdr *nh;
 	char buf[NLBUFSIZ];
 	ssize_t status;
 	uint32_t seq;
 
-	seq = nl_send(nl_sock_neigh, &req, RTM_GETNEIGH,
-		      NLM_F_DUMP, sizeof(req));
-	nl_foreach_oftype(nh, status, nl_sock_neigh, buf, seq, RTM_NEWNEIGH)
+	seq = nl_send(nl_sock, &req, RTM_GETNEIGH, NLM_F_DUMP, sizeof(req));
+	nl_foreach_oftype(nh, status, nl_sock, buf, seq, RTM_NEWNEIGH)
 		nl_neigh_msg_read(c, nh);
+
 	if (status < 0)
 		warn("netlink: RTM_GETNEIGH failed: %s", strerror_(-status));
 }
@@ -1298,8 +1297,8 @@ int nl_neigh_notify_init(const struct ctx *c)
 		return -1;
 	}
 
-	nl_neigh_sync(c, AF_INET, c->ifi4);
-	nl_neigh_sync(c, AF_INET6, c->ifi6);
+	nl_neigh_sync(c, AF_INET);
+	nl_neigh_sync(c, AF_INET6);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH 1/2] netlink: Use regular request/response netlink socket for initial neighbour sync

[David Gibson]

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

On Thu, May 21, 2026 at 08:01:45PM +0200, Stefano Brivio wrote:
> ...instead of the one dedicated to the neighbour monitor, because, if
> neighbour notifications start coming in before or while we send the
> initial request to read out the neighbour tables, messages and
> sequence numbers will collide.
> 
> For example, if nl_neigh_sync() sends a RTM_GETNEIGH request with
> sequence 20, we expect a corresponding reply with sequence 20. But
> given that we already used the same socket to subscribe to
> notifications, and notifications don't correspond to any specific
> request we sent, we might now get a message with sequence 0.

Heh.  Called it, kinda.  Nice job tracking this down.

> The collision between messages wouldn't actually matter, as we'll
> handle anyway any RTM_NEWNEIGH message in the same fashion, but we
> need to validate sequence numbers for robustness, and that will fail.
> 
> At the same time, we have to subscribe to neighbour notifications
> before calling nl_neigh_sync(), because we'll have a race condition
> otherwise, as we might miss neighbours that were added before the
> notifier is registered.
> 
> Use the regular nl_sock for nl_neigh_sync().
> 
> Drop the interface index from the request: we won't get any entry
> otherwise, because the Linux kernel (as of version 7.0) is unable to
> filter on it. Results are now filtered by interface index as we read
> them.
> 
> Passing along an interface index used to work when nl_neigh_sync()
> used the notifier socket, because NETLINK_GET_STRICT_CHK is not set
> on it, meaning that results weren't filtered at all (interface and
> IP version passed in the request were ignored altogether).
> 
> To reproduce the issue fixed here:
> 
> * detach a network and user namespace:
> 
>   [terminal 0]
>   $ unshare -rUn
>   # echo $$
>   1543307
> 
> * attach pasta to it:
> 
>   [terminal 1]
>   $ ./pasta -f --config-net 1543307 -I enp9s0
> 
> * enter that namespace from yet another terminal:
> 
>   [terminal 2]
>   $ nsenter --preserve-credentials -U -n -t 1543307
> 
> * start flooding the MAC address table of this namespace:
> 
>   [terminal 1]
>   # for i in $(seq 10 99); do for j in $(seq 10 99); do for k in $(seq 10 99); do ip ne add dev enp9s0 10.$i.$j.$k lladdr 00:11:22:$i:$j:$k; done; done; done
> 
> * and now start another instance of pasta in this namespace:
> 
>   [terminal 2]
>   # ./pasta -d --config-net
> 
> which will eventually result in pasta exiting with a message like:
> 
>   0.0253: netlink: Unexpected sequence number (0 != 34)
> 
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Link: https://bugs.passt.top/show_bug.cgi?id=203
> Fixes: 3c469013cfaa ("netlink: add subscription on changes in NDP/ARP table")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  netlink.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index c3c830e..0863734 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -1206,24 +1206,23 @@ static void nl_neigh_msg_read(const struct ctx *c, struct nlmsghdr *nh)
>   * @proto:	Protocol, AF_INET or AF_INET6
>   * @ifi:	Interface index
>   */
> -static void nl_neigh_sync(const struct ctx *c, int proto, int ifi)
> +static void nl_neigh_sync(const struct ctx *c, int proto)
>  {
>  	struct {
>  		struct nlmsghdr nlh;
>  		struct ndmsg    ndm;
>  	} req = {
> -		.ndm.ndm_family  = proto,
> -		.ndm.ndm_ifindex = ifi,
> +		.ndm.ndm_family = proto,
>  	};
>  	struct nlmsghdr *nh;
>  	char buf[NLBUFSIZ];
>  	ssize_t status;
>  	uint32_t seq;
>  
> -	seq = nl_send(nl_sock_neigh, &req, RTM_GETNEIGH,
> -		      NLM_F_DUMP, sizeof(req));
> -	nl_foreach_oftype(nh, status, nl_sock_neigh, buf, seq, RTM_NEWNEIGH)
> +	seq = nl_send(nl_sock, &req, RTM_GETNEIGH, NLM_F_DUMP, sizeof(req));
> +	nl_foreach_oftype(nh, status, nl_sock, buf, seq, RTM_NEWNEIGH)
>  		nl_neigh_msg_read(c, nh);
> +
>  	if (status < 0)
>  		warn("netlink: RTM_GETNEIGH failed: %s", strerror_(-status));
>  }
> @@ -1298,8 +1297,8 @@ int nl_neigh_notify_init(const struct ctx *c)
>  		return -1;
>  	}
>  
> -	nl_neigh_sync(c, AF_INET, c->ifi4);
> -	nl_neigh_sync(c, AF_INET6, c->ifi6);
> +	nl_neigh_sync(c, AF_INET);
> +	nl_neigh_sync(c, AF_INET6);
>  
>  	return 0;
>  }
> -- 
> 2.43.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] netlink: Fix comments to variables for netlink sockets and sequence

[Stefano Brivio]

Commit 3c469013cfaa ("netlink: add subscription on changes in NDP/ARP
table") added the descriptor for the new socket, int nl_sock_neigh,
next to the variables for the existing sockets, without updating the
comment, and before the variable reserved for the sequence number.

That seems to suggest that the sequence number applies to the notifier
socket as well, but that's not the case. Further, the comment didn't
match anymore.

Move the variable declaration for nl_sock_neigh below, and add a
separate comment for it, actually describing it. While at it, fix the
indentation.

Fixes: 3c469013cfaa ("netlink: add subscription on changes in NDP/ARP table")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 netlink.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/netlink.c b/netlink.c
index 0863734..8d20dbb 100644
--- a/netlink.c
+++ b/netlink.c
@@ -56,10 +56,12 @@
 #define NLBUFSIZ 65536
 
 /* Socket in init, in target namespace, sequence (just needs to be monotonic) */
-int nl_sock		 = -1;
-int nl_sock_ns		 = -1;
-static int nl_sock_neigh = -1;
-static int nl_seq	 = 1;
+int nl_sock			= -1;
+int nl_sock_ns			= -1;
+static int nl_seq		=  1;
+
+/* Socket for neighbour event notifier */
+static int nl_sock_neigh	= -1;
 
 /**
  * nl_sock_init_do() - Set up netlink sockets in init or target namespace
-- 
2.43.0

Re: [PATCH 2/2] netlink: Fix comments to variables for netlink sockets and sequence

[David Gibson]

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

On Thu, May 21, 2026 at 08:01:46PM +0200, Stefano Brivio wrote:
11;rgb:ffff/ffff/ffff> Commit 3c469013cfaa ("netlink: add subscription on changes in NDP/ARP
> table") added the descriptor for the new socket, int nl_sock_neigh,
> next to the variables for the existing sockets, without updating the
> comment, and before the variable reserved for the sequence number.
> 
> That seems to suggest that the sequence number applies to the notifier
> socket as well, but that's not the case. Further, the comment didn't
> match anymore.
> 
> Move the variable declaration for nl_sock_neigh below, and add a
> separate comment for it, actually describing it. While at it, fix the
> indentation.
> 
> Fixes: 3c469013cfaa ("netlink: add subscription on changes in NDP/ARP table")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  netlink.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index 0863734..8d20dbb 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -56,10 +56,12 @@
>  #define NLBUFSIZ 65536
>  
>  /* Socket in init, in target namespace, sequence (just needs to be monotonic) */
> -int nl_sock		 = -1;
> -int nl_sock_ns		 = -1;
> -static int nl_sock_neigh = -1;
> -static int nl_seq	 = 1;
> +int nl_sock			= -1;
> +int nl_sock_ns			= -1;
> +static int nl_seq		=  1;
> +
> +/* Socket for neighbour event notifier */
> +static int nl_sock_neigh	= -1;
>  
>  /**
>   * nl_sock_init_do() - Set up netlink sockets in init or target namespace
> -- 
> 2.43.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]