[PATCHv2 next] net: add option to not create fall-back tunnels in root-ns as well

[PATCHv2 next] net: add option to not create fall-back tunnels in root-ns as well

[Mahesh Bandewar]

The sysctl that was added  earlier by commit 79134e6ce2c ("net: do
not create fallback tunnels for non-default namespaces") to create
fall-back only in root-ns. This patch enhances that behavior to provide
option not to create fallback tunnels in root-ns as well. Since modules
that create fallback tunnels could be built-in and setting the sysctl
value after booting is pointless, so added a kernel cmdline options to
change this default. The default setting is preseved for backward
compatibility. The kernel command line option of fb_tunnels=initns will
set the sysctl value to 1 and will create fallback tunnels only in initns
while kernel cmdline fb_tunnels=none will set the sysctl value to 2 and
fallback tunnels are skipped in every netns.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Maciej Zenczykowski <maze@google.com>
Cc: Jian Yang <jianyang@google.com>
---
v1->v2
  Removed the Kconfig option which would force rebuild and replaced with
  kcmd-line option

 .../admin-guide/kernel-parameters.txt         |  5 +++++
 Documentation/admin-guide/sysctl/net.rst      | 20 +++++++++++++------
 include/linux/netdevice.h                     |  7 ++++++-
 net/core/sysctl_net_core.c                    | 17 ++++++++++++++--
 4 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a1068742a6df..09a51598c792 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -801,6 +801,11 @@
 
 	debug_objects	[KNL] Enable object debugging
 
+	fb_tunnels=	[NET]
+			Format: { initns | none }
+			See Documentation/admin-guide/sysctl/net.rst for
+			fb_tunnels_only_for_init_ns
+
 	no_debug_objects
 			[KNL] Disable object debugging
 
diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
index 42cd04bca548..57fd6ce68fe0 100644
--- a/Documentation/admin-guide/sysctl/net.rst
+++ b/Documentation/admin-guide/sysctl/net.rst
@@ -300,7 +300,6 @@ Note:
       0:    0     1     2     3     4     5     6     7
   RSS hash key:
   84:50:f4:00:a8:15:d1:a7:e9:7f:1d:60:35:c7:47:25:42:97:74:ca:56:bb:b6:a1:d8:43:e3:c9:0c:fd:17:55:c2:3a:4d:69:ed:f1:42:89
-
 netdev_tstamp_prequeue
 ----------------------
 
@@ -321,11 +320,20 @@ fb_tunnels_only_for_init_net
 ----------------------------
 
 Controls if fallback tunnels (like tunl0, gre0, gretap0, erspan0,
-sit0, ip6tnl0, ip6gre0) are automatically created when a new
-network namespace is created, if corresponding tunnel is present
-in initial network namespace.
-If set to 1, these devices are not automatically created, and
-user space is responsible for creating them if needed.
+sit0, ip6tnl0, ip6gre0) are automatically created. There are 3 possibilities
+(a) value = 0; respective fallback tunnels are created when module is
+loaded in every net namespaces (backward compatible behavior).
+(b) value = 1; [kcmd value: initns] respective fallback tunnels are
+created only in init net namespace and every other net namespace will
+not have them.
+(c) value = 2; [kcmd value: none] fallback tunnels are not created
+when a module is loaded in any of the net namespace. Setting value to
+"2" is pointless after boot if these modules are built-in, so there is
+a kernel command-line option that can change this default. Please refer to
+Documentation/admin-guide/kernel-parameters.txt for additional details.
+
+Not creating fallback tunnels gives control to userspace to create
+whatever is needed only and avoid creating devices which are redundant.
 
 Default : 0  (for compatibility reasons)
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b0e303f6603f..7efcdb9ee4ff 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -640,9 +640,14 @@ struct netdev_queue {
 extern int sysctl_fb_tunnels_only_for_init_net;
 extern int sysctl_devconf_inherit_init_net;
 
+/*
+ * sysctl_fb_tunnels_only_for_init_net == 0 : For all netns
+ *                                     == 1 : For initns only
+ *                                     == 2 : For none.
+ */
 static inline bool net_has_fallback_tunnels(const struct net *net)
 {
-	return net == &init_net ||
+	return (net == &init_net && sysctl_fb_tunnels_only_for_init_net == 1) ||
 	       !IS_ENABLED(CONFIG_SYSCTL) ||
 	       !sysctl_fb_tunnels_only_for_init_net;
 }
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index 6ada114bbcca..d86d8d11cfe4 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -22,7 +22,7 @@
 #include <net/busy_poll.h>
 #include <net/pkt_sched.h>
 
-static int two __maybe_unused = 2;
+static int two = 2;
 static int three = 3;
 static int min_sndbuf = SOCK_MIN_SNDBUF;
 static int min_rcvbuf = SOCK_MIN_RCVBUF;
@@ -546,7 +546,7 @@ static struct ctl_table net_core_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
+		.extra2		= &two,
 	},
 	{
 		.procname	= "devconf_inherit_init_net",
@@ -587,6 +587,19 @@ static struct ctl_table netns_core_table[] = {
 	{ }
 };
 
+static int __init fb_tunnels_only_for_init_net_sysctl_setup(char *str)
+{
+	/* fallback tunnels for initns only */
+	if (!strncmp(str, "initns", 6))
+		sysctl_fb_tunnels_only_for_init_net = 1;
+	/* no fallback tunnels anywhere */
+	else if (!strncmp(str, "none", 4))
+		sysctl_fb_tunnels_only_for_init_net = 2;
+
+	return 1;
+}
+__setup("fb_tunnels=", fb_tunnels_only_for_init_net_sysctl_setup);
+
 static __net_init int sysctl_core_net_init(struct net *net)
 {
 	struct ctl_table *tbl;
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCHv2 next] net: add option to not create fall-back tunnels in root-ns as well

[Randy Dunlap]

On 8/25/20 3:42 PM, Mahesh Bandewar wrote:
> The sysctl that was added  earlier by commit 79134e6ce2c ("net: do
> not create fallback tunnels for non-default namespaces") to create
> fall-back only in root-ns. This patch enhances that behavior to provide
> option not to create fallback tunnels in root-ns as well. Since modules
> that create fallback tunnels could be built-in and setting the sysctl
> value after booting is pointless, so added a kernel cmdline options to
> change this default. The default setting is preseved for backward
> compatibility. The kernel command line option of fb_tunnels=initns will
> set the sysctl value to 1 and will create fallback tunnels only in initns
> while kernel cmdline fb_tunnels=none will set the sysctl value to 2 and
> fallback tunnels are skipped in every netns.
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Maciej Zenczykowski <maze@google.com>
> Cc: Jian Yang <jianyang@google.com>
> ---
> v1->v2
>   Removed the Kconfig option which would force rebuild and replaced with
>   kcmd-line option
> 
>  .../admin-guide/kernel-parameters.txt         |  5 +++++
>  Documentation/admin-guide/sysctl/net.rst      | 20 +++++++++++++------
>  include/linux/netdevice.h                     |  7 ++++++-
>  net/core/sysctl_net_core.c                    | 17 ++++++++++++++--
>  4 files changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a1068742a6df..09a51598c792 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -801,6 +801,11 @@
>  
>  	debug_objects	[KNL] Enable object debugging
>  
> +	fb_tunnels=	[NET]
> +			Format: { initns | none }
> +			See Documentation/admin-guide/sysctl/net.rst for
> +			fb_tunnels_only_for_init_ns
> +

Not at this location in this file.
Entries in this file are meant to be in alphabetical order (mostly).

So leave debug_objects and no_debug_objects together, and insert fb_tunnels
between fail_make_request= and floppy=.

Thanks.

>  	no_debug_objects
>  			[KNL] Disable object debugging
>  

-- 
~Randy

Re: [PATCHv2 next] net: add option to not create fall-back tunnels in root-ns as well

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Aug 25, 2020 at 3:47 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 8/25/20 3:42 PM, Mahesh Bandewar wrote:
> > The sysctl that was added  earlier by commit 79134e6ce2c ("net: do
> > not create fallback tunnels for non-default namespaces") to create
> > fall-back only in root-ns. This patch enhances that behavior to provide
> > option not to create fallback tunnels in root-ns as well. Since modules
> > that create fallback tunnels could be built-in and setting the sysctl
> > value after booting is pointless, so added a kernel cmdline options to
> > change this default. The default setting is preseved for backward
> > compatibility. The kernel command line option of fb_tunnels=initns will
> > set the sysctl value to 1 and will create fallback tunnels only in initns
> > while kernel cmdline fb_tunnels=none will set the sysctl value to 2 and
> > fallback tunnels are skipped in every netns.
> >
> > Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Maciej Zenczykowski <maze@google.com>
> > Cc: Jian Yang <jianyang@google.com>
> > ---
> > v1->v2
> >   Removed the Kconfig option which would force rebuild and replaced with
> >   kcmd-line option
> >
> >  .../admin-guide/kernel-parameters.txt         |  5 +++++
> >  Documentation/admin-guide/sysctl/net.rst      | 20 +++++++++++++------
> >  include/linux/netdevice.h                     |  7 ++++++-
> >  net/core/sysctl_net_core.c                    | 17 ++++++++++++++--
> >  4 files changed, 40 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index a1068742a6df..09a51598c792 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -801,6 +801,11 @@
> >
> >       debug_objects   [KNL] Enable object debugging
> >
> > +     fb_tunnels=     [NET]
> > +                     Format: { initns | none }
> > +                     See Documentation/admin-guide/sysctl/net.rst for
> > +                     fb_tunnels_only_for_init_ns
> > +
>
> Not at this location in this file.
> Entries in this file are meant to be in alphabetical order (mostly).
>
> So leave debug_objects and no_debug_objects together, and insert fb_tunnels
> between fail_make_request= and floppy=.
>
I see. I'll fix it in the next revision.
thanks for the suggestion.
--mahesh..

> Thanks.
>
> >       no_debug_objects
> >                       [KNL] Disable object debugging
> >
>
> --
> ~Randy
>

Re: [PATCHv2 next] net: add option to not create fall-back tunnels in root-ns as well

[Maciej Żenczykowski]

On Tue, Aug 25, 2020 at 4:00 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Tue, Aug 25, 2020 at 3:47 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> >
> > On 8/25/20 3:42 PM, Mahesh Bandewar wrote:
> > > The sysctl that was added  earlier by commit 79134e6ce2c ("net: do
> > > not create fallback tunnels for non-default namespaces") to create
> > > fall-back only in root-ns. This patch enhances that behavior to provide
> > > option not to create fallback tunnels in root-ns as well. Since modules
> > > that create fallback tunnels could be built-in and setting the sysctl
> > > value after booting is pointless, so added a kernel cmdline options to
> > > change this default. The default setting is preseved for backward
> > > compatibility. The kernel command line option of fb_tunnels=initns will
> > > set the sysctl value to 1 and will create fallback tunnels only in initns
> > > while kernel cmdline fb_tunnels=none will set the sysctl value to 2 and
> > > fallback tunnels are skipped in every netns.
> > >
> > > Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Maciej Zenczykowski <maze@google.com>
> > > Cc: Jian Yang <jianyang@google.com>
> > > ---
> > > v1->v2
> > >   Removed the Kconfig option which would force rebuild and replaced with
> > >   kcmd-line option
> > >
> > >  .../admin-guide/kernel-parameters.txt         |  5 +++++
> > >  Documentation/admin-guide/sysctl/net.rst      | 20 +++++++++++++------
> > >  include/linux/netdevice.h                     |  7 ++++++-
> > >  net/core/sysctl_net_core.c                    | 17 ++++++++++++++--
> > >  4 files changed, 40 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index a1068742a6df..09a51598c792 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -801,6 +801,11 @@
> > >
> > >       debug_objects   [KNL] Enable object debugging
> > >
> > > +     fb_tunnels=     [NET]
> > > +                     Format: { initns | none }
> > > +                     See Documentation/admin-guide/sysctl/net.rst for
> > > +                     fb_tunnels_only_for_init_ns
> > > +
> >
> > Not at this location in this file.
> > Entries in this file are meant to be in alphabetical order (mostly).
> >
> > So leave debug_objects and no_debug_objects together, and insert fb_tunnels
> > between fail_make_request= and floppy=.
> >
> I see. I'll fix it in the next revision.
> thanks for the suggestion.
> --mahesh..
>
> > Thanks.
> >
> > >       no_debug_objects
> > >                       [KNL] Disable object debugging
> > >
> >
> > --
> > ~Randy

Setting it to 1 via kcmdline doesn't seem all that useful, since
instead of that you can just use initrc/sysctl.conf/etc.

Would it be simpler if it was just 'no_fb_tunnels' or
'no_fallback_tunnels' and the function just set the sysctl to 2
unconditionally?
(ie. no =.... parsing at all) that would also be less code...

btw. I also don't understand the '!IS_ENABLED(CONFIG_SYSCTL) ||'
piece.  Why not just delete that?
This seems to force fallback tunnels if you disable CONFIG_SYSCTL...
but (a) then the kcmdline option doesn't work,
and (b) that should already just happen by virtue of the sysctl defaulting to 0.

Re: [PATCHv2 next] net: add option to not create fall-back tunnels in root-ns as well

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Aug 25, 2020 at 5:42 PM Maciej Żenczykowski <maze@google.com> wrote:
>
> On Tue, Aug 25, 2020 at 4:00 PM Mahesh Bandewar (महेश बंडेवार)
> <maheshb@google.com> wrote:
> >
> > On Tue, Aug 25, 2020 at 3:47 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > >
> > > On 8/25/20 3:42 PM, Mahesh Bandewar wrote:
> > > > The sysctl that was added  earlier by commit 79134e6ce2c ("net: do
> > > > not create fallback tunnels for non-default namespaces") to create
> > > > fall-back only in root-ns. This patch enhances that behavior to provide
> > > > option not to create fallback tunnels in root-ns as well. Since modules
> > > > that create fallback tunnels could be built-in and setting the sysctl
> > > > value after booting is pointless, so added a kernel cmdline options to
> > > > change this default. The default setting is preseved for backward
> > > > compatibility. The kernel command line option of fb_tunnels=initns will
> > > > set the sysctl value to 1 and will create fallback tunnels only in initns
> > > > while kernel cmdline fb_tunnels=none will set the sysctl value to 2 and
> > > > fallback tunnels are skipped in every netns.
> > > >
> > > > Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> > > > Cc: Eric Dumazet <edumazet@google.com>
> > > > Cc: Maciej Zenczykowski <maze@google.com>
> > > > Cc: Jian Yang <jianyang@google.com>
> > > > ---
> > > > v1->v2
> > > >   Removed the Kconfig option which would force rebuild and replaced with
> > > >   kcmd-line option
> > > >
> > > >  .../admin-guide/kernel-parameters.txt         |  5 +++++
> > > >  Documentation/admin-guide/sysctl/net.rst      | 20 +++++++++++++------
> > > >  include/linux/netdevice.h                     |  7 ++++++-
> > > >  net/core/sysctl_net_core.c                    | 17 ++++++++++++++--
> > > >  4 files changed, 40 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > index a1068742a6df..09a51598c792 100644
> > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > @@ -801,6 +801,11 @@
> > > >
> > > >       debug_objects   [KNL] Enable object debugging
> > > >
> > > > +     fb_tunnels=     [NET]
> > > > +                     Format: { initns | none }
> > > > +                     See Documentation/admin-guide/sysctl/net.rst for
> > > > +                     fb_tunnels_only_for_init_ns
> > > > +
> > >
> > > Not at this location in this file.
> > > Entries in this file are meant to be in alphabetical order (mostly).
> > >
> > > So leave debug_objects and no_debug_objects together, and insert fb_tunnels
> > > between fail_make_request= and floppy=.
> > >
> > I see. I'll fix it in the next revision.
> > thanks for the suggestion.
> > --mahesh..
> >
> > > Thanks.
> > >
> > > >       no_debug_objects
> > > >                       [KNL] Disable object debugging
> > > >
> > >
> > > --
> > > ~Randy
>
> Setting it to 1 via kcmdline doesn't seem all that useful, since
> instead of that you can just use initrc/sysctl.conf/etc.
>
> Would it be simpler if it was just 'no_fb_tunnels' or
> 'no_fallback_tunnels' and the function just set the sysctl to 2
> unconditionally?
> (ie. no =.... parsing at all) that would also be less code...
>
To make it simple; all methods should be able to set all values. Otherwise I
agree that it makes less sense to set value = 1 via kcmd. Also one can assign
value = 2 to sysctl once kernel is booted, it may not produce desired results
always but would work if you load modules after changing the sysctl value. I
guess the idea here is to give user full control on what their
situation is and choose
the correct method for the desired end result.

> btw. I also don't understand the '!IS_ENABLED(CONFIG_SYSCTL) ||'
> piece.  Why not just delete that?
> This seems to force fallback tunnels if you disable CONFIG_SYSCTL...
> but (a) then the kcmdline option doesn't work,
> and (b) that should already just happen by virtue of the sysctl defaulting to 0.
agreed. will remove it (!IS_ENABLED(CONFIG_SYSCTL) check) in the next revision.