[iproute PATCH 0/6] flush many addresses and some cleanups

[iproute PATCH 0/6] flush many addresses and some cleanups

[Phil Sutter]

This patch series aims to silence spurious error messages when flushing
an interface with many addresses assigned and provides a few cleanups
found along the way.

The original issue was a test-case adding 40k IPv4 addresses to an
interface and calling 'ip addr flush' afterwards. Initially, this
resulted in an error message:

| Failed to send flush request: Cannot assign requested address

Iproute apparently tried to remove (secondary) addresses which didn't
exist anymore since the primary one had already been removed. (This
behaviour actually depends on the promote_secondaries sysctl setting.)
Patch 1/6 makes iproute simply ignore this error, as when flushing it is
not relevant. This also allows to remove iproute's previous workaround,
which is to flush secondary addresses before primary ones (patch 2/6).

Yet, still an error message is emitted on newer kernels, as they started
to check consistency of netlink dumps (which is broken by iproute as it
alters the data while it is dumped). This error may as well be ignored,
as it's expected behaviour while flushing addresses. Though in order to
do that, libnetlink API had to be extended a bit to actually allow to
ignore certain nlmsg_flags bits. Patch 3/6 therefore extends libnetlink,
patch 4/6 then makes use of the extended functionality.

While debugging the issue, an unnecessary check has been discovered
(patch 5/6) as well as a possible simplification in iptoken.c was found
(patch 6/6).

Phil Sutter (6):
  ipaddress: make flush command more error-tolerant
  ipaddress: simplify ipaddr_flush()
  libnetlink: introduce nc_flags
  ipaddress: fix ipaddr_flush for Linux >= 3.1
  ipaddress: drop unnecessary check in ipaddr_list_flush_or_save()
  iptoken: simplify iptoken_list a bit

 include/libnetlink.h |  7 ++++++-
 ip/ipaddress.c       | 51 +++++++++------------------------------------------
 ip/iptoken.c         |  6 +-----
 lib/libnetlink.c     | 10 ++++++----
 4 files changed, 22 insertions(+), 52 deletions(-)

-- 
2.1.2

[iproute PATCH 1/6] ipaddress: make flush command more error-tolerant

[Phil Sutter]

The core issue here is that with promote_secondaries sysctl setting
being turned off, removing the primary address implicitly removes all
secondaries as well. Iproute is aware of this and therefore tries to
remove all secondary addresses first to circumvent errors due to
removing non-existent addresses. But this works only if not too many IP
addresses are assigned to an interface, otherwise the RTM_GETADDR
response is split up into multiple buffers. In my test-case, flushing
more than 42 IPv4 addresses was sufficient to trigger an error:

Failed to send flush request: Cannot assign requested address

This patch fixes the issue by simply ignoring EADDRNOTAVAIL when
flushing.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index f290205..75b3e27 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -893,7 +893,12 @@ int print_linkinfo(const struct sockaddr_nl *who,
 
 static int flush_update(void)
 {
-	if (rtnl_send_check(&rth, filter.flushb, filter.flushp) < 0) {
+	int rc = rtnl_send_check(&rth, filter.flushb, filter.flushp);
+
+	/* if promote_secondaries sysctl setting is off, removing the primary
+	 * address makes us try to remove non-existent secondaries. Since we're
+	 * flushing, this can be sanely ignored. */
+	if (rc < 0 && errno != EADDRNOTAVAIL) {
 		perror("Failed to send flush request");
 		return -1;
 	}
-- 
2.1.2

[iproute PATCH 2/6] ipaddress: simplify ipaddr_flush()

[Phil Sutter]

Since it's no longer relevant whether an IP address is primary or
secondary when flushing, ipaddr_flush() can be simplified a bit.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 38 +-------------------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 75b3e27..b5b444b 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1143,28 +1143,6 @@ brief_exit:
 	return 0;
 }
 
-static int print_addrinfo_primary(const struct sockaddr_nl *who,
-				  struct nlmsghdr *n, void *arg)
-{
-	struct ifaddrmsg *ifa = NLMSG_DATA(n);
-
-	if (ifa->ifa_flags & IFA_F_SECONDARY)
-		return 0;
-
-	return print_addrinfo(who, n, arg);
-}
-
-static int print_addrinfo_secondary(const struct sockaddr_nl *who,
-				    struct nlmsghdr *n, void *arg)
-{
-	struct ifaddrmsg *ifa = NLMSG_DATA(n);
-
-	if (!(ifa->ifa_flags & IFA_F_SECONDARY))
-		return 0;
-
-	return print_addrinfo(who, n, arg);
-}
-
 struct nlmsg_list
 {
 	struct nlmsg_list *next;
@@ -1415,26 +1393,12 @@ static int ipaddr_flush(void)
 	filter.flushe = sizeof(flushb);
 
 	while ((max_flush_loops == 0) || (round < max_flush_loops)) {
-		const struct rtnl_dump_filter_arg a[3] = {
-			{
-				.filter = print_addrinfo_secondary,
-				.arg1 = stdout,
-			},
-			{
-				.filter = print_addrinfo_primary,
-				.arg1 = stdout,
-			},
-			{
-				.filter = NULL,
-				.arg1 = NULL,
-			},
-		};
 		if (rtnl_wilddump_request(&rth, filter.family, RTM_GETADDR) < 0) {
 			perror("Cannot send dump request");
 			exit(1);
 		}
 		filter.flushed = 0;
-		if (rtnl_dump_filter_l(&rth, a) < 0) {
+		if (rtnl_dump_filter(&rth, print_addrinfo, stdout) < 0) {
 			fprintf(stderr, "Flush terminated\n");
 			exit(1);
 		}
-- 
2.1.2

[iproute PATCH 3/6] libnetlink: introduce nc_flags

[Phil Sutter]

Allow for a filter to ignore certain nlmsg_flags.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/libnetlink.h |  7 ++++++-
 lib/libnetlink.c     | 10 ++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index 2280c39..38e54f4 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -60,11 +60,16 @@ struct rtnl_dump_filter_arg
 {
 	rtnl_filter_t filter;
 	void *arg1;
+	__u16 nc_flags;
 };
 
 int rtnl_dump_filter_l(struct rtnl_handle *rth,
 			      const struct rtnl_dump_filter_arg *arg);
-int rtnl_dump_filter(struct rtnl_handle *rth, rtnl_filter_t filter, void *arg);
+int rtnl_dump_filter_nc(struct rtnl_handle *rth,
+			rtnl_filter_t filter,
+			void *arg, __u16 nc_flags);
+#define rtnl_dump_filter(rth, filter, arg) \
+	rtnl_dump_fliter_nc(rth, filter, arg, 0)
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr *answer, size_t len)
 	__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 09b0e91..922ec2d 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -259,6 +259,8 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 			while (NLMSG_OK(h, msglen)) {
 				int err = 0;
 
+				h->nlmsg_flags &= ~a->nc_flags;
+
 				if (nladdr.nl_pid != 0 ||
 				    h->nlmsg_pid != rth->local.nl_pid ||
 				    h->nlmsg_seq != rth->dump)
@@ -317,13 +319,13 @@ skip_it:
 	}
 }
 
-int rtnl_dump_filter(struct rtnl_handle *rth,
+int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		     rtnl_filter_t filter,
-		     void *arg1)
+		     void *arg1, __u16 nc_flags)
 {
 	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = filter, .arg1 = arg1, },
-		{ .filter = NULL,   .arg1 = NULL, },
+		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
 	};
 
 	return rtnl_dump_filter_l(rth, a);
-- 
2.1.2

[iproute PATCH 4/6] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Phil Sutter]

Linux version 3.1 introduced a consistency check for netlink dumps in
commit 670dc28 ("netlink: advertise incomplete dumps"). This bites
iproute2 when flushing more addresses than can fit into a single
RTM_GETADDR response. To silence the spurious error message "Dump was
interrupted and may be inconsistent.", advise rtnl_dump_filter_l() to
not care about NLM_F_DUMP_INTR.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b5b444b..23e0308 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1398,7 +1398,8 @@ static int ipaddr_flush(void)
 			exit(1);
 		}
 		filter.flushed = 0;
-		if (rtnl_dump_filter(&rth, print_addrinfo, stdout) < 0) {
+		if (rtnl_dump_filter_nc(&rth, print_addrinfo,
+					stdout, NLM_F_DUMP_INTR) < 0) {
 			fprintf(stderr, "Flush terminated\n");
 			exit(1);
 		}
-- 
2.1.2

[iproute PATCH 5/6] ipaddress: drop unnecessary check in ipaddr_list_flush_or_save()

[Phil Sutter]

Right after ipaddr_reset_filter(), filter.family is always AF_UNSPEC.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 23e0308..ff7f2a8 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1446,10 +1446,7 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 
 	ipaddr_reset_filter(oneline, 0);
 	filter.showqueue = 1;
-
-	if (filter.family == AF_UNSPEC)
-		filter.family = preferred_family;
-
+	filter.family = preferred_family;
 	filter.group = -1;
 
 	if (action == IPADD_FLUSH) {
-- 
2.1.2

[iproute PATCH 6/6] iptoken: simplify iptoken_list a bit

[Phil Sutter]

Since it uses only a single filter, rtnl_dump_filter() can be used.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptoken.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/ip/iptoken.c b/ip/iptoken.c
index a38194c..428f133 100644
--- a/ip/iptoken.c
+++ b/ip/iptoken.c
@@ -95,10 +95,6 @@ static int iptoken_list(int argc, char **argv)
 {
 	int af = AF_INET6;
 	struct rtnl_dump_args da;
-	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = print_token, .arg1 = &da, },
-		{ .filter = NULL, .arg1 = NULL, },
-	};
 
 	memset(&da, 0, sizeof(da));
 	da.fp = stdout;
@@ -118,7 +114,7 @@ static int iptoken_list(int argc, char **argv)
 		return -1;
 	}
 
-	if (rtnl_dump_filter_l(&rth, a) < 0) {
+	if (rtnl_dump_filter(&rth, print_token, &da) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -1;
 	}
-- 
2.1.2

[iproute PATCH v2 0/6] flush many addresses and some cleanups

[Phil Sutter]

This patch series aims to silence spurious error messages when flushing
an interface with many addresses assigned and provides a few cleanups
found along the way.

The original issue was a test-case adding 40k IPv4 addresses to an
interface and calling 'ip addr flush' afterwards. Initially, this
resulted in an error message:

| Failed to send flush request: Cannot assign requested address

Iproute apparently tried to remove (secondary) addresses which didn't
exist anymore since the primary one had already been removed. (This
behaviour actually depends on the promote_secondaries sysctl setting.)
Patch 1/6 makes iproute simply ignore this error, as when flushing it is
not relevant. This also allows to remove iproute's previous workaround,
which is to flush secondary addresses before primary ones (patch 2/6).

Yet, still an error message is emitted on newer kernels, as they started
to check consistency of netlink dumps (which is broken by iproute as it
alters the data while it is dumped). This error may as well be ignored,
as it's expected behaviour while flushing addresses. Though in order to
do that, libnetlink API had to be extended a bit to actually allow to
ignore certain nlmsg_flags bits. Patch 3/6 therefore extends libnetlink,
patch 4/6 then makes use of the extended functionality.

While debugging the issue, an unnecessary check has been discovered
(patch 5/6) as well as a possible simplification in iptoken.c was found
(patch 6/6).

Please note that this series was built upon previously sent in patch
"ip_common.h header cleanup".

Changes since v1:
- Add forgotten hint about dependent patch (see above).
- Fix typo in patch 3/6 (sorry, I forgot to test-build a last-minute
  cleanup).
- All other patches remain unchanged.

Phil Sutter (6):
  ipaddress: make flush command more error-tolerant
  ipaddress: simplify ipaddr_flush()
  libnetlink: introduce nc_flags
  ipaddress: fix ipaddr_flush for Linux >= 3.1
  ipaddress: drop unnecessary check in ipaddr_list_flush_or_save()
  iptoken: simplify iptoken_list a bit

 include/libnetlink.h |  7 ++++++-
 ip/ipaddress.c       | 51 +++++++++------------------------------------------
 ip/iptoken.c         |  6 +-----
 lib/libnetlink.c     | 10 ++++++----
 4 files changed, 22 insertions(+), 52 deletions(-)

-- 
2.1.2

[iproute PATCH v2 1/6] ipaddress: make flush command more error-tolerant

[Phil Sutter]

The core issue here is that with promote_secondaries sysctl setting
being turned off, removing the primary address implicitly removes all
secondaries as well. Iproute is aware of this and therefore tries to
remove all secondary addresses first to circumvent errors due to
removing non-existent addresses. But this works only if not too many IP
addresses are assigned to an interface, otherwise the RTM_GETADDR
response is split up into multiple buffers. In my test-case, flushing
more than 42 IPv4 addresses was sufficient to trigger an error:

Failed to send flush request: Cannot assign requested address

This patch fixes the issue by simply ignoring EADDRNOTAVAIL when
flushing.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index f290205..75b3e27 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -893,7 +893,12 @@ int print_linkinfo(const struct sockaddr_nl *who,
 
 static int flush_update(void)
 {
-	if (rtnl_send_check(&rth, filter.flushb, filter.flushp) < 0) {
+	int rc = rtnl_send_check(&rth, filter.flushb, filter.flushp);
+
+	/* if promote_secondaries sysctl setting is off, removing the primary
+	 * address makes us try to remove non-existent secondaries. Since we're
+	 * flushing, this can be sanely ignored. */
+	if (rc < 0 && errno != EADDRNOTAVAIL) {
 		perror("Failed to send flush request");
 		return -1;
 	}
-- 
2.1.2

[iproute PATCH v2 2/6] ipaddress: simplify ipaddr_flush()

[Phil Sutter]

Since it's no longer relevant whether an IP address is primary or
secondary when flushing, ipaddr_flush() can be simplified a bit.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 38 +-------------------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 75b3e27..b5b444b 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1143,28 +1143,6 @@ brief_exit:
 	return 0;
 }
 
-static int print_addrinfo_primary(const struct sockaddr_nl *who,
-				  struct nlmsghdr *n, void *arg)
-{
-	struct ifaddrmsg *ifa = NLMSG_DATA(n);
-
-	if (ifa->ifa_flags & IFA_F_SECONDARY)
-		return 0;
-
-	return print_addrinfo(who, n, arg);
-}
-
-static int print_addrinfo_secondary(const struct sockaddr_nl *who,
-				    struct nlmsghdr *n, void *arg)
-{
-	struct ifaddrmsg *ifa = NLMSG_DATA(n);
-
-	if (!(ifa->ifa_flags & IFA_F_SECONDARY))
-		return 0;
-
-	return print_addrinfo(who, n, arg);
-}
-
 struct nlmsg_list
 {
 	struct nlmsg_list *next;
@@ -1415,26 +1393,12 @@ static int ipaddr_flush(void)
 	filter.flushe = sizeof(flushb);
 
 	while ((max_flush_loops == 0) || (round < max_flush_loops)) {
-		const struct rtnl_dump_filter_arg a[3] = {
-			{
-				.filter = print_addrinfo_secondary,
-				.arg1 = stdout,
-			},
-			{
-				.filter = print_addrinfo_primary,
-				.arg1 = stdout,
-			},
-			{
-				.filter = NULL,
-				.arg1 = NULL,
-			},
-		};
 		if (rtnl_wilddump_request(&rth, filter.family, RTM_GETADDR) < 0) {
 			perror("Cannot send dump request");
 			exit(1);
 		}
 		filter.flushed = 0;
-		if (rtnl_dump_filter_l(&rth, a) < 0) {
+		if (rtnl_dump_filter(&rth, print_addrinfo, stdout) < 0) {
 			fprintf(stderr, "Flush terminated\n");
 			exit(1);
 		}
-- 
2.1.2

[iproute PATCH v2 3/6] libnetlink: introduce nc_flags

[Phil Sutter]

Allow for a filter to ignore certain nlmsg_flags.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changed since v1:
- Fix typo in #define of rtnl_dump_filter().

 include/libnetlink.h |  7 ++++++-
 lib/libnetlink.c     | 10 ++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index 2280c39..431189e 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -60,11 +60,16 @@ struct rtnl_dump_filter_arg
 {
 	rtnl_filter_t filter;
 	void *arg1;
+	__u16 nc_flags;
 };
 
 int rtnl_dump_filter_l(struct rtnl_handle *rth,
 			      const struct rtnl_dump_filter_arg *arg);
-int rtnl_dump_filter(struct rtnl_handle *rth, rtnl_filter_t filter, void *arg);
+int rtnl_dump_filter_nc(struct rtnl_handle *rth,
+			rtnl_filter_t filter,
+			void *arg, __u16 nc_flags);
+#define rtnl_dump_filter(rth, filter, arg) \
+	rtnl_dump_filter_nc(rth, filter, arg, 0)
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr *answer, size_t len)
 	__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 09b0e91..922ec2d 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -259,6 +259,8 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 			while (NLMSG_OK(h, msglen)) {
 				int err = 0;
 
+				h->nlmsg_flags &= ~a->nc_flags;
+
 				if (nladdr.nl_pid != 0 ||
 				    h->nlmsg_pid != rth->local.nl_pid ||
 				    h->nlmsg_seq != rth->dump)
@@ -317,13 +319,13 @@ skip_it:
 	}
 }
 
-int rtnl_dump_filter(struct rtnl_handle *rth,
+int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		     rtnl_filter_t filter,
-		     void *arg1)
+		     void *arg1, __u16 nc_flags)
 {
 	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = filter, .arg1 = arg1, },
-		{ .filter = NULL,   .arg1 = NULL, },
+		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
 	};
 
 	return rtnl_dump_filter_l(rth, a);
-- 
2.1.2

[iproute PATCH v2 4/6] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Phil Sutter]

Linux version 3.1 introduced a consistency check for netlink dumps in
commit 670dc28 ("netlink: advertise incomplete dumps"). This bites
iproute2 when flushing more addresses than can fit into a single
RTM_GETADDR response. To silence the spurious error message "Dump was
interrupted and may be inconsistent.", advise rtnl_dump_filter_l() to
not care about NLM_F_DUMP_INTR.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b5b444b..23e0308 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1398,7 +1398,8 @@ static int ipaddr_flush(void)
 			exit(1);
 		}
 		filter.flushed = 0;
-		if (rtnl_dump_filter(&rth, print_addrinfo, stdout) < 0) {
+		if (rtnl_dump_filter_nc(&rth, print_addrinfo,
+					stdout, NLM_F_DUMP_INTR) < 0) {
 			fprintf(stderr, "Flush terminated\n");
 			exit(1);
 		}
-- 
2.1.2

[iproute PATCH v2 5/6] ipaddress: drop unnecessary check in ipaddr_list_flush_or_save()

[Phil Sutter]

Right after ipaddr_reset_filter(), filter.family is always AF_UNSPEC.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 23e0308..ff7f2a8 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1446,10 +1446,7 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 
 	ipaddr_reset_filter(oneline, 0);
 	filter.showqueue = 1;
-
-	if (filter.family == AF_UNSPEC)
-		filter.family = preferred_family;
-
+	filter.family = preferred_family;
 	filter.group = -1;
 
 	if (action == IPADD_FLUSH) {
-- 
2.1.2

[iproute PATCH v2 6/6] iptoken: simplify iptoken_list a bit

[Phil Sutter]

Since it uses only a single filter, rtnl_dump_filter() can be used.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptoken.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/ip/iptoken.c b/ip/iptoken.c
index a38194c..428f133 100644
--- a/ip/iptoken.c
+++ b/ip/iptoken.c
@@ -95,10 +95,6 @@ static int iptoken_list(int argc, char **argv)
 {
 	int af = AF_INET6;
 	struct rtnl_dump_args da;
-	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = print_token, .arg1 = &da, },
-		{ .filter = NULL, .arg1 = NULL, },
-	};
 
 	memset(&da, 0, sizeof(da));
 	da.fp = stdout;
@@ -118,7 +114,7 @@ static int iptoken_list(int argc, char **argv)
 		return -1;
 	}
 
-	if (rtnl_dump_filter_l(&rth, a) < 0) {
+	if (rtnl_dump_filter(&rth, print_token, &da) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -1;
 	}
-- 
2.1.2

Re: [iproute PATCH v2 4/6] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Sergei Shtylyov]

Hello.

On 11/08/2015 11:22 PM, Phil Sutter wrote:

> Linux version 3.1 introduced a consistency check for netlink dumps in
> commit 670dc28 ("netlink: advertise incomplete dumps"). This bites

    The scripts/checkpatch.pl now enforces 12-digit commit ID...

> iproute2 when flushing more addresses than can fit into a single
> RTM_GETADDR response. To silence the spurious error message "Dump was
> interrupted and may be inconsistent.", advise rtnl_dump_filter_l() to
> not care about NLM_F_DUMP_INTR.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>

MBR, Sergei

Re: [iproute PATCH v2 4/6] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Sergei Shtylyov]

On 11/09/2015 09:51 PM, Sergei Shtylyov wrote:

>> Linux version 3.1 introduced a consistency check for netlink dumps in
>> commit 670dc28 ("netlink: advertise incomplete dumps"). This bites
>
>     The scripts/checkpatch.pl now enforces 12-digit commit ID...

    Sorry, didn't realize it wasn't a kernel patch. :-)

>> iproute2 when flushing more addresses than can fit into a single
>> RTM_GETADDR response. To silence the spurious error message "Dump was
>> interrupted and may be inconsistent.", advise rtnl_dump_filter_l() to
>> not care about NLM_F_DUMP_INTR.
>>
>> Signed-off-by: Phil Sutter <phil@nwl.cc>

[...]

MBR, Sergei

Re: [iproute PATCH v2 4/6] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Phil Sutter]

On Mon, Nov 09, 2015 at 09:58:50PM +0300, Sergei Shtylyov wrote:
> On 11/09/2015 09:51 PM, Sergei Shtylyov wrote:
> 
> >> Linux version 3.1 introduced a consistency check for netlink dumps in
> >> commit 670dc28 ("netlink: advertise incomplete dumps"). This bites
> >
> >     The scripts/checkpatch.pl now enforces 12-digit commit ID...
> 
>     Sorry, didn't realize it wasn't a kernel patch. :-)

Heh, no problem! If more projects used checkpatch.pl, the world would be
a better place.

My quoting commits was usually a matter of copying what 'git --oneline
show' outputs. Thanks for the heads-up, I've adjusted core.abbrev now.

Cheers, Phil

Re: [iproute PATCH v2 6/6] iptoken: simplify iptoken_list a bit

[Daniel Borkmann]

On 11/08/2015 09:22 PM, Phil Sutter wrote:
> Since it uses only a single filter, rtnl_dump_filter() can be used.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [iproute PATCH v2 1/6] ipaddress: make flush command more error-tolerant

[Stephen Hemminger]

On Sun,  8 Nov 2015 21:22:16 +0100
Phil Sutter <phil@nwl.cc> wrote:

> The core issue here is that with promote_secondaries sysctl setting
> being turned off, removing the primary address implicitly removes all
> secondaries as well. Iproute is aware of this and therefore tries to
> remove all secondary addresses first to circumvent errors due to
> removing non-existent addresses. But this works only if not too many IP
> addresses are assigned to an interface, otherwise the RTM_GETADDR
> response is split up into multiple buffers. In my test-case, flushing
> more than 42 IPv4 addresses was sufficient to trigger an error:
> 
> Failed to send flush request: Cannot assign requested address
> 
> This patch fixes the issue by simply ignoring EADDRNOTAVAIL when
> flushing.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Patch does not apply to current tree. I applied an earlier patch from Neil Horman
and this supersedes this.

Please redo the patch series.

[iproute PATCH v3 0/5] flush many addresses and some cleanups

[Phil Sutter]

This patch series aims to silence spurious error messages when flushing
an interface with many addresses assigned and provides a few cleanups
found along the way.

The original issue was a test-case adding 40k IPv4 addresses to an
interface and calling 'ip addr flush' afterwards. Initially, this
resulted in an error message:

| Failed to send flush request: Cannot assign requested address

Iproute apparently tried to remove (secondary) addresses which didn't
exist anymore since the primary one had already been removed. (This
behaviour actually depends on the promote_secondaries sysctl setting.)
Neil Horman fixed this issue in his recent patch e149d4e ("iproute2:
Ignore EADDRNOTAVAIL errors during address flush operation"). This also
allows to remove iproute's previous workaround, which is to flush
secondary addresses before primary ones (patch 1/5).

Yet, still an error message is emitted on newer kernels, as they started
to check consistency of netlink dumps (which is broken by iproute as it
alters the data while it is dumped). This error may as well be ignored,
as it's expected behaviour while flushing addresses. Though in order to
do that, libnetlink API had to be extended a bit to actually allow to
ignore certain nlmsg_flags bits. Patch 2/5 therefore extends libnetlink,
patch 3/5 then makes use of the extended functionality.

While debugging the issue, an unnecessary check has been discovered
(patch 4/5) as well as a possible simplification in iptoken.c was found
(patch 5/5).

Changes since v1:
- Add forgotten hint about dependent patch (see above).
- Fix typo in patch 3/6 (sorry, I forgot to test-build a last-minute
  cleanup).
- All other patches remain unchanged.

Changes since v2:
- Drop first patch since it was superseded by e149d4e ("iproute2:
  Ignore EADDRNOTAVAIL errors during address flush operation").
- Adjust cover letter text accordingly.

Phil Sutter (5):
  ipaddress: simplify ipaddr_flush()
  libnetlink: introduce nc_flags
  ipaddress: fix ipaddr_flush for Linux >= 3.1
  ipaddress: drop unnecessary check in ipaddr_list_flush_or_save()
  iptoken: simplify iptoken_list a bit

 include/libnetlink.h |  7 ++++++-
 ip/ipaddress.c       | 44 +++-----------------------------------------
 ip/iptoken.c         |  6 +-----
 lib/libnetlink.c     | 10 ++++++----
 4 files changed, 16 insertions(+), 51 deletions(-)

-- 
2.5.0

[iproute PATCH v3 1/5] ipaddress: simplify ipaddr_flush()

[Phil Sutter]

Since it's no longer relevant whether an IP address is primary or
secondary when flushing, ipaddr_flush() can be simplified a bit.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 38 +-------------------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 05358c9..26e91c9 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1148,28 +1148,6 @@ brief_exit:
 	return 0;
 }
 
-static int print_addrinfo_primary(const struct sockaddr_nl *who,
-				  struct nlmsghdr *n, void *arg)
-{
-	struct ifaddrmsg *ifa = NLMSG_DATA(n);
-
-	if (ifa->ifa_flags & IFA_F_SECONDARY)
-		return 0;
-
-	return print_addrinfo(who, n, arg);
-}
-
-static int print_addrinfo_secondary(const struct sockaddr_nl *who,
-				    struct nlmsghdr *n, void *arg)
-{
-	struct ifaddrmsg *ifa = NLMSG_DATA(n);
-
-	if (!(ifa->ifa_flags & IFA_F_SECONDARY))
-		return 0;
-
-	return print_addrinfo(who, n, arg);
-}
-
 struct nlmsg_list
 {
 	struct nlmsg_list *next;
@@ -1420,26 +1398,12 @@ static int ipaddr_flush(void)
 	filter.flushe = sizeof(flushb);
 
 	while ((max_flush_loops == 0) || (round < max_flush_loops)) {
-		const struct rtnl_dump_filter_arg a[3] = {
-			{
-				.filter = print_addrinfo_secondary,
-				.arg1 = stdout,
-			},
-			{
-				.filter = print_addrinfo_primary,
-				.arg1 = stdout,
-			},
-			{
-				.filter = NULL,
-				.arg1 = NULL,
-			},
-		};
 		if (rtnl_wilddump_request(&rth, filter.family, RTM_GETADDR) < 0) {
 			perror("Cannot send dump request");
 			exit(1);
 		}
 		filter.flushed = 0;
-		if (rtnl_dump_filter_l(&rth, a) < 0) {
+		if (rtnl_dump_filter(&rth, print_addrinfo, stdout) < 0) {
 			fprintf(stderr, "Flush terminated\n");
 			exit(1);
 		}
-- 
2.5.0

[iproute PATCH v3 2/5] libnetlink: introduce nc_flags

[Phil Sutter]

Allow for a filter to ignore certain nlmsg_flags.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/libnetlink.h |  7 ++++++-
 lib/libnetlink.c     | 10 ++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index 2280c39..431189e 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -60,11 +60,16 @@ struct rtnl_dump_filter_arg
 {
 	rtnl_filter_t filter;
 	void *arg1;
+	__u16 nc_flags;
 };
 
 int rtnl_dump_filter_l(struct rtnl_handle *rth,
 			      const struct rtnl_dump_filter_arg *arg);
-int rtnl_dump_filter(struct rtnl_handle *rth, rtnl_filter_t filter, void *arg);
+int rtnl_dump_filter_nc(struct rtnl_handle *rth,
+			rtnl_filter_t filter,
+			void *arg, __u16 nc_flags);
+#define rtnl_dump_filter(rth, filter, arg) \
+	rtnl_dump_filter_nc(rth, filter, arg, 0)
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr *answer, size_t len)
 	__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 09b0e91..922ec2d 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -259,6 +259,8 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 			while (NLMSG_OK(h, msglen)) {
 				int err = 0;
 
+				h->nlmsg_flags &= ~a->nc_flags;
+
 				if (nladdr.nl_pid != 0 ||
 				    h->nlmsg_pid != rth->local.nl_pid ||
 				    h->nlmsg_seq != rth->dump)
@@ -317,13 +319,13 @@ skip_it:
 	}
 }
 
-int rtnl_dump_filter(struct rtnl_handle *rth,
+int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 		     rtnl_filter_t filter,
-		     void *arg1)
+		     void *arg1, __u16 nc_flags)
 {
 	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = filter, .arg1 = arg1, },
-		{ .filter = NULL,   .arg1 = NULL, },
+		{ .filter = filter, .arg1 = arg1, .nc_flags = nc_flags, },
+		{ .filter = NULL,   .arg1 = NULL, .nc_flags = 0, },
 	};
 
 	return rtnl_dump_filter_l(rth, a);
-- 
2.5.0

[iproute PATCH v3 3/5] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Phil Sutter]

Linux version 3.1 introduced a consistency check for netlink dumps in
commit 670dc28 ("netlink: advertise incomplete dumps"). This bites
iproute2 when flushing more addresses than can fit into a single
RTM_GETADDR response. To silence the spurious error message "Dump was
interrupted and may be inconsistent.", advise rtnl_dump_filter_l() to
not care about NLM_F_DUMP_INTR.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 26e91c9..9811eb4 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1403,7 +1403,8 @@ static int ipaddr_flush(void)
 			exit(1);
 		}
 		filter.flushed = 0;
-		if (rtnl_dump_filter(&rth, print_addrinfo, stdout) < 0) {
+		if (rtnl_dump_filter_nc(&rth, print_addrinfo,
+					stdout, NLM_F_DUMP_INTR) < 0) {
 			fprintf(stderr, "Flush terminated\n");
 			exit(1);
 		}
-- 
2.5.0

[iproute PATCH v3 4/5] ipaddress: drop unnecessary check in ipaddr_list_flush_or_save()

[Phil Sutter]

Right after ipaddr_reset_filter(), filter.family is always AF_UNSPEC.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipaddress.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 9811eb4..bc8359e 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1451,10 +1451,7 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 
 	ipaddr_reset_filter(oneline, 0);
 	filter.showqueue = 1;
-
-	if (filter.family == AF_UNSPEC)
-		filter.family = preferred_family;
-
+	filter.family = preferred_family;
 	filter.group = -1;
 
 	if (action == IPADD_FLUSH) {
-- 
2.5.0

[iproute PATCH v3 5/5] iptoken: simplify iptoken_list a bit

[Phil Sutter]

Since it uses only a single filter, rtnl_dump_filter() can be used.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptoken.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/ip/iptoken.c b/ip/iptoken.c
index a38194c..428f133 100644
--- a/ip/iptoken.c
+++ b/ip/iptoken.c
@@ -95,10 +95,6 @@ static int iptoken_list(int argc, char **argv)
 {
 	int af = AF_INET6;
 	struct rtnl_dump_args da;
-	const struct rtnl_dump_filter_arg a[2] = {
-		{ .filter = print_token, .arg1 = &da, },
-		{ .filter = NULL, .arg1 = NULL, },
-	};
 
 	memset(&da, 0, sizeof(da));
 	da.fp = stdout;
@@ -118,7 +114,7 @@ static int iptoken_list(int argc, char **argv)
 		return -1;
 	}
 
-	if (rtnl_dump_filter_l(&rth, a) < 0) {
+	if (rtnl_dump_filter(&rth, print_token, &da) < 0) {
 		fprintf(stderr, "Dump terminated\n");
 		return -1;
 	}
-- 
2.5.0

Re: [iproute PATCH v3 3/5] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Sergei Shtylyov]

Hello.

On 11/24/2015 5:31 PM, Phil Sutter wrote:

> Linux version 3.1 introduced a consistency check for netlink dumps in
> commit 670dc28 ("netlink: advertise incomplete dumps"). This bites

    Need 12-digit SHA1 here, scripts/checkpatch.pl checks this now.

> iproute2 when flushing more addresses than can fit into a single
> RTM_GETADDR response. To silence the spurious error message "Dump was
> interrupted and may be inconsistent.", advise rtnl_dump_filter_l() to
> not care about NLM_F_DUMP_INTR.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
[...]

MBR, Sergei

Re: [iproute PATCH v3 3/5] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Phil Sutter]

Hi Sergei,

On Wed, Nov 25, 2015 at 04:36:04PM +0300, Sergei Shtylyov wrote:
> On 11/24/2015 5:31 PM, Phil Sutter wrote:
> 
> > Linux version 3.1 introduced a consistency check for netlink dumps in
> > commit 670dc28 ("netlink: advertise incomplete dumps"). This bites
> 
>     Need 12-digit SHA1 here, scripts/checkpatch.pl checks this now.

Dejavu! [1]

Again, this is luckily just iproute2. But thanks again for the reminder,
I've set 'core.abbrev = 13' now globally on all my machines, that should
hold for a while. :)

Cheers, Phil

[1]: https://www.mail-archive.com/netdev@vger.kernel.org/msg86385.html

Re: [iproute PATCH v3 3/5] ipaddress: fix ipaddr_flush for Linux >= 3.1

[Sergei Shtylyov]

Hello.

On 11/25/2015 5:32 PM, Phil Sutter wrote:

>> On 11/24/2015 5:31 PM, Phil Sutter wrote:
>>
>>> Linux version 3.1 introduced a consistency check for netlink dumps in
>>> commit 670dc28 ("netlink: advertise incomplete dumps"). This bites
>>
>>      Need 12-digit SHA1 here, scripts/checkpatch.pl checks this now.

> Dejavu! [1]

    Oh, indeed! Sorry about that, don't remember already. :-)

> Again, this is luckily just iproute2. But thanks again for the reminder,

     Yeah, I didn't look at the subject too closely... :-/

> I've set 'core.abbrev = 13' now globally on all my machines, that should
> hold for a while. :)

> Cheers, Phil
>
> [1]: https://www.mail-archive.com/netdev@vger.kernel.org/msg86385.html

MBR, Sergei

Re: [iproute PATCH v3 0/5] flush many addresses and some cleanups

[Stephen Hemminger]

On Tue, 24 Nov 2015 15:30:59 +0100
Phil Sutter <phil@nwl.cc> wrote:

> This patch series aims to silence spurious error messages when flushing
> an interface with many addresses assigned and provides a few cleanups
> found along the way.
> 
> The original issue was a test-case adding 40k IPv4 addresses to an
> interface and calling 'ip addr flush' afterwards. Initially, this
> resulted in an error message:
> 
> | Failed to send flush request: Cannot assign requested address
> 
> Iproute apparently tried to remove (secondary) addresses which didn't
> exist anymore since the primary one had already been removed. (This
> behaviour actually depends on the promote_secondaries sysctl setting.)
> Neil Horman fixed this issue in his recent patch e149d4e ("iproute2:
> Ignore EADDRNOTAVAIL errors during address flush operation"). This also
> allows to remove iproute's previous workaround, which is to flush
> secondary addresses before primary ones (patch 1/5).
> 
> Yet, still an error message is emitted on newer kernels, as they started
> to check consistency of netlink dumps (which is broken by iproute as it
> alters the data while it is dumped). This error may as well be ignored,
> as it's expected behaviour while flushing addresses. Though in order to
> do that, libnetlink API had to be extended a bit to actually allow to
> ignore certain nlmsg_flags bits. Patch 2/5 therefore extends libnetlink,
> patch 3/5 then makes use of the extended functionality.
> 
> While debugging the issue, an unnecessary check has been discovered
> (patch 4/5) as well as a possible simplification in iptoken.c was found
> (patch 5/5).
> 
> Changes since v1:
> - Add forgotten hint about dependent patch (see above).
> - Fix typo in patch 3/6 (sorry, I forgot to test-build a last-minute
>   cleanup).
> - All other patches remain unchanged.
> 
> Changes since v2:
> - Drop first patch since it was superseded by e149d4e ("iproute2:
>   Ignore EADDRNOTAVAIL errors during address flush operation").
> - Adjust cover letter text accordingly.
> 
> Phil Sutter (5):
>   ipaddress: simplify ipaddr_flush()
>   libnetlink: introduce nc_flags
>   ipaddress: fix ipaddr_flush for Linux >= 3.1
>   ipaddress: drop unnecessary check in ipaddr_list_flush_or_save()
>   iptoken: simplify iptoken_list a bit
> 
>  include/libnetlink.h |  7 ++++++-
>  ip/ipaddress.c       | 44 +++-----------------------------------------
>  ip/iptoken.c         |  6 +-----
>  lib/libnetlink.c     | 10 ++++++----
>  4 files changed, 16 insertions(+), 51 deletions(-)
>

Applied, thanks