Me and Xiaoming are working on some kernel/sysctl.c spring cleaning.
During a recent linux-next merge conflict it became clear that
the kitchen sink on kernel/sysctl.c creates too many conflicts,
and so we need to do away with stuffing everyone's knobs on this
one file.
This is part of that work. This is not expected to get merged yet, but
since our delta is pretty considerable at this point, we need to piece
meal this and collect reviews for what we have so far. This follows up
on some of his recent work.
This series focuses on a new helper to deal with subdirectories and
empty subdirectories. The terminology that we will embrace will be
that things like "fs", "kernel", "debug" are based directories, and
directories underneath this are subdirectories.
In this case, the cleanup ends up also trimming the amount of
code we have for sysctls.
If this seems reasonable we'll kdocify this a bit too.
This code has been boot tested without issues, and I'm letting 0day do
its thing to test against many kconfig builds. If you however spot
any issues please let us know.
Luis Chamberlain (9):
sysctl: add new register_sysctl_subdir() helper
cdrom: use new sysctl subdir helper register_sysctl_subdir()
hpet: use new sysctl subdir helper register_sysctl_subdir()
i915: use new sysctl subdir helper register_sysctl_subdir()
macintosh/mac_hid.c: use new sysctl subdir helper
register_sysctl_subdir()
ocfs2: use new sysctl subdir helper register_sysctl_subdir()
test_sysctl: use new sysctl subdir helper register_sysctl_subdir()
sysctl: add helper to register empty subdir
fs: move binfmt_misc sysctl to its own file
Xiaoming Ni (4):
inotify: simplify sysctl declaration with register_sysctl_subdir()
firmware_loader: simplify sysctl declaration with
register_sysctl_subdir()
eventpoll: simplify sysctl declaration with register_sysctl_subdir()
random: simplify sysctl declaration with register_sysctl_subdir()
drivers/base/firmware_loader/fallback.c | 4 +
drivers/base/firmware_loader/fallback.h | 11 +++
drivers/base/firmware_loader/fallback_table.c | 22 ++++-
drivers/cdrom/cdrom.c | 23 +----
drivers/char/hpet.c | 22 +----
drivers/char/random.c | 14 +++-
drivers/gpu/drm/i915/i915_perf.c | 22 +----
drivers/macintosh/mac_hid.c | 25 +-----
fs/binfmt_misc.c | 1 +
fs/eventpoll.c | 10 ++-
fs/notify/inotify/inotify_user.c | 11 ++-
fs/ocfs2/stackglue.c | 27 +-----
include/linux/inotify.h | 3 -
include/linux/poll.h | 2 -
include/linux/sysctl.h | 21 ++++-
kernel/sysctl.c | 84 +++++++++++--------
lib/test_sysctl.c | 23 +----
17 files changed, 144 insertions(+), 181 deletions(-)
--
2.26.2
The way to create a subdirectory from the base set of directories
is a bit obscure, so provide a helper which makes this clear, and
also helps remove boiler plate code required to do this work.
Signed-off-by: Luis Chamberlain <[email protected]>
---
include/linux/sysctl.h | 7 +++++++
kernel/sysctl.c | 16 +++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 33a471b56345..89c92390e6de 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -208,6 +208,8 @@ extern void register_sysctl_init(const char *path, struct ctl_table *table,
extern struct ctl_table_header *register_sysctl_subdir(const char *base,
const char *subdir,
struct ctl_table *table);
+extern void register_sysctl_empty_subdir(const char *base, const char *subdir);
+
void do_sysctl_args(void);
extern int pwrsw_enabled;
@@ -231,6 +233,11 @@ inline struct ctl_table_header *register_sysctl_subdir(const char *base,
return NULL;
}
+static inline void register_sysctl_empty_subdir(const char *base,
+ const char *subdir)
+{
+}
+
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 f9a35325d5d5..460532cd5ac8 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3188,13 +3188,17 @@ struct ctl_table_header *register_sysctl_subdir(const char *base,
{ }
};
- if (!table->procname)
+ if (table != sysctl_mount_point && !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);
+ if (table != sysctl_mount_point)
+ pr_err("failed when creating subdirectory sysctl %s/%s/%s\n",
+ base, subdir, table->procname);
+ else
+ pr_err("failed when creating empty subddirectory %s/%s\n",
+ base, subdir);
goto out;
}
kmemleak_not_leak(hdr);
@@ -3202,6 +3206,12 @@ struct ctl_table_header *register_sysctl_subdir(const char *base,
return hdr;
}
EXPORT_SYMBOL_GPL(register_sysctl_subdir);
+
+void register_sysctl_empty_subdir(const char *base,
+ const char *subdir)
+{
+ register_sysctl_subdir(base, subdir, sysctl_mount_point);
+}
#endif /* CONFIG_SYSCTL */
/*
* No sense putting this after each symbol definition, twice,
--
2.26.2
From: Xiaoming Ni <[email protected]>
Move the firmware config sysctl table to fallback_table.c and use the
new register_sysctl_subdir() helper. This removes the clutter from
kernel/sysctl.c.
Signed-off-by: Xiaoming Ni <[email protected]>
Signed-off-by: Luis Chamberlain <[email protected]>
---
drivers/base/firmware_loader/fallback.c | 4 ++++
drivers/base/firmware_loader/fallback.h | 11 ++++++++++
drivers/base/firmware_loader/fallback_table.c | 22 +++++++++++++++++--
include/linux/sysctl.h | 1 -
kernel/sysctl.c | 7 ------
5 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index d9ac7296205e..8190653ae9a3 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -200,12 +200,16 @@ static struct class firmware_class = {
int register_sysfs_loader(void)
{
+ int ret = register_firmware_config_sysctl();
+ if (ret != 0)
+ return ret;
return class_register(&firmware_class);
}
void unregister_sysfs_loader(void)
{
class_unregister(&firmware_class);
+ unregister_firmware_config_sysctl();
}
static ssize_t firmware_loading_show(struct device *dev,
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index 06f4577733a8..7d2cb5f6ceb8 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -42,6 +42,17 @@ void fw_fallback_set_default_timeout(void);
int register_sysfs_loader(void);
void unregister_sysfs_loader(void);
+#ifdef CONFIG_SYSCTL
+extern int register_firmware_config_sysctl(void);
+extern void unregister_firmware_config_sysctl(void);
+#else
+static inline int register_firmware_config_sysctl(void)
+{
+ return 0;
+}
+static inline void unregister_firmware_config_sysctl(void) { }
+#endif /* CONFIG_SYSCTL */
+
#else /* CONFIG_FW_LOADER_USER_HELPER */
static inline int firmware_fallback_sysfs(struct firmware *fw, const char *name,
struct device *device,
diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c
index 46a731dede6f..4234aa5ee5df 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -24,7 +24,7 @@ struct firmware_fallback_config fw_fallback_config = {
EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE);
#ifdef CONFIG_SYSCTL
-struct ctl_table firmware_config_table[] = {
+static struct ctl_table firmware_config_table[] = {
{
.procname = "force_sysfs_fallback",
.data = &fw_fallback_config.force_sysfs_fallback,
@@ -45,4 +45,22 @@ struct ctl_table firmware_config_table[] = {
},
{ }
};
-#endif
+
+static struct ctl_table_header *hdr;
+int register_firmware_config_sysctl(void)
+{
+ if (hdr)
+ return -EEXIST;
+ hdr = register_sysctl_subdir("kernel", "firmware_config",
+ firmware_config_table);
+ if (!hdr)
+ return -ENOMEM;
+ return 0;
+}
+
+void unregister_firmware_config_sysctl(void)
+{
+ if (hdr)
+ unregister_sysctl_table(hdr);
+}
+#endif /* CONFIG_SYSCTL */
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 58bc978d4f03..aa01f54d0442 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -217,7 +217,6 @@ extern int no_unaligned_warning;
extern struct ctl_table sysctl_mount_point[];
extern struct ctl_table random_table[];
-extern struct ctl_table firmware_config_table[];
extern struct ctl_table epoll_table[];
#else /* CONFIG_SYSCTL */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 30c2d521502a..e007375c8a11 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2088,13 +2088,6 @@ static struct ctl_table kern_table[] = {
.mode = 0555,
.child = usermodehelper_table,
},
-#ifdef CONFIG_FW_LOADER_USER_HELPER
- {
- .procname = "firmware_config",
- .mode = 0555,
- .child = firmware_config_table,
- },
-#endif
{
.procname = "overflowuid",
.data = &overflowuid,
--
2.26.2
This simplifies the code considerably. The following coccinelle
SmPL grammar rule was used to transform this code.
// pycocci sysctl-subdir.cocci drivers/gpu/drm/i915/i915_perf.c
@c1@
expression E1;
identifier subdir, sysctls;
@@
static struct ctl_table subdir[] = {
{
.procname = E1,
.maxlen = 0,
.mode = 0555,
.child = sysctls,
},
{ }
};
@c2@
identifier c1.subdir;
expression E2;
identifier base;
@@
static struct ctl_table base[] = {
{
.procname = E2,
.maxlen = 0,
.mode = 0555,
.child = subdir,
},
{ }
};
@c3@
identifier c2.base;
identifier header;
@@
header = register_sysctl_table(base);
@r1 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.subdir, c1.sysctls;
@@
-static struct ctl_table subdir[] = {
- {
- .procname = E1,
- .maxlen = 0,
- .mode = 0555,
- .child = sysctls,
- },
- { }
-};
@r2 depends on c1 && c2 && c3@
identifier c1.subdir;
expression c2.E2;
identifier c2.base;
@@
-static struct ctl_table base[] = {
- {
- .procname = E2,
- .maxlen = 0,
- .mode = 0555,
- .child = subdir,
- },
- { }
-};
@r3 depends on c1 && c2 && c3@
expression c1.E1;
identifier c1.sysctls;
expression c2.E2;
identifier c2.base;
identifier c3.header;
@@
header =
-register_sysctl_table(base);
+register_sysctl_subdir(E2, E1, sysctls);
Generated-by: Coccinelle SmPL
Signed-off-by: Luis Chamberlain <[email protected]>
---
drivers/gpu/drm/i915/i915_perf.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 665bb076e84d..52509b573794 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -4203,26 +4203,6 @@ static struct ctl_table oa_table[] = {
{}
};
-static struct ctl_table i915_root[] = {
- {
- .procname = "i915",
- .maxlen = 0,
- .mode = 0555,
- .child = oa_table,
- },
- {}
-};
-
-static struct ctl_table dev_root[] = {
- {
- .procname = "dev",
- .maxlen = 0,
- .mode = 0555,
- .child = i915_root,
- },
- {}
-};
-
/**
* i915_perf_init - initialize i915-perf state on module bind
* @i915: i915 device instance
@@ -4383,7 +4363,7 @@ static int destroy_config(int id, void *p, void *data)
void i915_perf_sysctl_register(void)
{
- sysctl_header = register_sysctl_table(dev_root);
+ sysctl_header = register_sysctl_subdir("dev", "i915", oa_table);
}
void i915_perf_sysctl_unregister(void)
--
2.26.2
On Fri, May 29, 2020 at 07:41:07AM +0000, Luis Chamberlain wrote:
> The way to create a subdirectory from the base set of directories
> is a bit obscure, so provide a helper which makes this clear, and
> also helps remove boiler plate code required to do this work.
>
> Signed-off-by: Luis Chamberlain <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Fri, May 29, 2020 at 07:41:04AM +0000, Luis Chamberlain wrote:
> From: Xiaoming Ni <[email protected]>
>
> Move the firmware config sysctl table to fallback_table.c and use the
> new register_sysctl_subdir() helper. This removes the clutter from
> kernel/sysctl.c.
>
> Signed-off-by: Xiaoming Ni <[email protected]>
> Signed-off-by: Luis Chamberlain <[email protected]>
> ---
> drivers/base/firmware_loader/fallback.c | 4 ++++
> drivers/base/firmware_loader/fallback.h | 11 ++++++++++
> drivers/base/firmware_loader/fallback_table.c | 22 +++++++++++++++++--
> include/linux/sysctl.h | 1 -
> kernel/sysctl.c | 7 ------
> 5 files changed, 35 insertions(+), 10 deletions(-)
So it now takes more lines than the old stuff? :(
>
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index d9ac7296205e..8190653ae9a3 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -200,12 +200,16 @@ static struct class firmware_class = {
>
> int register_sysfs_loader(void)
> {
> + int ret = register_firmware_config_sysctl();
> + if (ret != 0)
> + return ret;
checkpatch :(
> return class_register(&firmware_class);
And if that fails?
> }
>
> void unregister_sysfs_loader(void)
> {
> class_unregister(&firmware_class);
> + unregister_firmware_config_sysctl();
> }
>
> static ssize_t firmware_loading_show(struct device *dev,
> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
> index 06f4577733a8..7d2cb5f6ceb8 100644
> --- a/drivers/base/firmware_loader/fallback.h
> +++ b/drivers/base/firmware_loader/fallback.h
> @@ -42,6 +42,17 @@ void fw_fallback_set_default_timeout(void);
>
> int register_sysfs_loader(void);
> void unregister_sysfs_loader(void);
> +#ifdef CONFIG_SYSCTL
> +extern int register_firmware_config_sysctl(void);
> +extern void unregister_firmware_config_sysctl(void);
> +#else
> +static inline int register_firmware_config_sysctl(void)
> +{
> + return 0;
> +}
> +static inline void unregister_firmware_config_sysctl(void) { }
> +#endif /* CONFIG_SYSCTL */
> +
> #else /* CONFIG_FW_LOADER_USER_HELPER */
> static inline int firmware_fallback_sysfs(struct firmware *fw, const char *name,
> struct device *device,
> diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c
> index 46a731dede6f..4234aa5ee5df 100644
> --- a/drivers/base/firmware_loader/fallback_table.c
> +++ b/drivers/base/firmware_loader/fallback_table.c
> @@ -24,7 +24,7 @@ struct firmware_fallback_config fw_fallback_config = {
> EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE);
>
> #ifdef CONFIG_SYSCTL
> -struct ctl_table firmware_config_table[] = {
> +static struct ctl_table firmware_config_table[] = {
> {
> .procname = "force_sysfs_fallback",
> .data = &fw_fallback_config.force_sysfs_fallback,
> @@ -45,4 +45,22 @@ struct ctl_table firmware_config_table[] = {
> },
> { }
> };
> -#endif
> +
> +static struct ctl_table_header *hdr;
> +int register_firmware_config_sysctl(void)
> +{
> + if (hdr)
> + return -EEXIST;
How can hdr be set?
> + hdr = register_sysctl_subdir("kernel", "firmware_config",
> + firmware_config_table);
> + if (!hdr)
> + return -ENOMEM;
> + return 0;
> +}
> +
> +void unregister_firmware_config_sysctl(void)
> +{
> + if (hdr)
> + unregister_sysctl_table(hdr);
Why can't unregister_sysctl_table() take a null pointer value?
And what sets 'hdr' (worst name for a static variable) to NULL so that
it knows not to be unregistered again as it looks like
register_firmware_config_sysctl() could be called multiple times.
thanks,
greg k-h
On 2020/5/29 18:26, Greg KH wrote:
> On Fri, May 29, 2020 at 07:41:04AM +0000, Luis Chamberlain wrote:
>> From: Xiaoming Ni <[email protected]>
>>
>> Move the firmware config sysctl table to fallback_table.c and use the
>> new register_sysctl_subdir() helper. This removes the clutter from
>> kernel/sysctl.c.
>>
>> Signed-off-by: Xiaoming Ni <[email protected]>
>> Signed-off-by: Luis Chamberlain <[email protected]>
>> ---
>> drivers/base/firmware_loader/fallback.c | 4 ++++
>> drivers/base/firmware_loader/fallback.h | 11 ++++++++++
>> drivers/base/firmware_loader/fallback_table.c | 22 +++++++++++++++++--
>> include/linux/sysctl.h | 1 -
>> kernel/sysctl.c | 7 ------
>> 5 files changed, 35 insertions(+), 10 deletions(-)
>
> So it now takes more lines than the old stuff? :(
>
CONFIG_FW_LOADER = m
Before cleaning, no matter whether ko is loaded or not, the sysctl
interface will be created, but now we need to add register and
unregister interfaces, so the number of lines of code has increased
>>
>> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
>> index d9ac7296205e..8190653ae9a3 100644
>> --- a/drivers/base/firmware_loader/fallback.c
>> +++ b/drivers/base/firmware_loader/fallback.c
>> @@ -200,12 +200,16 @@ static struct class firmware_class = {
>>
>> int register_sysfs_loader(void)
>> {
>> + int ret = register_firmware_config_sysctl();
>> + if (ret != 0)
>> + return ret;
>
> checkpatch :(
This is my fault, thanks for your guidance
>
>> return class_register(&firmware_class);
>
> And if that fails?
>
Yes, it is better to call register_firmware_config_sysctl() after
class_register().
thanks for your guidance.
>> }
>>
>> void unregister_sysfs_loader(void)
>> {
>> class_unregister(&firmware_class);
>> + unregister_firmware_config_sysctl();
>> }
>>
>> static ssize_t firmware_loading_show(struct device *dev,
>> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
>> index 06f4577733a8..7d2cb5f6ceb8 100644
>> --- a/drivers/base/firmware_loader/fallback.h
>> +++ b/drivers/base/firmware_loader/fallback.h
>> @@ -42,6 +42,17 @@ void fw_fallback_set_default_timeout(void);
>>
>> int register_sysfs_loader(void);
>> void unregister_sysfs_loader(void);
>> +#ifdef CONFIG_SYSCTL
>> +extern int register_firmware_config_sysctl(void);
>> +extern void unregister_firmware_config_sysctl(void);
>> +#else
>> +static inline int register_firmware_config_sysctl(void)
>> +{
>> + return 0;
>> +}
>> +static inline void unregister_firmware_config_sysctl(void) { }
>> +#endif /* CONFIG_SYSCTL */
>> +
>> #else /* CONFIG_FW_LOADER_USER_HELPER */
>> static inline int firmware_fallback_sysfs(struct firmware *fw, const char *name,
>> struct device *device,
>> diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c
>> index 46a731dede6f..4234aa5ee5df 100644
>> --- a/drivers/base/firmware_loader/fallback_table.c
>> +++ b/drivers/base/firmware_loader/fallback_table.c
>> @@ -24,7 +24,7 @@ struct firmware_fallback_config fw_fallback_config = {
>> EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE);
>>
>> #ifdef CONFIG_SYSCTL
>> -struct ctl_table firmware_config_table[] = {
>> +static struct ctl_table firmware_config_table[] = {
>> {
>> .procname = "force_sysfs_fallback",
>> .data = &fw_fallback_config.force_sysfs_fallback,
>> @@ -45,4 +45,22 @@ struct ctl_table firmware_config_table[] = {
>> },
>> { }
>> };
>> -#endif
>> +
>> +static struct ctl_table_header *hdr;
>> +int register_firmware_config_sysctl(void)
>> +{
>> + if (hdr)
>> + return -EEXIST;
>
> How can hdr be set?
>
It's my mistake, register_firmware_config_sysctl() is not exported,
there will be no repeated calls.
thanks for your guidance.
>> + hdr = register_sysctl_subdir("kernel", "firmware_config",
>> + firmware_config_table);
>> + if (!hdr)
>> + return -ENOMEM;
>> + return 0;
>> +}
>> +
>> +void unregister_firmware_config_sysctl(void)
>> +{
>> + if (hdr)
>> + unregister_sysctl_table(hdr);
>
> Why can't unregister_sysctl_table() take a null pointer value?
Sorry, I didn't notice that the unregister_sysctl_table() already checks
the input parameters.
thanks for your guidance.
> And what sets 'hdr' (worst name for a static variable) to NULL so that
> it knows not to be unregistered again as it looks like
> register_firmware_config_sysctl() could be called multiple times.
How about renaming hdr to firmware_config_sysct_table_header?
+ if (hdr)
+ return -EEXIST;
After deleting this code in register_firmware_config_sysctl(), and
considering register_firmware_config_sysctl() and
unregister_firmware_config_sysctl() are not exported, whether there is
no need to add "hdr = NULL;" ?
Thanks
Xiaoming Ni
On Fri, May 29, 2020 at 12:26:13PM +0200, Greg KH wrote:
> On Fri, May 29, 2020 at 07:41:04AM +0000, Luis Chamberlain wrote:
> > From: Xiaoming Ni <[email protected]>
> >
> > Move the firmware config sysctl table to fallback_table.c and use the
> > new register_sysctl_subdir() helper. This removes the clutter from
> > kernel/sysctl.c.
> >
> > Signed-off-by: Xiaoming Ni <[email protected]>
> > Signed-off-by: Luis Chamberlain <[email protected]>
> > ---
> > drivers/base/firmware_loader/fallback.c | 4 ++++
> > drivers/base/firmware_loader/fallback.h | 11 ++++++++++
> > drivers/base/firmware_loader/fallback_table.c | 22 +++++++++++++++++--
> > include/linux/sysctl.h | 1 -
> > kernel/sysctl.c | 7 ------
> > 5 files changed, 35 insertions(+), 10 deletions(-)
>
> So it now takes more lines than the old stuff? :(
Pretty much agreed with the other changes, thanks for the review!
But this diff-stat change, indeed, it is unfortunate that we end up
with more code here than before. We'll try to reduce it instead
somehow, however in some cases during this spring-cleaning, since
the goal is to move code from one file to another, it *may* require
more code. So it won't always be negative. But we'll try!
Luis
Luis Chamberlain <[email protected]> writes:
> The way to create a subdirectory from the base set of directories
> is a bit obscure, so provide a helper which makes this clear, and
> also helps remove boiler plate code required to do this work.
I agreee calling:
register_sysctl("fs/binfmt_misc", sysctl_mount_point)
is a bit obscure but if you are going to make a wrapper
please make it the trivial one liner above.
Say something that looks like:
struct sysctl_header *register_sysctl_mount_point(const char *path)
{
return register_sysctl(path, sysctl_mount_point);
}
And yes please talk about a mount point and not an empty dir, as these
are permanently empty directories to serve as mount points. There are
some subtle but important permission checks this allows in the case of
unprivileged mounts.
Further code like this belong in proc_sysctl.c next to all of the code
it is related to so that it is easier to see how to refactor the code if
necessary.
Eric
>
> Signed-off-by: Luis Chamberlain <[email protected]>
> ---
> include/linux/sysctl.h | 7 +++++++
> kernel/sysctl.c | 16 +++++++++++++---
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 33a471b56345..89c92390e6de 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -208,6 +208,8 @@ extern void register_sysctl_init(const char *path, struct ctl_table *table,
> extern struct ctl_table_header *register_sysctl_subdir(const char *base,
> const char *subdir,
> struct ctl_table *table);
> +extern void register_sysctl_empty_subdir(const char *base, const char *subdir);
> +
> void do_sysctl_args(void);
>
> extern int pwrsw_enabled;
> @@ -231,6 +233,11 @@ inline struct ctl_table_header *register_sysctl_subdir(const char *base,
> return NULL;
> }
>
> +static inline void register_sysctl_empty_subdir(const char *base,
> + const char *subdir)
> +{
> +}
> +
> 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 f9a35325d5d5..460532cd5ac8 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -3188,13 +3188,17 @@ struct ctl_table_header *register_sysctl_subdir(const char *base,
> { }
> };
>
> - if (!table->procname)
> + if (table != sysctl_mount_point && !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);
> + if (table != sysctl_mount_point)
> + pr_err("failed when creating subdirectory sysctl %s/%s/%s\n",
> + base, subdir, table->procname);
> + else
> + pr_err("failed when creating empty subddirectory %s/%s\n",
> + base, subdir);
> goto out;
> }
> kmemleak_not_leak(hdr);
> @@ -3202,6 +3206,12 @@ struct ctl_table_header *register_sysctl_subdir(const char *base,
> return hdr;
> }
> EXPORT_SYMBOL_GPL(register_sysctl_subdir);
> +
> +void register_sysctl_empty_subdir(const char *base,
> + const char *subdir)
> +{
> + register_sysctl_subdir(base, subdir, sysctl_mount_point);
> +}
> #endif /* CONFIG_SYSCTL */
> /*
> * No sense putting this after each symbol definition, twice,
On Fri, May 29, 2020 at 08:03:02AM -0500, Eric W. Biederman wrote:
> Luis Chamberlain <[email protected]> writes:
>
> > The way to create a subdirectory from the base set of directories
> > is a bit obscure, so provide a helper which makes this clear, and
> > also helps remove boiler plate code required to do this work.
>
> I agreee calling:
> register_sysctl("fs/binfmt_misc", sysctl_mount_point)
> is a bit obscure but if you are going to make a wrapper
> please make it the trivial one liner above.
>
> Say something that looks like:
> struct sysctl_header *register_sysctl_mount_point(const char *path)
> {
> return register_sysctl(path, sysctl_mount_point);
> }
>
> And yes please talk about a mount point and not an empty dir, as these
> are permanently empty directories to serve as mount points. There are
> some subtle but important permission checks this allows in the case of
> unprivileged mounts.
>
> Further code like this belong in proc_sysctl.c next to all of the code
> it is related to so that it is easier to see how to refactor the code if
> necessary.
Alrighty, it's been a while since this kernel/sysctl.c kitchen sink
cleanup... so it's time to respin this now that the merge window is
open. I already rebased patches, addressed all input and now just
waiting to fix any compilation errors. I'm going to split the patches
up into real small sets so to ensure we just get this through becauase
getting this in otherwise is going to be hard.
I'd appreciate folk's review once the patches start going out. I think
a hard part will be deciding what tree this should got through.
Luis