[PATCH iproute2 0/4] monitor command fixes

[PATCH iproute2 0/4] monitor command fixes

[Benjamin Poirier]

Fix problems in `bridge monitor` and `ip monitor` regarding the filtering
of object types and address families.

Benjamin Poirier (4):
  bridge: Do not print stray prefixes in monitor mode
  ip-monitor: Do not listen for nexthops by default when specifying
    stats
  ip-monitor: Include stats events in default and "all" cases
  ip-monitor: Fix the selection of rtnl groups when listening for all
    object types

 bridge/br_common.h |   1 +
 bridge/fdb.c       |   3 ++
 bridge/link.c      |   4 ++
 bridge/mdb.c       |   3 ++
 bridge/monitor.c   |  35 ++++----------
 bridge/vlan.c      |   4 ++
 bridge/vni.c       |   4 ++
 ip/ipmonitor.c     | 115 +++++++++++++++++----------------------------
 8 files changed, 71 insertions(+), 98 deletions(-)

-- 
2.37.2

[PATCH iproute2 1/4] bridge: Do not print stray prefixes in monitor mode

[Benjamin Poirier]

When using `bridge monitor` with the '-timestamp' option or the "all"
parameter, prefixes are printed before the actual event descriptions.
Currently, those prefixes are printed for each netlink message that's
received. However, some netlink messages do not lead to an event
description being printed. That's usually because a message is not related
to AF_BRIDGE. This results in stray prefixes being printed.

Restructure accept_msg() and its callees such that prefixes are only
printed after a message has been checked for eligibility.

The issue can be witnessed using the following commands:
	ip link add dummy0 type dummy
	# Start `bridge monitor all` now in another terminal.
	# Cause a stray "[LINK]" to be printed (family 10).
	# It does not appear yet because the output is line buffered.
	ip link set dev dummy0 up
	# Cause a stray "[NEIGH]" to be printed (family 2).
	ip neigh add 10.0.0.1 lladdr 02:00:00:00:00:01 dev dummy0
	# Cause a genuine entry to be printed, which flushes the previous
	# output.
	bridge fdb add 02:00:00:00:00:01 dev dummy0
	# We now see:
	# [LINK][NEIGH][NEIGH]02:00:00:00:00:01 dev dummy0 self permanent

Fixes: d04bc300c3e3 ("Add bridge command")
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 bridge/br_common.h |  1 +
 bridge/fdb.c       |  3 +++
 bridge/link.c      |  4 ++++
 bridge/mdb.c       |  3 +++
 bridge/monitor.c   | 35 ++++++++++-------------------------
 bridge/vlan.c      |  4 ++++
 bridge/vni.c       |  4 ++++
 7 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/bridge/br_common.h b/bridge/br_common.h
index 841f0594..da677df8 100644
--- a/bridge/br_common.h
+++ b/bridge/br_common.h
@@ -16,6 +16,7 @@ int print_vlan_rtm(struct nlmsghdr *n, void *arg, bool monitor,
 		   bool global_only);
 int print_vnifilter_rtm(struct nlmsghdr *n, void *arg, bool monitor);
 void br_print_router_port_stats(struct rtattr *pattr);
+void print_headers(FILE *fp, const char *label);
 
 int do_fdb(int argc, char **argv);
 int do_mdb(int argc, char **argv);
diff --git a/bridge/fdb.c b/bridge/fdb.c
index 5f71bde0..775feb12 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -179,6 +179,8 @@ int print_fdb(struct nlmsghdr *n, void *arg)
 	if (filter_dynamic && (r->ndm_state & NUD_PERMANENT))
 		return 0;
 
+	print_headers(fp, "[NEIGH]");
+
 	open_json_object(NULL);
 	if (n->nlmsg_type == RTM_DELNEIGH)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
@@ -810,6 +812,7 @@ static int fdb_flush(int argc, char **argv)
 int do_fdb(int argc, char **argv)
 {
 	ll_init_map(&rth);
+	timestamp = 0;
 
 	if (argc > 0) {
 		if (matches(*argv, "add") == 0)
diff --git a/bridge/link.c b/bridge/link.c
index 3810fa04..fef3a9ef 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -230,6 +230,8 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
 	if (!name)
 		return -1;
 
+	print_headers(fp, "[LINK]");
+
 	open_json_object(NULL);
 	if (n->nlmsg_type == RTM_DELLINK)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
@@ -588,6 +590,8 @@ static int brlink_show(int argc, char **argv)
 int do_link(int argc, char **argv)
 {
 	ll_init_map(&rth);
+	timestamp = 0;
+
 	if (argc > 0) {
 		if (matches(*argv, "set") == 0 ||
 		    matches(*argv, "change") == 0)
diff --git a/bridge/mdb.c b/bridge/mdb.c
index 7b5863d3..d3afc900 100644
--- a/bridge/mdb.c
+++ b/bridge/mdb.c
@@ -380,6 +380,8 @@ int print_mdb_mon(struct nlmsghdr *n, void *arg)
 	if (ret != 1)
 		return ret;
 
+	print_headers(fp, "[MDB]");
+
 	if (n->nlmsg_type == RTM_DELMDB)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
@@ -564,6 +566,7 @@ static int mdb_modify(int cmd, int flags, int argc, char **argv)
 int do_mdb(int argc, char **argv)
 {
 	ll_init_map(&rth);
+	timestamp = 0;
 
 	if (argc > 0) {
 		if (matches(*argv, "add") == 0)
diff --git a/bridge/monitor.c b/bridge/monitor.c
index f17c1906..e321516a 100644
--- a/bridge/monitor.c
+++ b/bridge/monitor.c
@@ -35,42 +35,22 @@ static void usage(void)
 	exit(-1);
 }
 
-static int print_tunnel_rtm(struct nlmsghdr *n, void *arg, bool monitor)
-{
-	struct tunnel_msg *tmsg = NLMSG_DATA(n);
-
-	if (tmsg->family == PF_BRIDGE)
-		return print_vnifilter_rtm(n, arg, monitor);
-
-	return 0;
-}
-
 static int accept_msg(struct rtnl_ctrl_data *ctrl,
 		      struct nlmsghdr *n, void *arg)
 {
 	FILE *fp = arg;
 
-	if (timestamp)
-		print_timestamp(fp);
-
 	switch (n->nlmsg_type) {
 	case RTM_NEWLINK:
 	case RTM_DELLINK:
-		if (prefix_banner)
-			fprintf(fp, "[LINK]");
-
 		return print_linkinfo(n, arg);
 
 	case RTM_NEWNEIGH:
 	case RTM_DELNEIGH:
-		if (prefix_banner)
-			fprintf(fp, "[NEIGH]");
 		return print_fdb(n, arg);
 
 	case RTM_NEWMDB:
 	case RTM_DELMDB:
-		if (prefix_banner)
-			fprintf(fp, "[MDB]");
 		return print_mdb_mon(n, arg);
 
 	case NLMSG_TSTAMP:
@@ -79,21 +59,26 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
 
 	case RTM_NEWVLAN:
 	case RTM_DELVLAN:
-		if (prefix_banner)
-			fprintf(fp, "[VLAN]");
 		return print_vlan_rtm(n, arg, true, false);
 
 	case RTM_NEWTUNNEL:
 	case RTM_DELTUNNEL:
-		if (prefix_banner)
-			fprintf(fp, "[TUNNEL]");
-		return print_tunnel_rtm(n, arg, true);
+		return print_vnifilter_rtm(n, arg, true);
 
 	default:
 		return 0;
 	}
 }
 
+void print_headers(FILE *fp, const char *label)
+{
+	if (timestamp)
+		print_timestamp(fp);
+
+	if (prefix_banner)
+		fprintf(fp, "%s", label);
+}
+
 int do_monitor(int argc, char **argv)
 {
 	char *file = NULL;
diff --git a/bridge/vlan.c b/bridge/vlan.c
index 390a2037..13df1e84 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -1032,6 +1032,7 @@ int print_vlan_rtm(struct nlmsghdr *n, void *arg, bool monitor, bool global_only
 	struct br_vlan_msg *bvm = NLMSG_DATA(n);
 	int len = n->nlmsg_len;
 	struct rtattr *a;
+	FILE *fp = arg;
 	int rem;
 
 	if (n->nlmsg_type != RTM_NEWVLAN && n->nlmsg_type != RTM_DELVLAN &&
@@ -1053,6 +1054,8 @@ int print_vlan_rtm(struct nlmsghdr *n, void *arg, bool monitor, bool global_only
 	if (filter_index && filter_index != bvm->ifindex)
 		return 0;
 
+	print_headers(fp, "[VLAN]");
+
 	if (n->nlmsg_type == RTM_DELVLAN)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
@@ -1333,6 +1336,7 @@ static int vlan_global(int argc, char **argv)
 int do_vlan(int argc, char **argv)
 {
 	ll_init_map(&rth);
+	timestamp = 0;
 
 	if (argc > 0) {
 		if (matches(*argv, "add") == 0)
diff --git a/bridge/vni.c b/bridge/vni.c
index a0c2792c..e776797a 100644
--- a/bridge/vni.c
+++ b/bridge/vni.c
@@ -309,6 +309,7 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg, bool monitor)
 	int len = n->nlmsg_len;
 	bool first = true;
 	struct rtattr *t;
+	FILE *fp = arg;
 	int rem;
 
 	if (n->nlmsg_type != RTM_NEWTUNNEL &&
@@ -331,6 +332,8 @@ int print_vnifilter_rtm(struct nlmsghdr *n, void *arg, bool monitor)
 	if (filter_index && filter_index != tmsg->ifindex)
 		return 0;
 
+	print_headers(fp, "[TUNNEL]");
+
 	if (n->nlmsg_type == RTM_DELTUNNEL)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
@@ -418,6 +421,7 @@ static int vni_show(int argc, char **argv)
 int do_vni(int argc, char **argv)
 {
 	ll_init_map(&rth);
+	timestamp = 0;
 
 	if (argc > 0) {
 		if (strcmp(*argv, "add") == 0)
-- 
2.37.2

Re: [PATCH iproute2 1/4] bridge: Do not print stray prefixes in monitor mode

[Stephen Hemminger]

On Thu, 22 Sep 2022 15:19:35 +0900
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> When using `bridge monitor` with the '-timestamp' option or the "all"
> parameter, prefixes are printed before the actual event descriptions.
> Currently, those prefixes are printed for each netlink message that's
> received. However, some netlink messages do not lead to an event
> description being printed. That's usually because a message is not related
> to AF_BRIDGE. This results in stray prefixes being printed.
> 
> Restructure accept_msg() and its callees such that prefixes are only
> printed after a message has been checked for eligibility.
> 
> The issue can be witnessed using the following commands:
> 	ip link add dummy0 type dummy
> 	# Start `bridge monitor all` now in another terminal.
> 	# Cause a stray "[LINK]" to be printed (family 10).
> 	# It does not appear yet because the output is line buffered.
> 	ip link set dev dummy0 up
> 	# Cause a stray "[NEIGH]" to be printed (family 2).
> 	ip neigh add 10.0.0.1 lladdr 02:00:00:00:00:01 dev dummy0
> 	# Cause a genuine entry to be printed, which flushes the previous
> 	# output.
> 	bridge fdb add 02:00:00:00:00:01 dev dummy0
> 	# We now see:
> 	# [LINK][NEIGH][NEIGH]02:00:00:00:00:01 dev dummy0 self permanent
> 
> Fixes: d04bc300c3e3 ("Add bridge command")
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---

Looks good, reminds me that the bridge command need to be converted
to use json_print.

[PATCH iproute2] ip-monitor: Do not error out when RTNLGRP_STATS is not available

[Benjamin Poirier]

Following commit 4e8a9914c4d4 ("ip-monitor: Include stats events in default
and "all" cases"), `ip monitor` fails to start on kernels which do not
contain linux.git commit 5fd0b838efac ("net: rtnetlink: Add UAPI toggle for
IFLA_OFFLOAD_XSTATS_L3_STATS") because the netlink group RTNLGRP_STATS
doesn't exist:

 $ ip monitor
 Failed to add stats group to list

When "stats" is not explicitly requested, change the error to a warning so
that `ip monitor` and `ip monitor all` continue to work on older kernels.

Note that the same change is not done for RTNLGRP_NEXTHOP because its value
is 32 and group numbers <= 32 are always supported; see the comment above
netlink_change_ngroups() in the kernel source. Therefore
NETLINK_ADD_MEMBERSHIP 32 does not error out even on kernels which do not
support RTNLGRP_NEXTHOP.

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 4e8a9914c4d4 ("ip-monitor: Include stats events in default and "all" cases")
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 ip/ipmonitor.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index 8a72ea42..45e4e8f1 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -195,6 +195,8 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
 int do_ipmonitor(int argc, char **argv)
 {
 	unsigned int groups = 0, lmask = 0;
+	/* "needed" mask */
+	unsigned int nmask;
 	char *file = NULL;
 	int ifindex = 0;
 
@@ -253,6 +255,7 @@ int do_ipmonitor(int argc, char **argv)
 	ipneigh_reset_filter(ifindex);
 	ipnetconf_reset_filter(ifindex);
 
+	nmask = lmask;
 	if (!lmask)
 		lmask = IPMON_L_ALL;
 
@@ -328,8 +331,11 @@ int do_ipmonitor(int argc, char **argv)
 
 	if (lmask & IPMON_LSTATS &&
 	    rtnl_add_nl_group(&rth, RTNLGRP_STATS) < 0) {
+		if (!(nmask & IPMON_LSTATS))
+			fprintf(stderr, "Warning: ");
 		fprintf(stderr, "Failed to add stats group to list\n");
-		exit(1);
+		if (nmask & IPMON_LSTATS)
+			exit(1);
 	}
 
 	if (listen_all_nsid && rtnl_listen_all_nsid(&rth) < 0)
-- 
2.37.2

Re: [PATCH iproute2] ip-monitor: Do not error out when RTNLGRP_STATS is not available

[Stephen Hemminger]

On Wed, 26 Oct 2022 07:29:09 +0900
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> Following commit 4e8a9914c4d4 ("ip-monitor: Include stats events in default
> and "all" cases"), `ip monitor` fails to start on kernels which do not
> contain linux.git commit 5fd0b838efac ("net: rtnetlink: Add UAPI toggle for
> IFLA_OFFLOAD_XSTATS_L3_STATS") because the netlink group RTNLGRP_STATS
> doesn't exist:
> 
>  $ ip monitor
>  Failed to add stats group to list
> 
> When "stats" is not explicitly requested, change the error to a warning so
> that `ip monitor` and `ip monitor all` continue to work on older kernels.
> 
> Note that the same change is not done for RTNLGRP_NEXTHOP because its value
> is 32 and group numbers <= 32 are always supported; see the comment above
> netlink_change_ngroups() in the kernel source. Therefore
> NETLINK_ADD_MEMBERSHIP 32 does not error out even on kernels which do not
> support RTNLGRP_NEXTHOP.
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 4e8a9914c4d4 ("ip-monitor: Include stats events in default and "all" cases")
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---
>  ip/ipmonitor.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
> index 8a72ea42..45e4e8f1 100644
> --- a/ip/ipmonitor.c
> +++ b/ip/ipmonitor.c
> @@ -195,6 +195,8 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
>  int do_ipmonitor(int argc, char **argv)
>  {
>  	unsigned int groups = 0, lmask = 0;
> +	/* "needed" mask */
> +	unsigned int nmask;
>  	char *file = NULL;
>  	int ifindex = 0;
>  
> @@ -253,6 +255,7 @@ int do_ipmonitor(int argc, char **argv)
>  	ipneigh_reset_filter(ifindex);
>  	ipnetconf_reset_filter(ifindex);
>  
> +	nmask = lmask;
>  	if (!lmask)
>  		lmask = IPMON_L_ALL;
>  
> @@ -328,8 +331,11 @@ int do_ipmonitor(int argc, char **argv)
>  
>  	if (lmask & IPMON_LSTATS &&
>  	    rtnl_add_nl_group(&rth, RTNLGRP_STATS) < 0) {
> +		if (!(nmask & IPMON_LSTATS))
> +			fprintf(stderr, "Warning: ");
>  		fprintf(stderr, "Failed to add stats group to list\n");
> -		exit(1);
> +		if (nmask & IPMON_LSTATS)
> +			exit(1);
>  	}
>  
>  	if (listen_all_nsid && rtnl_listen_all_nsid(&rth) < 0)

You still end up warning on older kernels. My version is simpler.
All needs to not include lstats

Re: [PATCH iproute2] ip-monitor: Do not error out when RTNLGRP_STATS is not available

[Stephen Hemminger]

On Wed, 26 Oct 2022 07:29:09 +0900
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> Following commit 4e8a9914c4d4 ("ip-monitor: Include stats events in default
> and "all" cases"), `ip monitor` fails to start on kernels which do not
> contain linux.git commit 5fd0b838efac ("net: rtnetlink: Add UAPI toggle for
> IFLA_OFFLOAD_XSTATS_L3_STATS") because the netlink group RTNLGRP_STATS
> doesn't exist:
> 
>  $ ip monitor
>  Failed to add stats group to list
> 
> When "stats" is not explicitly requested, change the error to a warning so
> that `ip monitor` and `ip monitor all` continue to work on older kernels.
> 
> Note that the same change is not done for RTNLGRP_NEXTHOP because its value
> is 32 and group numbers <= 32 are always supported; see the comment above
> netlink_change_ngroups() in the kernel source. Therefore
> NETLINK_ADD_MEMBERSHIP 32 does not error out even on kernels which do not
> support RTNLGRP_NEXTHOP.
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 4e8a9914c4d4 ("ip-monitor: Include stats events in default and "all" cases")
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

There are two acceptable solutions:
1. Ignore the error, and never print any warning.
2. Don't ask for the stats feature with the default "ip monitor" and "ip monitor all"

Either way, it needs to be totally silent when built and run on older kernels.

[PATCH iproute2 v2] ip-monitor: Do not error out when RTNLGRP_STATS is not available

[Benjamin Poirier]

Following commit 4e8a9914c4d4 ("ip-monitor: Include stats events in default
and "all" cases"), `ip monitor` fails to start on kernels which do not
contain linux.git commit 5fd0b838efac ("net: rtnetlink: Add UAPI toggle for
IFLA_OFFLOAD_XSTATS_L3_STATS") because the netlink group RTNLGRP_STATS
doesn't exist:

 $ ip monitor
 Failed to add stats group to list

When "stats" is not explicitly requested, ignore the error so that `ip
monitor` and `ip monitor all` continue to work on older kernels.

Note that the same change is not done for RTNLGRP_NEXTHOP because its value
is 32 and group numbers <= 32 are always supported; see the comment above
netlink_change_ngroups() in the kernel source. Therefore
NETLINK_ADD_MEMBERSHIP 32 does not error out even on kernels which do not
support RTNLGRP_NEXTHOP.

v2:
* Silently ignore a failure to implicitly add the stats group, instead of
  printing a warning.

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 4e8a9914c4d4 ("ip-monitor: Include stats events in default and "all" cases")
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---

> There are two acceptable solutions:
> 1. Ignore the error, and never print any warning.
> 2. Don't ask for the stats feature with the default "ip monitor" and "ip monitor all"
> 
> Either way, it needs to be totally silent when built and run on older kernels.

Strictly speaking, the patch below is solution 1*:
  Ignore the error, and never print any warning ... when implicitly
adding the stats group.

Before 4e8a9914c4d4, `ip mon stats` used to error out if the stats group
could not be added. That behavior is preserved.


 ip/ipmonitor.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index 8a72ea42..d808369c 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -195,6 +195,8 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
 int do_ipmonitor(int argc, char **argv)
 {
 	unsigned int groups = 0, lmask = 0;
+	/* "needed" mask, failure to enable is an error */
+	unsigned int nmask;
 	char *file = NULL;
 	int ifindex = 0;
 
@@ -253,6 +255,7 @@ int do_ipmonitor(int argc, char **argv)
 	ipneigh_reset_filter(ifindex);
 	ipnetconf_reset_filter(ifindex);
 
+	nmask = lmask;
 	if (!lmask)
 		lmask = IPMON_L_ALL;
 
@@ -327,7 +330,8 @@ int do_ipmonitor(int argc, char **argv)
 	}
 
 	if (lmask & IPMON_LSTATS &&
-	    rtnl_add_nl_group(&rth, RTNLGRP_STATS) < 0) {
+	    rtnl_add_nl_group(&rth, RTNLGRP_STATS) < 0 &&
+	    nmask & IPMON_LSTATS) {
 		fprintf(stderr, "Failed to add stats group to list\n");
 		exit(1);
 	}
-- 
2.37.2

[PATCH iproute2 2/4] ip-monitor: Do not listen for nexthops by default when specifying stats

[Benjamin Poirier]

`ip monitor stats` listens for changes to nexthops and stats. It should
listen for stats only.

Fixes: a05a27c07cbf ("ipmonitor: Add monitoring support for stats events")
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 ip/ipmonitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index a4326d2a..380ab526 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -262,6 +262,7 @@ int do_ipmonitor(int argc, char **argv)
 		} else if (strcmp(*argv, "stats") == 0) {
 			lstats = 1;
 			groups = 0;
+			nh_set = 0;
 		} else if (strcmp(*argv, "all") == 0) {
 			prefix_banner = 1;
 		} else if (matches(*argv, "all-nsid") == 0) {
-- 
2.37.2

[PATCH iproute2 3/4] ip-monitor: Include stats events in default and "all" cases

[Benjamin Poirier]

It seems that stats were omitted from `ip monitor` and `ip monitor all`.
Since all other event types are included, include stats as well. Use the
same logic as for nexthops.

Fixes: a05a27c07cbf ("ipmonitor: Add monitoring support for stats events")
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 ip/ipmonitor.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index 380ab526..cb2195d1 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -180,6 +180,7 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
 
 int do_ipmonitor(int argc, char **argv)
 {
+	int lstats = 0, stats_set = 1;
 	int lnexthop = 0, nh_set = 1;
 	char *file = NULL;
 	unsigned int groups = 0;
@@ -190,7 +191,6 @@ int do_ipmonitor(int argc, char **argv)
 	int lprefix = 0;
 	int lneigh = 0;
 	int lnetconf = 0;
-	int lstats = 0;
 	int lrule = 0;
 	int lnsid = 0;
 	int ifindex = 0;
@@ -224,41 +224,51 @@ int do_ipmonitor(int argc, char **argv)
 			llink = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "address") == 0) {
 			laddr = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "route") == 0) {
 			lroute = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "mroute") == 0) {
 			lmroute = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "prefix") == 0) {
 			lprefix = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "neigh") == 0) {
 			lneigh = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "netconf") == 0) {
 			lnetconf = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "rule") == 0) {
 			lrule = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "nsid") == 0) {
 			lnsid = 1;
 			groups = 0;
 			nh_set = 0;
+			stats_set = 0;
 		} else if (matches(*argv, "nexthop") == 0) {
 			lnexthop = 1;
 			groups = 0;
+			stats_set = 0;
 		} else if (strcmp(*argv, "stats") == 0) {
 			lstats = 1;
 			groups = 0;
@@ -336,6 +346,8 @@ int do_ipmonitor(int argc, char **argv)
 	}
 	if (nh_set)
 		lnexthop = 1;
+	if (stats_set)
+		lstats = 1;
 
 	if (file) {
 		FILE *fp;
-- 
2.37.2

Re: [PATCH iproute2 3/4] ip-monitor: Include stats events in default and "all" cases

[Stephen Hemminger]

On Thu, 22 Sep 2022 15:19:37 +0900
Benjamin Poirier <bpoirier@nvidia.com> wrote:

> It seems that stats were omitted from `ip monitor` and `ip monitor all`.
> Since all other event types are included, include stats as well. Use the
> same logic as for nexthops.
> 
> Fixes: a05a27c07cbf ("ipmonitor: Add monitoring support for stats events")
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>

This change has caused a regression. Simple ip monitor command now fails.

$ ip monitor
Failed to add stats group to list

The failure is from setsockopt() returning -EINVAL

Using git bisect this patch is identified as the cause.
Please fix ASAP or will revert from next release and maybe even make
a point release to remove it.

[PATCH iproute2 4/4] ip-monitor: Fix the selection of rtnl groups when listening for all object types

[Benjamin Poirier]

Currently, when using `ip monitor`, family-specific rtnl multicast groups
(ex. RTNLGRP_IPV4_IFADDR) are used when specifying the '-family' option (or
one of its short forms) and an object type is specified (ex. `ip -4 monitor
addr`) but not when listening for changes to all object types (ex. `ip -4
monitor`). In that case, multicast groups for all families, regardless of
the '-family' option, are used. Depending on the object type, this leads to
ignoring the '-family' selection (MROUTE, ADDR, NETCONF), or printing stray
prefix headers with no event (ROUTE, RULE).

Rewrite the parameter parsing code so that per-family rtnl multicast groups
are selected in all cases.

The issue can be witnessed while running `ip -4 monitor label` at the same
time as the following command:
	ip link add dummy0 address 02:00:00:00:00:01 up type dummy
The output includes:
[ROUTE][ROUTE][ADDR]9: dummy0    inet6 fe80::ff:fe00:1/64 scope link
       valid_lft forever preferred_lft forever
Notice the stray "[ROUTE]" labels (related to filtered out ipv6 routes) and
the ipv6 ADDR entry. Those do not appear if using `ip -4 monitor label
route address`.

Fixes: aba5acdfdb34 ("(Logical change 1.3)")
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 ip/ipmonitor.c | 128 ++++++++++++++++---------------------------------
 1 file changed, 42 insertions(+), 86 deletions(-)

diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index cb2195d1..8a72ea42 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -178,40 +178,26 @@ static int accept_msg(struct rtnl_ctrl_data *ctrl,
 	return 0;
 }
 
+#define IPMON_LLINK		BIT(0)
+#define IPMON_LADDR		BIT(1)
+#define IPMON_LROUTE		BIT(2)
+#define IPMON_LMROUTE		BIT(3)
+#define IPMON_LPREFIX		BIT(4)
+#define IPMON_LNEIGH		BIT(5)
+#define IPMON_LNETCONF		BIT(6)
+#define IPMON_LSTATS		BIT(7)
+#define IPMON_LRULE		BIT(8)
+#define IPMON_LNSID		BIT(9)
+#define IPMON_LNEXTHOP		BIT(10)
+
+#define IPMON_L_ALL		(~0)
+
 int do_ipmonitor(int argc, char **argv)
 {
-	int lstats = 0, stats_set = 1;
-	int lnexthop = 0, nh_set = 1;
+	unsigned int groups = 0, lmask = 0;
 	char *file = NULL;
-	unsigned int groups = 0;
-	int llink = 0;
-	int laddr = 0;
-	int lroute = 0;
-	int lmroute = 0;
-	int lprefix = 0;
-	int lneigh = 0;
-	int lnetconf = 0;
-	int lrule = 0;
-	int lnsid = 0;
 	int ifindex = 0;
 
-	groups |= nl_mgrp(RTNLGRP_LINK);
-	groups |= nl_mgrp(RTNLGRP_IPV4_IFADDR);
-	groups |= nl_mgrp(RTNLGRP_IPV6_IFADDR);
-	groups |= nl_mgrp(RTNLGRP_IPV4_ROUTE);
-	groups |= nl_mgrp(RTNLGRP_IPV6_ROUTE);
-	groups |= nl_mgrp(RTNLGRP_MPLS_ROUTE);
-	groups |= nl_mgrp(RTNLGRP_IPV4_MROUTE);
-	groups |= nl_mgrp(RTNLGRP_IPV6_MROUTE);
-	groups |= nl_mgrp(RTNLGRP_IPV6_PREFIX);
-	groups |= nl_mgrp(RTNLGRP_NEIGH);
-	groups |= nl_mgrp(RTNLGRP_IPV4_NETCONF);
-	groups |= nl_mgrp(RTNLGRP_IPV6_NETCONF);
-	groups |= nl_mgrp(RTNLGRP_IPV4_RULE);
-	groups |= nl_mgrp(RTNLGRP_IPV6_RULE);
-	groups |= nl_mgrp(RTNLGRP_NSID);
-	groups |= nl_mgrp(RTNLGRP_MPLS_NETCONF);
-
 	rtnl_close(&rth);
 
 	while (argc > 0) {
@@ -221,58 +207,27 @@ int do_ipmonitor(int argc, char **argv)
 		} else if (matches(*argv, "label") == 0) {
 			prefix_banner = 1;
 		} else if (matches(*argv, "link") == 0) {
-			llink = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LLINK;
 		} else if (matches(*argv, "address") == 0) {
-			laddr = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LADDR;
 		} else if (matches(*argv, "route") == 0) {
-			lroute = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LROUTE;
 		} else if (matches(*argv, "mroute") == 0) {
-			lmroute = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LMROUTE;
 		} else if (matches(*argv, "prefix") == 0) {
-			lprefix = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LPREFIX;
 		} else if (matches(*argv, "neigh") == 0) {
-			lneigh = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LNEIGH;
 		} else if (matches(*argv, "netconf") == 0) {
-			lnetconf = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LNETCONF;
 		} else if (matches(*argv, "rule") == 0) {
-			lrule = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LRULE;
 		} else if (matches(*argv, "nsid") == 0) {
-			lnsid = 1;
-			groups = 0;
-			nh_set = 0;
-			stats_set = 0;
+			lmask |= IPMON_LNSID;
 		} else if (matches(*argv, "nexthop") == 0) {
-			lnexthop = 1;
-			groups = 0;
-			stats_set = 0;
+			lmask |= IPMON_LNEXTHOP;
 		} else if (strcmp(*argv, "stats") == 0) {
-			lstats = 1;
-			groups = 0;
-			nh_set = 0;
+			lmask |= IPMON_LSTATS;
 		} else if (strcmp(*argv, "all") == 0) {
 			prefix_banner = 1;
 		} else if (matches(*argv, "all-nsid") == 0) {
@@ -298,15 +253,18 @@ int do_ipmonitor(int argc, char **argv)
 	ipneigh_reset_filter(ifindex);
 	ipnetconf_reset_filter(ifindex);
 
-	if (llink)
+	if (!lmask)
+		lmask = IPMON_L_ALL;
+
+	if (lmask & IPMON_LLINK)
 		groups |= nl_mgrp(RTNLGRP_LINK);
-	if (laddr) {
+	if (lmask & IPMON_LADDR) {
 		if (!preferred_family || preferred_family == AF_INET)
 			groups |= nl_mgrp(RTNLGRP_IPV4_IFADDR);
 		if (!preferred_family || preferred_family == AF_INET6)
 			groups |= nl_mgrp(RTNLGRP_IPV6_IFADDR);
 	}
-	if (lroute) {
+	if (lmask & IPMON_LROUTE) {
 		if (!preferred_family || preferred_family == AF_INET)
 			groups |= nl_mgrp(RTNLGRP_IPV4_ROUTE);
 		if (!preferred_family || preferred_family == AF_INET6)
@@ -314,20 +272,20 @@ int do_ipmonitor(int argc, char **argv)
 		if (!preferred_family || preferred_family == AF_MPLS)
 			groups |= nl_mgrp(RTNLGRP_MPLS_ROUTE);
 	}
-	if (lmroute) {
+	if (lmask & IPMON_LMROUTE) {
 		if (!preferred_family || preferred_family == AF_INET)
 			groups |= nl_mgrp(RTNLGRP_IPV4_MROUTE);
 		if (!preferred_family || preferred_family == AF_INET6)
 			groups |= nl_mgrp(RTNLGRP_IPV6_MROUTE);
 	}
-	if (lprefix) {
+	if (lmask & IPMON_LPREFIX) {
 		if (!preferred_family || preferred_family == AF_INET6)
 			groups |= nl_mgrp(RTNLGRP_IPV6_PREFIX);
 	}
-	if (lneigh) {
+	if (lmask & IPMON_LNEIGH) {
 		groups |= nl_mgrp(RTNLGRP_NEIGH);
 	}
-	if (lnetconf) {
+	if (lmask & IPMON_LNETCONF) {
 		if (!preferred_family || preferred_family == AF_INET)
 			groups |= nl_mgrp(RTNLGRP_IPV4_NETCONF);
 		if (!preferred_family || preferred_family == AF_INET6)
@@ -335,19 +293,15 @@ int do_ipmonitor(int argc, char **argv)
 		if (!preferred_family || preferred_family == AF_MPLS)
 			groups |= nl_mgrp(RTNLGRP_MPLS_NETCONF);
 	}
-	if (lrule) {
+	if (lmask & IPMON_LRULE) {
 		if (!preferred_family || preferred_family == AF_INET)
 			groups |= nl_mgrp(RTNLGRP_IPV4_RULE);
 		if (!preferred_family || preferred_family == AF_INET6)
 			groups |= nl_mgrp(RTNLGRP_IPV6_RULE);
 	}
-	if (lnsid) {
+	if (lmask & IPMON_LNSID) {
 		groups |= nl_mgrp(RTNLGRP_NSID);
 	}
-	if (nh_set)
-		lnexthop = 1;
-	if (stats_set)
-		lstats = 1;
 
 	if (file) {
 		FILE *fp;
@@ -366,12 +320,14 @@ int do_ipmonitor(int argc, char **argv)
 	if (rtnl_open(&rth, groups) < 0)
 		exit(1);
 
-	if (lnexthop && rtnl_add_nl_group(&rth, RTNLGRP_NEXTHOP) < 0) {
+	if (lmask & IPMON_LNEXTHOP &&
+	    rtnl_add_nl_group(&rth, RTNLGRP_NEXTHOP) < 0) {
 		fprintf(stderr, "Failed to add nexthop group to list\n");
 		exit(1);
 	}
 
-	if (lstats && rtnl_add_nl_group(&rth, RTNLGRP_STATS) < 0) {
+	if (lmask & IPMON_LSTATS &&
+	    rtnl_add_nl_group(&rth, RTNLGRP_STATS) < 0) {
 		fprintf(stderr, "Failed to add stats group to list\n");
 		exit(1);
 	}
-- 
2.37.2