| Message ID | 20210818200951.7621-1-ssuryaextr@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ipmr: ip6mr: Add ability to display non default caches and vifs | expand |
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
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.
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
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. >
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 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.
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
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 { } };
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(-)