[PATCH] ipv6: enable net.ipv6.route sysctls in network namespace

We want to increase route cache size in network namespace
created with user namespace. Currently ipv6 route settings
are disabled for non-initial network namespaces.
Since routes are per network namespace it is safe
to enable these sysctls.

Signed-off-by: Alexander Kuznetsov <redacted>
Acked-by: Dmitry Yakunin <redacted>
---
 net/ipv6/route.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b6ddf23..de85e3b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -6415,10 +6415,6 @@ struct ctl_table * __net_init ipv6_route_sysctl_init(struct net *net)
 		table[8].data = &net->ipv6.sysctl.ip6_rt_min_advmss;
 		table[9].data = &net->ipv6.sysctl.ip6_rt_gc_min_interval;
 		table[10].data = &net->ipv6.sysctl.skip_notify_on_dev_down;
-
-		/* Don't export sysctls to unprivileged users */
-		if (net->user_ns != &init_user_ns)
-			table[0].procname = NULL;
 	}
 
 	return table;

-- 
2.7.4

On Tue, 21 Sep 2021 09:22:04 +0300 Alexander Kuznetsov wrote:

We want to increase route cache size in network namespace
created with user namespace. Currently ipv6 route settings
are disabled for non-initial network namespaces.
Since routes are per network namespace it is safe
to enable these sysctls.

Signed-off-by: Alexander Kuznetsov <redacted>
Acked-by: Dmitry Yakunin <redacted>

Your CC list is very narrow. IMO you should CC Eric B on this, 
at the very least.

Why only remove this part and not any other part of 464dc801c76aa?

I don't know much about user ns, are we making an assumption here that
this user ns corresponds to a net ns? Or just because it's _possible_
to make them 1:1 we can shift the decision to the admin?

Jakub Kicinski [off-list ref] wrote:

On Tue, 21 Sep 2021 09:22:04 +0300 Alexander Kuznetsov wrote:

quoted

We want to increase route cache size in network namespace
created with user namespace. Currently ipv6 route settings
are disabled for non-initial network namespaces.
Since routes are per network namespace it is safe
to enable these sysctls.

Are routes accounted towards memcg or something like that?

Otherwise userns could start eating up memory by cranking the limit
up to 11 and just adds a gazillion routes?

On 9/21/21 9:32 AM, Florian Westphal wrote:

Jakub Kicinski [off-list ref] wrote:

quoted

On Tue, 21 Sep 2021 09:22:04 +0300 Alexander Kuznetsov wrote:

quoted

We want to increase route cache size in network namespace
created with user namespace. Currently ipv6 route settings
are disabled for non-initial network namespaces.
Since routes are per network namespace it is safe
to enable these sysctls.

Are routes accounted towards memcg or something like that?

Otherwise userns could start eating up memory by cranking the limit
up to 11 and just adds a gazillion routes?

Adding FIB entries I believe is now handled after commit:

commit 6126891c6d4f6f4ef50323d2020635ee255a796e
Author: Vasily Averin [off-list ref]
Date:   Mon Jul 19 13:44:31 2021 +0300

    memcg: enable accounting for IP address and routing-related objects


The ip6_rt_max_size sysctl manages the number of dst entries (cached
dst's and exceptions) that can be created, and there should be some
limit that network namespace users can not exceed.

On 21 Sep 2021, at 16:09, Jakub Kicinski [off-list ref] wrote:

On Tue, 21 Sep 2021 09:22:04 +0300 Alexander Kuznetsov wrote:

quoted

We want to increase route cache size in network namespace
created with user namespace. Currently ipv6 route settings
are disabled for non-initial network namespaces.
Since routes are per network namespace it is safe
to enable these sysctls.

Signed-off-by: Alexander Kuznetsov <redacted>
Acked-by: Dmitry Yakunin <redacted>

Your CC list is very narrow. IMO you should CC Eric B on this, 
at the very least.

Why only remove this part and not any other part of 464dc801c76aa?

We remove this part by analogy with 5cdda5f1d6add and enable only sysctls that we need.

quoted

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b6ddf23..de85e3b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -6415,10 +6415,6 @@ struct ctl_table * __net_init ipv6_route_sysctl_init(struct net *net)

		table[8].data = &net->ipv6.sysctl.ip6_rt_min_advmss;
		table[9].data = &net->ipv6.sysctl.ip6_rt_gc_min_interval;
		table[10].data = &net->ipv6.sysctl.skip_notify_on_dev_down;
-
-		/* Don't export sysctls to unprivileged users */
-		if (net->user_ns != &init_user_ns)
-			table[0].procname = NULL;
	}

	return table;

I don't know much about user ns, are we making an assumption here that
this user ns corresponds to a net ns? Or just because it's _possible_
to make them 1:1 we can shift the decision to the admin?

Sorry, but I don't understand what you mean.

Alexander Kuznetsov [off-list ref] writes:

We want to increase route cache size in network namespace
created with user namespace. Currently ipv6 route settings
are disabled for non-initial network namespaces.
Since routes are per network namespace it is safe
to enable these sysctls.

The case where this matters is when the network namespaces is created by
by the root user in a user namespace.  AKA this is something we allow
any user to do.

These were disabled because out of an abundance of caution rather than
any particular policy when the kernel started allowing non-root users
to create network namespaces.

That said it would really help if your commit message listed the
sysctls you are enabling and listed why it was safe to enable them.

That is it would really help if you performed the review that says
the sysctls are safe for ordinary users to use and that they won't
enable DOS attacks or the like.

If you just care about the route cache size you can only enable
that sysctl.

These are the 10 sysctls you are enabling.  All you talk about
in your commit message is route cache size which I believe is
the 3rd entry in the table net->ipv6.sysctl.ip6_rt_max_size.
It certainly is not all of them.

		table[0].data = &net->ipv6.sysctl.flush_delay;
		table[1].data = &net->ipv6.ip6_dst_ops.gc_thresh;
		table[2].data = &net->ipv6.sysctl.ip6_rt_max_size;
		table[3].data = &net->ipv6.sysctl.ip6_rt_gc_min_interval;
		table[4].data = &net->ipv6.sysctl.ip6_rt_gc_timeout;
		table[5].data = &net->ipv6.sysctl.ip6_rt_gc_interval;
		table[6].data = &net->ipv6.sysctl.ip6_rt_gc_elasticity;
		table[7].data = &net->ipv6.sysctl.ip6_rt_mtu_expires;
		table[8].data = &net->ipv6.sysctl.ip6_rt_min_advmss;
		table[9].data = &net->ipv6.sysctl.ip6_rt_gc_min_interval;
		table[10].data = &net->ipv6.sysctl.skip_notify_on_dev_down;


I took a quick look and we don't enable any of these for ipv4 either.

I suspect it is probably reasonable to enable these sysctls for
all users of the system to use, but can we please show the reason
for each sysctl why it is safe?

Thank you.

Eric