[PATCH v1 net-next 0/6] net: Random cleanup for netns initialisation.

[PATCH v1 net-next 0/6] net: Random cleanup for netns initialisation.

[Kuniyuki Iwashima]

patch 1 & 2 suppress unwanted memory allocation for net->gen->ptr[].

patch 3 ~ 6 move part of netns initialisation to prenet_init() that
do not reqruire pernet_ops_rwsem.


Kuniyuki Iwashima (6):
  l2tp: Don't assign net->gen->ptr[] for pppol2tp_net_ops.
  net: Don't register pernet_operations if only one of id or size is
    specified.
  net: Initialise net->passive once in preinit_net().
  net: Call preinit_net() without pernet_ops_rwsem.
  net: Slim down setup_net().
  net: Initialise net.core sysctl defaults in preinit_net().

 include/net/net_namespace.h |  4 +-
 net/core/net_namespace.c    | 84 ++++++++++++++++---------------------
 net/l2tp/l2tp_ppp.c         |  3 --
 3 files changed, 39 insertions(+), 52 deletions(-)

-- 
2.30.2

[PATCH v1 net-next 1/6] l2tp: Don't assign net->gen->ptr[] for pppol2tp_net_ops.

[Kuniyuki Iwashima]

Commit fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and
ppp parts") converted net->gen->ptr[pppol2tp_net_id] in l2tp_ppp.c to
net->gen->ptr[l2tp_net_id] in l2tp_core.c.

Now the leftover wastes one entry of net->gen->ptr[] in each netns.

Let's avoid the unwanted allocation.

Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---
CC: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_ppp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 3596290047b2..246089b17910 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1406,8 +1406,6 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname,
  * L2TPv2, we dump only L2TPv2 tunnels and sessions here.
  *****************************************************************************/
 
-static unsigned int pppol2tp_net_id;
-
 #ifdef CONFIG_PROC_FS
 
 struct pppol2tp_seq_data {
@@ -1641,7 +1639,6 @@ static __net_exit void pppol2tp_exit_net(struct net *net)
 static struct pernet_operations pppol2tp_net_ops = {
 	.init = pppol2tp_init_net,
 	.exit = pppol2tp_exit_net,
-	.id   = &pppol2tp_net_id,
 };
 
 /*****************************************************************************
-- 
2.30.2

Re: [PATCH v1 net-next 1/6] l2tp: Don't assign net->gen->ptr[] for pppol2tp_net_ops.

[Jakub Kicinski]

On Mon, 29 Jul 2024 14:07:56 -0700 Kuniyuki Iwashima wrote:
> Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")

pourquoi the Fixes tag? 

Re: [PATCH v1 net-next 1/6] l2tp: Don't assign net->gen->ptr[] for pppol2tp_net_ops.

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 30 Jul 2024 18:52:19 -0700
> On Mon, 29 Jul 2024 14:07:56 -0700 Kuniyuki Iwashima wrote:
> > Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
> 
> pourquoi the Fixes tag? 

Ah, forgot to remove it.
Will remove it in v2.

Merci!

P.S. TIL pourquoi :)

[PATCH v1 net-next 2/6] net: Don't register pernet_operations if only one of id or size is specified.

[Kuniyuki Iwashima]

We can allocate per-netns memory for struct pernet_operations by specifying
id and size.

register_pernet_operations() assigns an id to pernet_operations and later
ops_init() allocates the specified size of memory as net->gen->ptr[id].

If id is missing, no memory is allocated.  If size is not specified,
pernet_operations just wastes an entry of net->gen->ptr[] for every netns.

net_generic is useful only when both id and size are specified, so let's
ensure that.

While we are at it, we add const to both fields.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/net_namespace.h |  4 ++--
 net/core/net_namespace.c    | 12 ++++++++----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 20c34bd7a077..e67b483cc8bb 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -451,8 +451,8 @@ struct pernet_operations {
 	/* Following method is called with RTNL held. */
 	void (*exit_batch_rtnl)(struct list_head *net_exit_list,
 				struct list_head *dev_kill_list);
-	unsigned int *id;
-	size_t size;
+	unsigned int * const id;
+	const size_t size;
 };
 
 /*
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 6a823ba906c6..1d164ae097a5 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -125,7 +125,7 @@ static int ops_init(const struct pernet_operations *ops, struct net *net)
 	int err = -ENOMEM;
 	void *data = NULL;
 
-	if (ops->id && ops->size) {
+	if (ops->id) {
 		data = kzalloc(ops->size, GFP_KERNEL);
 		if (!data)
 			goto out;
@@ -140,7 +140,7 @@ static int ops_init(const struct pernet_operations *ops, struct net *net)
 	if (!err)
 		return 0;
 
-	if (ops->id && ops->size) {
+	if (ops->id) {
 		ng = rcu_dereference_protected(net->gen,
 					       lockdep_is_held(&pernet_ops_rwsem));
 		ng->ptr[*ops->id] = NULL;
@@ -182,7 +182,8 @@ static void ops_free_list(const struct pernet_operations *ops,
 			  struct list_head *net_exit_list)
 {
 	struct net *net;
-	if (ops->size && ops->id) {
+
+	if (ops->id) {
 		list_for_each_entry(net, net_exit_list, exit_list)
 			kfree(net_generic(net, *ops->id));
 	}
@@ -1244,7 +1245,7 @@ static int __register_pernet_operations(struct list_head *list,
 	LIST_HEAD(net_exit_list);
 
 	list_add_tail(&ops->list, list);
-	if (ops->init || (ops->id && ops->size)) {
+	if (ops->init || ops->id) {
 		/* We held write locked pernet_ops_rwsem, and parallel
 		 * setup_net() and cleanup_net() are not possible.
 		 */
@@ -1310,6 +1311,9 @@ static int register_pernet_operations(struct list_head *list,
 {
 	int error;
 
+	if (WARN_ON((ops->id && !ops->size) || (!ops->id && ops->size)))
+		return -EINVAL;
+
 	if (ops->id) {
 		error = ida_alloc_min(&net_generic_ids, MIN_PERNET_OPS_ID,
 				GFP_KERNEL);
-- 
2.30.2

Re: [PATCH v1 net-next 2/6] net: Don't register pernet_operations if only one of id or size is specified.

[Jakub Kicinski]

On Mon, 29 Jul 2024 14:07:57 -0700 Kuniyuki Iwashima wrote:
> +	if (WARN_ON((ops->id && !ops->size) || (!ops->id && ops->size)))

I'd write as:

	if (WARN_ON(!!ops->id != !!ops->size))

or

	if (WARN_ON(!!ops->id ^ !!ops->size))

but not 100% sure if it's idiomatic or just my preference..

Re: [PATCH v1 net-next 2/6] net: Don't register pernet_operations if only one of id or size is specified.

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 30 Jul 2024 18:54:12 -0700
> On Mon, 29 Jul 2024 14:07:57 -0700 Kuniyuki Iwashima wrote:
> > +	if (WARN_ON((ops->id && !ops->size) || (!ops->id && ops->size)))
> 
> I'd write as:
> 
> 	if (WARN_ON(!!ops->id != !!ops->size))
> 
> or
> 
> 	if (WARN_ON(!!ops->id ^ !!ops->size))
> 
> but not 100% sure if it's idiomatic or just my preference..

Actually I wrote the latter first so will use it in v2.
Just not confident about which was easier to read.

Thanks!

[PATCH v1 net-next 3/6] net: Initialise net->passive once in preinit_net().

[Kuniyuki Iwashima]

When initialising the root netns, we set net->passive in setup_net().

However, we do it twice for non-root netns in copy_net_ns() and
setup_net().

This is because we could bypass setup_net() in copy_net_ns() if
down_read_killable() fails.

preinit_net() is a better place to put such an operation.

Let's initialise net->passive in preinit_net().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/net_namespace.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 1d164ae097a5..e4e99e7ba9f8 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -312,6 +312,7 @@ EXPORT_SYMBOL_GPL(get_net_ns_by_id);
 /* init code that must occur even if setup_net() is not called. */
 static __net_init void preinit_net(struct net *net)
 {
+	refcount_set(&net->passive, 1);
 	ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
 }
 
@@ -329,7 +330,6 @@ static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
 	refcount_set(&net->ns.count, 1);
 	ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
 
-	refcount_set(&net->passive, 1);
 	get_random_bytes(&net->hash_mix, sizeof(u32));
 	preempt_disable();
 	net->net_cookie = gen_cookie_next(&net_cookie);
@@ -498,7 +498,6 @@ struct net *copy_net_ns(unsigned long flags,
 	}
 
 	preinit_net(net);
-	refcount_set(&net->passive, 1);
 	net->ucounts = ucounts;
 	get_user_ns(user_ns);
 
-- 
2.30.2

[PATCH v1 net-next 4/6] net: Call preinit_net() without pernet_ops_rwsem.

[Kuniyuki Iwashima]

When initialising the root netns, we call preinit_net() under
pernet_ops_rwsem.

However, the operations in preinit_net() do not require pernet_ops_rwsem.

Also, we don't hold it for preinit_net() when initialising non-root netns.

To be consistent, let's call preinit_net() without pernet_ops_rwsem in
net_ns_init().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/net_namespace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index e4e99e7ba9f8..f1b6cea7a9b6 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -1199,8 +1199,9 @@ void __init net_ns_init(void)
 #ifdef CONFIG_KEYS
 	init_net.key_domain = &init_net_key_domain;
 #endif
-	down_write(&pernet_ops_rwsem);
 	preinit_net(&init_net);
+
+	down_write(&pernet_ops_rwsem);
 	if (setup_net(&init_net, &init_user_ns))
 		panic("Could not setup the initial network namespace");
 
-- 
2.30.2

[PATCH v1 net-next 5/6] net: Slim down setup_net().

[Kuniyuki Iwashima]

Most initialisations in setup_net() do not require pernet_ops_rwsem
and can be moved to preinit_net().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/net_namespace.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index f1b6cea7a9b6..7498f2cebbfe 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -310,16 +310,26 @@ struct net *get_net_ns_by_id(const struct net *net, int id)
 EXPORT_SYMBOL_GPL(get_net_ns_by_id);
 
 /* init code that must occur even if setup_net() is not called. */
-static __net_init void preinit_net(struct net *net)
+static __net_init void preinit_net(struct net *net, struct user_namespace *user_ns)
 {
 	refcount_set(&net->passive, 1);
+	refcount_set(&net->ns.count, 1);
+	ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
 	ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
+
+	get_random_bytes(&net->hash_mix, sizeof(u32));
+	net->dev_base_seq = 1;
+	net->user_ns = user_ns;
+
+	idr_init(&net->netns_ids);
+	spin_lock_init(&net->nsid_lock);
+	mutex_init(&net->ipv4.ra_mutex);
 }
 
 /*
  * setup_net runs the initializers for the network namespace object.
  */
-static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
+static __net_init int setup_net(struct net *net)
 {
 	/* Must be called with pernet_ops_rwsem held */
 	const struct pernet_operations *ops, *saved_ops;
@@ -327,18 +337,9 @@ static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
 	LIST_HEAD(dev_kill_list);
 	int error = 0;
 
-	refcount_set(&net->ns.count, 1);
-	ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
-
-	get_random_bytes(&net->hash_mix, sizeof(u32));
 	preempt_disable();
 	net->net_cookie = gen_cookie_next(&net_cookie);
 	preempt_enable();
-	net->dev_base_seq = 1;
-	net->user_ns = user_ns;
-	idr_init(&net->netns_ids);
-	spin_lock_init(&net->nsid_lock);
-	mutex_init(&net->ipv4.ra_mutex);
 
 	list_for_each_entry(ops, &pernet_list, list) {
 		error = ops_init(ops, net);
@@ -497,7 +498,7 @@ struct net *copy_net_ns(unsigned long flags,
 		goto dec_ucounts;
 	}
 
-	preinit_net(net);
+	preinit_net(net, user_ns);
 	net->ucounts = ucounts;
 	get_user_ns(user_ns);
 
@@ -505,7 +506,7 @@ struct net *copy_net_ns(unsigned long flags,
 	if (rv < 0)
 		goto put_userns;
 
-	rv = setup_net(net, user_ns);
+	rv = setup_net(net);
 
 	up_read(&pernet_ops_rwsem);
 
@@ -1199,10 +1200,10 @@ void __init net_ns_init(void)
 #ifdef CONFIG_KEYS
 	init_net.key_domain = &init_net_key_domain;
 #endif
-	preinit_net(&init_net);
+	preinit_net(&init_net, &init_user_ns);
 
 	down_write(&pernet_ops_rwsem);
-	if (setup_net(&init_net, &init_user_ns))
+	if (setup_net(&init_net))
 		panic("Could not setup the initial network namespace");
 
 	init_net_initialized = true;
-- 
2.30.2

[PATCH v1 net-next 6/6] net: Initialise net.core sysctl defaults in preinit_net().

[Kuniyuki Iwashima]

Commit 7c3f1875c66f ("net: move somaxconn init from sysctl code")
introduced net_defaults_ops to make sure that net.core sysctl knobs
are always initialised even if CONFIG_SYSCTL is disabled.

Such operations better fit preinit_net() added for a similar purpose
by commit 6e77a5a4af05 ("net: initialize net->notrefcnt_tracker earlier").

Let's initialise the sysctl defaults in preinit_net().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/net_namespace.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 7498f2cebbfe..a96a3be77f12 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -309,6 +309,16 @@ struct net *get_net_ns_by_id(const struct net *net, int id)
 }
 EXPORT_SYMBOL_GPL(get_net_ns_by_id);
 
+static __net_init void preinit_net_sysctl(struct net *net)
+{
+	net->core.sysctl_somaxconn = SOMAXCONN;
+	/* Limits per socket sk_omem_alloc usage.
+	 * TCP zerocopy regular usage needs 128 KB.
+	 */
+	net->core.sysctl_optmem_max = 128 * 1024;
+	net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
+}
+
 /* init code that must occur even if setup_net() is not called. */
 static __net_init void preinit_net(struct net *net, struct user_namespace *user_ns)
 {
@@ -324,6 +334,7 @@ static __net_init void preinit_net(struct net *net, struct user_namespace *user_
 	idr_init(&net->netns_ids);
 	spin_lock_init(&net->nsid_lock);
 	mutex_init(&net->ipv4.ra_mutex);
+	preinit_net_sysctl(net);
 }
 
 /*
@@ -384,32 +395,6 @@ static __net_init int setup_net(struct net *net)
 	goto out;
 }
 
-static int __net_init net_defaults_init_net(struct net *net)
-{
-	net->core.sysctl_somaxconn = SOMAXCONN;
-	/* Limits per socket sk_omem_alloc usage.
-	 * TCP zerocopy regular usage needs 128 KB.
-	 */
-	net->core.sysctl_optmem_max = 128 * 1024;
-	net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
-
-	return 0;
-}
-
-static struct pernet_operations net_defaults_ops = {
-	.init = net_defaults_init_net,
-};
-
-static __init int net_defaults_init(void)
-{
-	if (register_pernet_subsys(&net_defaults_ops))
-		panic("Cannot initialize net default settings");
-
-	return 0;
-}
-
-core_initcall(net_defaults_init);
-
 #ifdef CONFIG_NET_NS
 static struct ucounts *inc_net_namespaces(struct user_namespace *ns)
 {
-- 
2.30.2

Re: [PATCH v1 net-next 0/6] net: Random cleanup for netns initialisation.

[Jakub Kicinski]

On Mon, 29 Jul 2024 14:07:55 -0700 Kuniyuki Iwashima wrote:
> do not reqruire pernet_ops_rwsem.

require

Re: [PATCH v1 net-next 0/6] net: Random cleanup for netns initialisation.

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 30 Jul 2024 18:52:34 -0700
> On Mon, 29 Jul 2024 14:07:55 -0700 Kuniyuki Iwashima wrote:
> > do not reqruire pernet_ops_rwsem.
> 
> require

Aha, I should've used Grammarly even for short cover letter :)

will fix it.

Thanks!