| Message ID | 20200529074108.16928-2-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sysctl: spring cleaning | expand |
On Fri, 29 May 2020, Luis Chamberlain <mcgrof@kernel.org> wrote: > Often enough all we need to do is create a subdirectory so that > we can stuff sysctls underneath it. However, *if* that directory > was already created early on the boot sequence we really have no > need to use the full boiler plate code for it, we can just use > local variables to help us guide sysctl to place the new leaf files. I find it hard to figure out the lifetime requirements for the tables passed in; when it's okay to use local variables and when you need longer lifetimes. It's not documented, everyone appears to be using static tables for this. It's far from obvious. BR, Jani.
On Fri, May 29, 2020 at 11:13:21AM +0300, Jani Nikula wrote: > On Fri, 29 May 2020, Luis Chamberlain <mcgrof@kernel.org> wrote: > > Often enough all we need to do is create a subdirectory so that > > we can stuff sysctls underneath it. However, *if* that directory > > was already created early on the boot sequence we really have no > > need to use the full boiler plate code for it, we can just use > > local variables to help us guide sysctl to place the new leaf files. > > I find it hard to figure out the lifetime requirements for the tables > passed in; when it's okay to use local variables and when you need > longer lifetimes. It's not documented, everyone appears to be using > static tables for this. It's far from obvious. I agree 2000% that it is not obvious. What made me consider it was that I *knew* that the base directory would already exist, so it wouldn't make sense for the code to rely on earlier parts of a table if part of the hierarchy already existed. In fact, a *huge* part of the due dilligence on this and futre series on this cleanup will be to be 100% sure that the base path is already created. And so this use is obviously dangerous, you just *need* to know that the base path is created before. Non-posted changes also deal with link order to help address this in other places, given that link order controls how *initcalls() (early_initcall(), late_initcall(), etc) are ordered if you have multiple of these. I had a link order series long ago which augmented our ability to make things clearer at a link order. Eventually I believe this will become more important, specially as we end up wanting to async more code. For now, we can only rely on manual code inspection for ensuring proper ordering. Part of the implicit aspects of this cleanup is to slowly make these things clearer for each base path. So... the "fs" base path will actually end up being created in fs/sysctl.c after we are *fully* done with the fs sysctl cleanups. Luis
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index ddaa06ddd852..58bc978d4f03 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -205,6 +205,9 @@ void unregister_sysctl_table(struct ctl_table_header * table); extern int sysctl_init(void); extern void register_sysctl_init(const char *path, struct ctl_table *table, const char *table_name); +extern struct ctl_table_header *register_sysctl_subdir(const char *base, + const char *subdir, + struct ctl_table *table); void do_sysctl_args(void); extern int pwrsw_enabled; @@ -223,6 +226,14 @@ static inline struct ctl_table_header *register_sysctl_table(struct ctl_table * return NULL; } +static +inline struct ctl_table_header *register_sysctl_subdir(const char *base, + const char *subdir, + struct ctl_table *table) +{ + return NULL; +} + static inline struct ctl_table_header *register_sysctl_paths( const struct ctl_path *path, struct ctl_table *table) { diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 008ac0576ae5..04ff032f2863 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -3195,6 +3195,43 @@ void __init register_sysctl_init(const char *path, struct ctl_table *table, } kmemleak_not_leak(hdr); } + +struct ctl_table_header *register_sysctl_subdir(const char *base, + const char *subdir, + struct ctl_table *table) +{ + struct ctl_table_header *hdr = NULL; + struct ctl_table subdir_table[] = { + { + .procname = subdir, + .mode = 0555, + .child = table, + }, + { } + }; + struct ctl_table base_table[] = { + { + .procname = base, + .mode = 0555, + .child = subdir_table, + }, + { } + }; + + if (!table->procname) + goto out; + + hdr = register_sysctl_table(base_table); + if (unlikely(!hdr)) { + pr_err("failed when creating subdirectory sysctl %s/%s/%s\n", + base, subdir, table->procname); + goto out; + } + kmemleak_not_leak(hdr); +out: + return hdr; +} +EXPORT_SYMBOL_GPL(register_sysctl_subdir); #endif /* CONFIG_SYSCTL */ /* * No sense putting this after each symbol definition, twice,
Often enough all we need to do is create a subdirectory so that we can stuff sysctls underneath it. However, *if* that directory was already created early on the boot sequence we really have no need to use the full boiler plate code for it, we can just use local variables to help us guide sysctl to place the new leaf files. So use a helper to do precisely this. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- include/linux/sysctl.h | 11 +++++++++++ kernel/sysctl.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)