[PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[Stephen Suryaputra]

With multiple mroute tables it seems that there should be a way to
display caches and vifs for the non-default table. Add two sysctls to
control what to display. The default values for the sysctls are
RT_TABLE_DEFAULT (253) and RT6_TABLE_DFLT (254).

Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
---
 Documentation/networking/ip-sysctl.rst | 14 ++++++++++++++
 include/net/netns/ipv4.h               |  3 +++
 include/net/netns/ipv6.h               |  3 +++
 net/ipv4/af_inet.c                     |  3 +++
 net/ipv4/ipmr.c                        | 14 ++++++++++++--
 net/ipv4/sysctl_net_ipv4.c             |  9 +++++++++
 net/ipv6/ip6mr.c                       | 14 ++++++++++++--
 net/ipv6/route.c                       |  3 +++
 net/ipv6/sysctl_net_ipv6.c             |  9 +++++++++
 9 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index d91ab28718d4..de47563514f0 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -1382,6 +1382,13 @@ mc_forwarding - BOOLEAN
 	conf/all/mc_forwarding must also be set to TRUE to enable multicast
 	routing	for the interface
 
+ip_mr_table_id - UNSIGNED INTEGER
+	Only valid for kernels built with CONFIG_IP_MROUTE_MULTIPLE_TABLES and
+	CONFIG_PROC_FS enabled. It is used to set the multicast routing table id
+	to display in /proc/net/ip_mr_cache and /proc/net/ip_mr_vif
+
+	Default: 253 (RT_TABLE_DEFAULT)
+
 medium_id - INTEGER
 	Integer value used to differentiate the devices by the medium they
 	are attached to. Two devices can have different id values when
@@ -2192,6 +2199,13 @@ mtu - INTEGER
 
 	Default: 1280 (IPv6 required minimum)
 
+ip6_mr_table_id - UNSIGNED INTEGER
+	Only valid for kernels built with CONFIG_IPV6_MROUTE_MULTIPLE_TABLES and
+	CONFIG_PROC_FS enabled. It is used to set the multicast routing table id
+	to display in /proc/net/ip6_mr_cache and /proc/net/ip6_mr_vif
+
+	Default: 254 (RT6_TABLE_DFLT)
+
 ip_nonlocal_bind - BOOLEAN
 	If set, allows processes to bind() to non-local IPv6 addresses,
 	which can be quite useful - but may break some applications.
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 2f65701a43c9..1c5c6bbdda1e 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -207,6 +207,9 @@ struct netns_ipv4 {
 #else
 	struct list_head	mr_tables;
 	struct fib_rules_ops	*mr_rules_ops;
+#ifdef CONFIG_PROC_FS
+	u32 sysctl_ip_mr_table_id;
+#endif
 #endif
 #endif
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index a4b550380316..f1b9fa46ca2c 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -99,6 +99,9 @@ struct netns_ipv6 {
 #else
 	struct list_head	mr6_tables;
 	struct fib_rules_ops	*mr6_rules_ops;
+#ifdef CONFIG_PROC_FS
+	u32 sysctl_ip6_mr_table_id;
+#endif
 #endif
 #endif
 	atomic_t		dev_addr_genid;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 0e4d758c2585..2769ea08c519 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1870,6 +1870,9 @@ static __net_init int inet_init_net(struct net *net)
 
 	net->ipv4.sysctl_fib_notify_on_flag_change = 0;
 
+#if defined(CONFIG_IP_MROUTE_MULTIPLE_TABLES) && defined(CONFIG_PROC_FS)
+	net->ipv4.sysctl_ip_mr_table_id = RT_TABLE_DEFAULT;
+#endif
 	return 0;
 }
 
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 2dda856ca260..8a955b6853c6 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -2896,8 +2896,13 @@ static void *ipmr_vif_seq_start(struct seq_file *seq, loff_t *pos)
 	struct mr_vif_iter *iter = seq->private;
 	struct net *net = seq_file_net(seq);
 	struct mr_table *mrt;
+#ifdef CONFIG_IP_MROUTE_MULTIPLE_TABLES
+	u32 mr_table_id = net->ipv4.sysctl_ip_mr_table_id;
+#else
+	u32 mr_table_id = RT_TABLE_DEFAULT;
+#endif
 
-	mrt = ipmr_get_table(net, RT_TABLE_DEFAULT);
+	mrt = ipmr_get_table(net, mr_table_id);
 	if (!mrt)
 		return ERR_PTR(-ENOENT);
 
@@ -2947,8 +2952,13 @@ static void *ipmr_mfc_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct net *net = seq_file_net(seq);
 	struct mr_table *mrt;
+#ifdef CONFIG_IP_MROUTE_MULTIPLE_TABLES
+	u32 mr_table_id = net->ipv4.sysctl_ip_mr_table_id;
+#else
+	u32 mr_table_id = RT_TABLE_DEFAULT;
+#endif
 
-	mrt = ipmr_get_table(net, RT_TABLE_DEFAULT);
+	mrt = ipmr_get_table(net, mr_table_id);
 	if (!mrt)
 		return ERR_PTR(-ENOENT);
 
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 6f1e64d49232..f2133a4aab86 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -1406,6 +1406,15 @@ static struct ctl_table ipv4_net_table[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= &two,
 	},
+#if defined(CONFIG_IP_MROUTE_MULTIPLE_TABLES) && defined(CONFIG_PROC_FS)
+	{
+		.procname	= "ip_mr_table_id",
+		.data		= &init_net.ipv4.sysctl_ip_mr_table_id,
+		.maxlen		= sizeof(init_net.ipv4.sysctl_ip_mr_table_id),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
+#endif
 	{ }
 };
 
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 36ed9efb8825..56df543e298d 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -408,8 +408,13 @@ static void *ip6mr_vif_seq_start(struct seq_file *seq, loff_t *pos)
 	struct mr_vif_iter *iter = seq->private;
 	struct net *net = seq_file_net(seq);
 	struct mr_table *mrt;
+#ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
+	u32 mr_table_id = net->ipv6.sysctl_ip6_mr_table_id;
+#else
+	u32 mr_table_id = RT6_TABLE_DFLT;
+#endif
 
-	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
+	mrt = ip6mr_get_table(net, mr_table_id);
 	if (!mrt)
 		return ERR_PTR(-ENOENT);
 
@@ -458,8 +463,13 @@ static void *ipmr_mfc_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct net *net = seq_file_net(seq);
 	struct mr_table *mrt;
+#ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
+	u32 mr_table_id = net->ipv6.sysctl_ip6_mr_table_id;
+#else
+	u32 mr_table_id = RT6_TABLE_DFLT;
+#endif
 
-	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
+	mrt = ip6mr_get_table(net, mr_table_id);
 	if (!mrt)
 		return ERR_PTR(-ENOENT);
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 6cf4bb89ca69..5c91546abc26 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -6470,6 +6470,9 @@ static int __net_init ip6_route_net_init(struct net *net)
 
 	net->ipv6.ip6_rt_gc_expire = 30*HZ;
 
+#if defined(CONFIG_IPV6_MROUTE_MULTIPLE_TABLES) && defined(CONFIG_PROC_FS)
+	net->ipv6.sysctl_ip6_mr_table_id = RT6_TABLE_DFLT;
+#endif
 	ret = 0;
 out:
 	return ret;
diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
index d53dd142bf87..053314dbbfff 100644
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -215,6 +215,15 @@ static struct ctl_table ipv6_table_template[] = {
 		.proc_handler	= proc_doulongvec_minmax,
 		.extra2		= &ioam6_id_wide_max,
 	},
+#if defined(CONFIG_IPV6_MROUTE_MULTIPLE_TABLES) && defined(CONFIG_PROC_FS)
+	{
+		.procname	= "ip6_mr_table_id",
+		.data		= &init_net.ipv6.sysctl_ip6_mr_table_id,
+		.maxlen		= sizeof(init_net.ipv6.sysctl_ip6_mr_table_id),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
+#endif
 	{ }
 };
 
-- 
2.25.1

Re: [PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[Nikolay Aleksandrov]

On 18/08/2021 23:09, Stephen Suryaputra wrote:
> With multiple mroute tables it seems that there should be a way to
> display caches and vifs for the non-default table. Add two sysctls to
> control what to display. The default values for the sysctls are
> RT_TABLE_DEFAULT (253) and RT6_TABLE_DFLT (254).
> 
> Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
> ---
>  Documentation/networking/ip-sysctl.rst | 14 ++++++++++++++
>  include/net/netns/ipv4.h               |  3 +++
>  include/net/netns/ipv6.h               |  3 +++
>  net/ipv4/af_inet.c                     |  3 +++
>  net/ipv4/ipmr.c                        | 14 ++++++++++++--
>  net/ipv4/sysctl_net_ipv4.c             |  9 +++++++++
>  net/ipv6/ip6mr.c                       | 14 ++++++++++++--
>  net/ipv6/route.c                       |  3 +++
>  net/ipv6/sysctl_net_ipv6.c             |  9 +++++++++
>  9 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index d91ab28718d4..de47563514f0 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -1382,6 +1382,13 @@ mc_forwarding - BOOLEAN
>  	conf/all/mc_forwarding must also be set to TRUE to enable multicast
>  	routing	for the interface

Sorry, but I don't see any point to this. We don't have it for any of the other
non-default cases, and I don't see a point of having it for ipmr either.
If you'd like to display the non-default tables then you query for them, you
don't change a sysctl to see them in /proc.
It sounds like a workaround for an issue that is not solved properly, and
generally it shouldn't be using /proc. If netlink interfaces are not sufficient
please improve them.

Why do we need a whole new sysctl or net proc entries ?

Cheers,
 Nik

Re: [PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[Stephen Suryaputra]

On Thu, Aug 19, 2021 at 01:37:21AM +0300, Nikolay Aleksandrov wrote:
> 
> Sorry, but I don't see any point to this. We don't have it for any of the other
> non-default cases, and I don't see a point of having it for ipmr either.
> If you'd like to display the non-default tables then you query for them, you
> don't change a sysctl to see them in /proc.
> It sounds like a workaround for an issue that is not solved properly, and
> generally it shouldn't be using /proc. If netlink interfaces are not sufficient
> please improve them.

We found that the ability to dump the tables from kernel point of view
is valuable for debugging the applications. Sometimes during the
development, bugs in the use of the netlink interfaces can be solved
quickly if the tables in the kernel can be viewed easily.
> 
> Why do we need a whole new sysctl or net proc entries ?

If you agree on the reasoning above, what do you recommend then? Again,
this is to easily view what's in the kernel.

Thanks,
Stephen.

Re: [PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[Andrew Lunn]

On Wed, Aug 18, 2021 at 06:50:22PM -0400, Stephen Suryaputra wrote:
> On Thu, Aug 19, 2021 at 01:37:21AM +0300, Nikolay Aleksandrov wrote:
> > 
> > Sorry, but I don't see any point to this. We don't have it for any of the other
> > non-default cases, and I don't see a point of having it for ipmr either.
> > If you'd like to display the non-default tables then you query for them, you
> > don't change a sysctl to see them in /proc.
> > It sounds like a workaround for an issue that is not solved properly, and
> > generally it shouldn't be using /proc. If netlink interfaces are not sufficient
> > please improve them.
> 
> We found that the ability to dump the tables from kernel point of view
> is valuable for debugging the applications. Sometimes during the
> development, bugs in the use of the netlink interfaces can be solved
> quickly if the tables in the kernel can be viewed easily.
>
> If you agree on the reasoning above, what do you recommend then? Again,
> this is to easily view what's in the kernel.

Does iproute2 allow you to dump the tables?

First work on a simple CLI tool to dump the tables. Make it bug free
and contribute it. Then work on your buggy multicast routing daemon.

    Andrew

Re: [PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[Nikolay Aleksandrov]

On 19/08/2021 01:50, Stephen Suryaputra wrote:
> On Thu, Aug 19, 2021 at 01:37:21AM +0300, Nikolay Aleksandrov wrote:
>>
>> Sorry, but I don't see any point to this. We don't have it for any of the other
>> non-default cases, and I don't see a point of having it for ipmr either.
>> If you'd like to display the non-default tables then you query for them, you
>> don't change a sysctl to see them in /proc.
>> It sounds like a workaround for an issue that is not solved properly, and
>> generally it shouldn't be using /proc. If netlink interfaces are not sufficient
>> please improve them.
> 
> We found that the ability to dump the tables from kernel point of view
> is valuable for debugging the applications. Sometimes during the
> development, bugs in the use of the netlink interfaces can be solved
> quickly if the tables in the kernel can be viewed easily.

What does that mean, are there bugs in netlink dumping capabilities?
If so, fix those. You can already dump all kernel tables via netlink, if there's
something missing just add it.

>>
>> Why do we need a whole new sysctl or net proc entries ?
> 
> If you agree on the reasoning above, what do you recommend then? Again,
> this is to easily view what's in the kernel.
> 

I do not. You can already use netlink to dump any table, I don't see any point
in making those available via /proc, it's not a precedent. We have a ton of other
(almost any) information already exported via netlink without any need for /proc,
there really is no argument to add this new support.

> Thanks,
> Stephen.
> 

Re: [PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[David Ahern]

On 8/18/21 5:12 PM, Nikolay Aleksandrov wrote:
> I do not. You can already use netlink to dump any table, I don't see any point
> in making those available via /proc, it's not a precedent. We have a ton of other
> (almost any) information already exported via netlink without any need for /proc,
> there really is no argument to add this new support.

agreed. From a routing perspective /proc files are very limiting. You
really need to be using netlink and table dumps. iproute2 and kernel
infra exist to efficiently request the dump of a specific table. What is
missing beyond that?

Re: [PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[Stephen Suryaputra]

On Wed, Aug 18, 2021 at 06:04:12PM -0600, David Ahern wrote:
> On 8/18/21 5:12 PM, Nikolay Aleksandrov wrote:
> > I do not. You can already use netlink to dump any table, I don't see any point
> > in making those available via /proc, it's not a precedent. We have a ton of other
> > (almost any) information already exported via netlink without any need for /proc,
> > there really is no argument to add this new support.
> 
> agreed. From a routing perspective /proc files are very limiting. You
> really need to be using netlink and table dumps. iproute2 and kernel
> infra exist to efficiently request the dump of a specific table. What is
> missing beyond that?

On this, I realized now that without /proc the multicast caches can be
displayed using iproute2. But, it doesn't seem to have support to
display vifs. Is there a public domain command line utility that can
display vifs on non default table, i.e. the one that uses the support in
the kernel in commit 772c344dbb23 ("net: ipmr: add getlink support")?

Thanks.

Re: [PATCH net-next] ipmr: ip6mr: Add ability to display non default caches and vifs

[Nikolay Aleksandrov]

On 21/09/2021 02:49, Stephen Suryaputra wrote:
> On Wed, Aug 18, 2021 at 06:04:12PM -0600, David Ahern wrote:
>> On 8/18/21 5:12 PM, Nikolay Aleksandrov wrote:
>>> I do not. You can already use netlink to dump any table, I don't see any point
>>> in making those available via /proc, it's not a precedent. We have a ton of other
>>> (almost any) information already exported via netlink without any need for /proc,
>>> there really is no argument to add this new support.
>>
>> agreed. From a routing perspective /proc files are very limiting. You
>> really need to be using netlink and table dumps. iproute2 and kernel
>> infra exist to efficiently request the dump of a specific table. What is
>> missing beyond that?
> 
> On this, I realized now that without /proc the multicast caches can be
> displayed using iproute2. But, it doesn't seem to have support to
> display vifs. Is there a public domain command line utility that can
> display vifs on non default table, i.e. the one that uses the support in
> the kernel in commit 772c344dbb23 ("net: ipmr: add getlink support")?
> 
> Thanks.
> 

Not that I know of, this was supposed to be used by FRR but other things were
eventually given higher priority and it got sidelined. I'm sure at some point
we'll get to using it in iproute2. The netlink interface can also be used to
extend the number of vifs in a dynamic way, it'd be nice if we finally deprecated
the old static and non-extendable interface and started using netlink for ipmr.

Cheers,
 Nik