[PATCH net-next] net: sysctl: cleanup net_sysctl_init()

[PATCH net-next] net: sysctl: cleanup net_sysctl_init()

[Alexander Lobakin]

'net_header' is not used outside of this function, so can be moved
from BSS onto the stack.
Declarations of one-element arrays are discouraged, and there's no
need to store 'empty' in BSS. Simply allocate it from heap at init.

Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 net/sysctl_net.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index d14dab8b6774..4cf81800a907 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -92,27 +92,34 @@ static struct pernet_operations sysctl_pernet_ops = {
 	.exit = sysctl_net_exit,
 };
 
-static struct ctl_table_header *net_header;
 __init int net_sysctl_init(void)
 {
-	static struct ctl_table empty[1];
+	struct ctl_table_header *net_header;
+	struct ctl_table *empty;
 	int ret = -ENOMEM;
+
 	/* Avoid limitations in the sysctl implementation by
 	 * registering "/proc/sys/net" as an empty directory not in a
 	 * network namespace.
 	 */
+
+	empty = kzalloc(sizeof(*empty), GFP_KERNEL);
+	if (!empty)
+		return ret;
+
 	net_header = register_sysctl("net", empty);
 	if (!net_header)
-		goto out;
+		goto err_free;
+
 	ret = register_pernet_subsys(&sysctl_pernet_ops);
-	if (ret)
-		goto out1;
-out:
-	return ret;
-out1:
+	if (!ret)
+		return 0;
+
 	unregister_sysctl_table(net_header);
-	net_header = NULL;
-	goto out;
+err_free:
+	kfree(empty);
+
+	return ret;
 }
 
 struct ctl_table_header *register_net_sysctl(struct net *net,
-- 
2.30.0

Re: [PATCH net-next] net: sysctl: cleanup net_sysctl_init()

[Jakub Kicinski]

On Wed, 06 Jan 2021 20:40:28 +0000 Alexander Lobakin wrote:
> 'net_header' is not used outside of this function, so can be moved
> from BSS onto the stack.
> Declarations of one-element arrays are discouraged, and there's no
> need to store 'empty' in BSS. Simply allocate it from heap at init.

Are you sure? It's passed as an argument to register_sysctl()
so it may well need to be valid for the lifetime of net_header.

Re: [PATCH net-next] net: sysctl: cleanup net_sysctl_init()

[Alexander Lobakin]

From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 6 Jan 2021 16:30:56 -0800

> On Wed, 06 Jan 2021 20:40:28 +0000 Alexander Lobakin wrote:
>> 'net_header' is not used outside of this function, so can be moved
>> from BSS onto the stack.
>> Declarations of one-element arrays are discouraged, and there's no
>> need to store 'empty' in BSS. Simply allocate it from heap at init.
>
> Are you sure? It's passed as an argument to register_sysctl()
> so it may well need to be valid for the lifetime of net_header.

I just moved it from BSS to the heap and allocate it using kzalloc(),
it's still valid through the lifetime of the kernel.

Al

Re: [PATCH net-next] net: sysctl: cleanup net_sysctl_init()

[Jakub Kicinski]

On Thu, 07 Jan 2021 09:13:40 +0000 Alexander Lobakin wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Wed, 6 Jan 2021 16:30:56 -0800
> 
> > On Wed, 06 Jan 2021 20:40:28 +0000 Alexander Lobakin wrote:  
> >> 'net_header' is not used outside of this function, so can be moved
> >> from BSS onto the stack.
> >> Declarations of one-element arrays are discouraged, and there's no
> >> need to store 'empty' in BSS. Simply allocate it from heap at init.  
> >
> > Are you sure? It's passed as an argument to register_sysctl()
> > so it may well need to be valid for the lifetime of net_header.  
> 
> I just moved it from BSS to the heap and allocate it using kzalloc(),
> it's still valid through the lifetime of the kernel.

I see it now, please don't break the normal flow of error handling.

What's the point of moving objects allocated in __init from BSS to 
the heap? If anything I'd think it'll take up more space when allocated
in the heap because of the metadata that needs to be tracked for
dynamic allocations.

The move of net_header makes sense AFAICT, but we may have to annotate
it somehow so kmemleak doesn't complain.