| Message ID | 20210106204014.34730-1-alobakin@pm.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: sysctl: cleanup net_sysctl_init() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 3 of 3 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 44 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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.
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
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.
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,
'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(-)