| Message ID | 20231204-const-sysctl-v2-13-7a5060b11447@weissschuh.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | sysctl: constify sysctl ctl_tables | expand |
On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > return -EROFS; > > /* Am I creating a permanently empty directory? */ > - if (sysctl_is_perm_empty_ctl_header(header)) { > + if (header->ctl_table == sysctl_mount_point || > + sysctl_is_perm_empty_ctl_header(header)) { > if (!RB_EMPTY_ROOT(&dir->root)) > return -EINVAL; > sysctl_set_perm_empty_ctl_header(dir_h); While you're at it. This just made me cringe, and curious if some other changes could be done to make this obviously clear during patch review that this is safe. Luis
On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > return -EROFS; > > > > /* Am I creating a permanently empty directory? */ > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > + if (header->ctl_table == sysctl_mount_point || > > + sysctl_is_perm_empty_ctl_header(header)) { > > if (!RB_EMPTY_ROOT(&dir->root)) > > return -EINVAL; > > sysctl_set_perm_empty_ctl_header(dir_h); > > While you're at it. This hunk is completely gone in v3/the code that you merged. > This just made me cringe, and curious if some other changes > could be done to make this obviously clear during patch review > that this is safe. Which kind of unsafety do you envision here?
On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > return -EROFS; > > > > > > /* Am I creating a permanently empty directory? */ > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > + if (header->ctl_table == sysctl_mount_point || > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > return -EINVAL; > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > While you're at it. > > This hunk is completely gone in v3/the code that you merged. It is worse in that it is not obvious: + if (table == sysctl_mount_point) + sysctl_set_perm_empty_ctl_header(head); > Which kind of unsafety do you envision here? Making the code obvious during patch review hy this is needed / special, and if we special case this, why not remove enum, and make it specific to only that one table. The catch is that it is not immediately obvious that we actually call sysctl_set_perm_empty_ctl_header() in other places, and it begs the question if this can be cleaned up somehow. Luis
On 2023-12-05 14:50:01-0800, Luis Chamberlain wrote: > On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > return -EROFS; > > > > > > > > /* Am I creating a permanently empty directory? */ > > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > > + if (header->ctl_table == sysctl_mount_point || > > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > > return -EINVAL; > > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > > > While you're at it. > > > > This hunk is completely gone in v3/the code that you merged. > > It is worse in that it is not obvious: > > + if (table == sysctl_mount_point) > + sysctl_set_perm_empty_ctl_header(head); > > > Which kind of unsafety do you envision here? > > Making the code obvious during patch review hy this is needed / > special, and if we special case this, why not remove enum, and make it > specific to only that one table. The catch is that it is not > immediately obvious that we actually call > sysctl_set_perm_empty_ctl_header() in other places, and it begs the > question if this can be cleaned up somehow. Making it specific won't work because the flag needs to be transferred from the leaf table to the table representing the directory. What do you think of the aproach taken in the attached patch? (On top of current sysctl-next, including my series) Note: Current sysctl-next ist still based on v6.6. From 2fb9887fb2a5024c2620f2d694bc6dcc32afde67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> Date: Wed, 6 Dec 2023 06:17:22 +0100 Subject: [PATCH] sysctl: simplify handling of permanently empty directories --- fs/proc/proc_sysctl.c | 76 +++++++++++++++++++----------------------- include/linux/sysctl.h | 13 ++------ 2 files changed, 36 insertions(+), 53 deletions(-) diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index c92e9b972ada..c4d6d09b0e68 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -17,6 +17,7 @@ #include <linux/bpf-cgroup.h> #include <linux/mount.h> #include <linux/kmemleak.h> +#include <linux/cleanup.h> #include "internal.h" #define list_for_each_table_entry(entry, header) \ @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; static const struct file_operations proc_sys_dir_file_operations; static const struct inode_operations proc_sys_dir_operations; -/* Support for permanently empty directories */ -static const struct ctl_table sysctl_mount_point[] = { - { } -}; - -/** - * register_sysctl_mount_point() - registers a sysctl mount point - * @path: path for the mount point - * - * Used to create a permanently empty directory to serve as mount point. - * There are some subtle but important permission checks this allows in the - * case of unprivileged mounts. - */ -struct ctl_table_header *register_sysctl_mount_point(const char *path) -{ - return register_sysctl(path, sysctl_mount_point); -} -EXPORT_SYMBOL(register_sysctl_mount_point); - -#define sysctl_is_perm_empty_ctl_header(hptr) \ - (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) -#define sysctl_set_perm_empty_ctl_header(hptr) \ - (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) -#define sysctl_clear_perm_empty_ctl_header(hptr) \ - (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT) - void proc_sys_poll_notify(struct ctl_table_poll *poll) { if (!poll) @@ -199,8 +174,6 @@ static void init_header(struct ctl_table_header *head, head->set = set; head->parent = NULL; head->node = node; - if (table == sysctl_mount_point) - sysctl_set_perm_empty_ctl_header(head); INIT_HLIST_HEAD(&head->inodes); if (node) { const struct ctl_table *entry; @@ -228,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) /* Is this a permanently empty directory? */ - if (sysctl_is_perm_empty_ctl_header(dir_h)) + if (dir->permanently_empty) return -EROFS; - /* Am I creating a permanently empty directory? */ - if (header->ctl_table_size > 0 && - sysctl_is_perm_empty_ctl_header(header)) { - if (!RB_EMPTY_ROOT(&dir->root)) - return -EINVAL; - sysctl_set_perm_empty_ctl_header(dir_h); - } - dir_h->nreg++; header->parent = dir; err = insert_links(header); @@ -254,8 +219,6 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) erase_header(header); put_links(header); fail_links: - if (header->ctl_table == sysctl_mount_point) - sysctl_clear_perm_empty_ctl_header(dir_h); header->parent = NULL; drop_sysctl_table(dir_h); return err; @@ -442,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, struct ctl_table_header *head, const struct ctl_table *table) { struct ctl_table_root *root = head->root; + struct ctl_dir *ctl_dir; struct inode *inode; struct proc_inode *ei; @@ -475,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, inode->i_mode |= S_IFDIR; inode->i_op = &proc_sys_dir_operations; inode->i_fop = &proc_sys_dir_file_operations; - if (sysctl_is_perm_empty_ctl_header(head)) + + ctl_dir = container_of(head, struct ctl_dir, header); + if (ctl_dir->permanently_empty) make_empty_dir_inode(inode); } @@ -1214,8 +1180,7 @@ static bool get_links(struct ctl_dir *dir, struct ctl_table_header *tmp_head; const struct ctl_table *entry, *link; - if (header->ctl_table_size == 0 || - sysctl_is_perm_empty_ctl_header(header)) + if (header->ctl_table_size == 0 || dir->permanently_empty) return true; /* Are there links available for every entry in table? */ @@ -1536,6 +1501,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) } EXPORT_SYMBOL(unregister_sysctl_table); +/** + * register_sysctl_mount_point() - registers a sysctl mount point + * @path: path for the mount point + * + * Used to create a permanently empty directory to serve as mount point. + * There are some subtle but important permission checks this allows in the + * case of unprivileged mounts. + */ +struct ctl_table_header *register_sysctl_mount_point(const char *path) +{ + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); + + if (IS_ERR(dir)) + return NULL; + + guard(spinlock)(&sysctl_lock); + + if (!RB_EMPTY_ROOT(&dir->root)) { + drop_sysctl_table(&dir->header); + return NULL; + } + + dir->permanently_empty = true; + return &dir->header; +} +EXPORT_SYMBOL(register_sysctl_mount_point); + void setup_sysctl_set(struct ctl_table_set *set, struct ctl_table_root *root, int (*is_seen)(struct ctl_table_set *)) diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index 7c96d5abafc7..329e68d484ed 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -177,23 +177,14 @@ struct ctl_table_header { struct ctl_dir *parent; struct ctl_node *node; struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */ - /** - * enum type - Enumeration to differentiate between ctl target types - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently - * empty directory target to serve - * as mount point. - */ - enum { - SYSCTL_TABLE_TYPE_DEFAULT, - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY - } type; }; struct ctl_dir { /* Header must be at the start of ctl_dir */ struct ctl_table_header header; struct rb_root root; + /* Permanently empty directory target to serve as mount point. */ + bool permanently_empty; }; struct ctl_table_set { base-commit: a6fd07f80ab7bd94edb4d56c35e61117ffb9957e prerequisite-patch-id: 0000000000000000000000000000000000000000 prerequisite-patch-id: 13932e9add940cb65c71e04b5efdfcd3622fd27e prerequisite-patch-id: 2e4d88f7b8aaa805598f0e87a3ea726825bb4264 prerequisite-patch-id: 674a680d9cb138cd34cfd0e1a4ec3a5d1c220078 prerequisite-patch-id: e27c92582aa20b1dfb122c172b336dbaf9d6508a prerequisite-patch-id: 9b409a34ab6a4d8d8c5225ba9a72db3116e3c8b3 prerequisite-patch-id: 86ff15a81d850ebda16bb707491251f4b705e4fd prerequisite-patch-id: b7ab65512ac9acfb2dd482b0271b399467afc56d prerequisite-patch-id: 0354922fbf2508a89f3e9d9a4e274fc98deb2e93 prerequisite-patch-id: b71389e82026ffc19cbb717bba1a014ad6cab6da prerequisite-patch-id: fbb0201f89bf6c41d0585af867bdeec8d51649b2 prerequisite-patch-id: e3b4b5b69b4eadf87ed97beb8c03a471e7628cb9 prerequisite-patch-id: 3fbc9745cf3f28872b3e63f6d1f6e2fd7598be8a prerequisite-patch-id: ba2b190c2e54cfb505a282e688c2222712f0acd7 prerequisite-patch-id: 47e5ca730748bb7bf9248a9e711045d8c1028199 prerequisite-patch-id: dcd9f87f00290d2f9be83e404f8883eb90c5fb1c prerequisite-patch-id: d4629be1a61585ab821da2d2850f246761f72f25 prerequisite-patch-id: f740190f4b94e57cbf3659f220d94483713341a1 prerequisite-patch-id: 301c2e530e2af4568267e19247d4a49ac2a9871d
This is the patch that I said could be on its own to facilitate review On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > In a future commit the sysctl core will only use > "const struct ctl_table". As a preparation for that move this mutable > field from "struct ctl_table" to "struct ctl_table_header". > > This is also more correct in general as this is in fact a property of > the header and not the table itself. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > fs/proc/proc_sysctl.c | 11 ++++++----- > include/linux/sysctl.h | 22 +++++++++++----------- > 2 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 689a30196d0c..a398cc77637f 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -31,7 +31,7 @@ static const struct inode_operations proc_sys_dir_operations; > > /* Support for permanently empty directories */ > static struct ctl_table sysctl_mount_point[] = { > - {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY } > + { } > }; > > /** > @@ -49,11 +49,11 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path) > EXPORT_SYMBOL(register_sysctl_mount_point); > > #define sysctl_is_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > + (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > #define sysctl_set_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > + (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > #define sysctl_clear_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT) > + (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT) > > void proc_sys_poll_notify(struct ctl_table_poll *poll) > { > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > return -EROFS; > > /* Am I creating a permanently empty directory? */ > - if (sysctl_is_perm_empty_ctl_header(header)) { > + if (header->ctl_table == sysctl_mount_point || > + sysctl_is_perm_empty_ctl_header(header)) { Why do you have to check that it is equal to sysctl_mount_point? It should be enough to make sure that the type of PERMANENTLY_EMPTY. no? > if (!RB_EMPTY_ROOT(&dir->root)) > return -EINVAL; > sysctl_set_perm_empty_ctl_header(dir_h); > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index ada36ef8cecb..061ea65104be 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -137,17 +137,6 @@ struct ctl_table { > void *data; > int maxlen; > umode_t mode; > - /** > - * enum type - Enumeration to differentiate between ctl target types > - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > - * empty directory target to serve > - * as mount point. > - */ > - enum { > - SYSCTL_TABLE_TYPE_DEFAULT, > - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > - } type; > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > void *extra1; > @@ -188,6 +177,17 @@ struct ctl_table_header { > struct ctl_dir *parent; > struct ctl_node *node; > struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */ > + /** > + * enum type - Enumeration to differentiate between ctl target types > + * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > + * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > + * empty directory target to serve > + * as mount point. > + */ > + enum { > + SYSCTL_TABLE_TYPE_DEFAULT, > + SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > + } type; > }; > > struct ctl_dir { > > -- > 2.43.0 >
On Tue, Dec 05, 2023 at 02:50:01PM -0800, Luis Chamberlain wrote: > On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > return -EROFS; > > > > > > > > /* Am I creating a permanently empty directory? */ > > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > > + if (header->ctl_table == sysctl_mount_point || > > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > > return -EINVAL; > > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > > > While you're at it. > > > > This hunk is completely gone in v3/the code that you merged. > > It is worse in that it is not obvious: > > + if (table == sysctl_mount_point) > + sysctl_set_perm_empty_ctl_header(head); Notice that the test is done on the header and the set is done on the dir_h. I mention this because here you wrote: "sysctl_set_perm_empty_ctl_header(head)" instead of "sysctl_set_perm_empty_ctl_header(dir_h)" dir_h and head are different. Was this your concern? or did I miss your point? > > > Which kind of unsafety do you envision here? > > Making the code obvious during patch review hy this is needed / > special, and if we special case this, why not remove enum, and make it > specific to only that one table. The catch is that it is not > immediately obvious that we actually call > sysctl_set_perm_empty_ctl_header() in other places, and it begs the > question if this can be cleaned up somehow. > > Luis
On Wed, Dec 06, 2023 at 06:53:10AM +0100, Thomas Weißschuh wrote: > On 2023-12-05 14:50:01-0800, Luis Chamberlain wrote: > > On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > > return -EROFS; > > > > > > > > > > /* Am I creating a permanently empty directory? */ > > > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > > > + if (header->ctl_table == sysctl_mount_point || > > > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > > > return -EINVAL; > > > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > > > > > While you're at it. > > > > > > This hunk is completely gone in v3/the code that you merged. > > > > It is worse in that it is not obvious: > > > > + if (table == sysctl_mount_point) > > + sysctl_set_perm_empty_ctl_header(head); > > > > > Which kind of unsafety do you envision here? > > > > Making the code obvious during patch review hy this is needed / > > special, and if we special case this, why not remove enum, and make it > > specific to only that one table. The catch is that it is not > > immediately obvious that we actually call > > sysctl_set_perm_empty_ctl_header() in other places, and it begs the > > question if this can be cleaned up somehow. > > Making it specific won't work because the flag needs to be transferred > from the leaf table to the table representing the directory. > > What do you think of the aproach taken in the attached patch? > (On top of current sysctl-next, including my series) What would this new patch be fixing again? I could not follow ? Additionally, this might be another reason to set this patch aside :) > > Note: Current sysctl-next ist still based on v6.6. > From 2fb9887fb2a5024c2620f2d694bc6dcc32afde67 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> > Date: Wed, 6 Dec 2023 06:17:22 +0100 > Subject: [PATCH] sysctl: simplify handling of permanently empty directories > > --- > fs/proc/proc_sysctl.c | 76 +++++++++++++++++++----------------------- > include/linux/sysctl.h | 13 ++------ > 2 files changed, 36 insertions(+), 53 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index c92e9b972ada..c4d6d09b0e68 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -17,6 +17,7 @@ > #include <linux/bpf-cgroup.h> > #include <linux/mount.h> > #include <linux/kmemleak.h> > +#include <linux/cleanup.h> > #include "internal.h" > > #define list_for_each_table_entry(entry, header) \ > @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; > static const struct file_operations proc_sys_dir_file_operations; > static const struct inode_operations proc_sys_dir_operations; > > -/* Support for permanently empty directories */ > -static const struct ctl_table sysctl_mount_point[] = { > - { } > -}; > - > -/** > - * register_sysctl_mount_point() - registers a sysctl mount point > - * @path: path for the mount point > - * > - * Used to create a permanently empty directory to serve as mount point. > - * There are some subtle but important permission checks this allows in the > - * case of unprivileged mounts. > - */ > -struct ctl_table_header *register_sysctl_mount_point(const char *path) > -{ > - return register_sysctl(path, sysctl_mount_point); > -} > -EXPORT_SYMBOL(register_sysctl_mount_point); > - > -#define sysctl_is_perm_empty_ctl_header(hptr) \ > - (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_set_perm_empty_ctl_header(hptr) \ > - (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_clear_perm_empty_ctl_header(hptr) \ > - (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT) > - > void proc_sys_poll_notify(struct ctl_table_poll *poll) > { > if (!poll) > @@ -199,8 +174,6 @@ static void init_header(struct ctl_table_header *head, > head->set = set; > head->parent = NULL; > head->node = node; > - if (table == sysctl_mount_point) > - sysctl_set_perm_empty_ctl_header(head); > INIT_HLIST_HEAD(&head->inodes); > if (node) { > const struct ctl_table *entry; > @@ -228,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > /* Is this a permanently empty directory? */ > - if (sysctl_is_perm_empty_ctl_header(dir_h)) > + if (dir->permanently_empty) > return -EROFS; > > - /* Am I creating a permanently empty directory? */ > - if (header->ctl_table_size > 0 && > - sysctl_is_perm_empty_ctl_header(header)) { > - if (!RB_EMPTY_ROOT(&dir->root)) > - return -EINVAL; > - sysctl_set_perm_empty_ctl_header(dir_h); > - } > - > dir_h->nreg++; > header->parent = dir; > err = insert_links(header); > @@ -254,8 +219,6 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > erase_header(header); > put_links(header); > fail_links: > - if (header->ctl_table == sysctl_mount_point) > - sysctl_clear_perm_empty_ctl_header(dir_h); > header->parent = NULL; > drop_sysctl_table(dir_h); > return err; > @@ -442,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > struct ctl_table_header *head, const struct ctl_table *table) > { > struct ctl_table_root *root = head->root; > + struct ctl_dir *ctl_dir; > struct inode *inode; > struct proc_inode *ei; > > @@ -475,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > inode->i_mode |= S_IFDIR; > inode->i_op = &proc_sys_dir_operations; > inode->i_fop = &proc_sys_dir_file_operations; > - if (sysctl_is_perm_empty_ctl_header(head)) > + > + ctl_dir = container_of(head, struct ctl_dir, header); > + if (ctl_dir->permanently_empty) > make_empty_dir_inode(inode); > } > > @@ -1214,8 +1180,7 @@ static bool get_links(struct ctl_dir *dir, > struct ctl_table_header *tmp_head; > const struct ctl_table *entry, *link; > > - if (header->ctl_table_size == 0 || > - sysctl_is_perm_empty_ctl_header(header)) > + if (header->ctl_table_size == 0 || dir->permanently_empty) > return true; > > /* Are there links available for every entry in table? */ > @@ -1536,6 +1501,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) > } > EXPORT_SYMBOL(unregister_sysctl_table); > > +/** > + * register_sysctl_mount_point() - registers a sysctl mount point > + * @path: path for the mount point > + * > + * Used to create a permanently empty directory to serve as mount point. > + * There are some subtle but important permission checks this allows in the > + * case of unprivileged mounts. > + */ > +struct ctl_table_header *register_sysctl_mount_point(const char *path) > +{ > + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); > + > + if (IS_ERR(dir)) > + return NULL; > + > + guard(spinlock)(&sysctl_lock); > + > + if (!RB_EMPTY_ROOT(&dir->root)) { > + drop_sysctl_table(&dir->header); > + return NULL; > + } > + > + dir->permanently_empty = true; > + return &dir->header; > +} > +EXPORT_SYMBOL(register_sysctl_mount_point); > + > void setup_sysctl_set(struct ctl_table_set *set, > struct ctl_table_root *root, > int (*is_seen)(struct ctl_table_set *)) > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index 7c96d5abafc7..329e68d484ed 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -177,23 +177,14 @@ struct ctl_table_header { > struct ctl_dir *parent; > struct ctl_node *node; > struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */ > - /** > - * enum type - Enumeration to differentiate between ctl target types > - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > - * empty directory target to serve > - * as mount point. > - */ > - enum { > - SYSCTL_TABLE_TYPE_DEFAULT, > - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > - } type; > }; > > struct ctl_dir { > /* Header must be at the start of ctl_dir */ > struct ctl_table_header header; > struct rb_root root; > + /* Permanently empty directory target to serve as mount point. */ > + bool permanently_empty; > }; > > struct ctl_table_set { > > base-commit: a6fd07f80ab7bd94edb4d56c35e61117ffb9957e > prerequisite-patch-id: 0000000000000000000000000000000000000000 > prerequisite-patch-id: 13932e9add940cb65c71e04b5efdfcd3622fd27e > prerequisite-patch-id: 2e4d88f7b8aaa805598f0e87a3ea726825bb4264 > prerequisite-patch-id: 674a680d9cb138cd34cfd0e1a4ec3a5d1c220078 > prerequisite-patch-id: e27c92582aa20b1dfb122c172b336dbaf9d6508a > prerequisite-patch-id: 9b409a34ab6a4d8d8c5225ba9a72db3116e3c8b3 > prerequisite-patch-id: 86ff15a81d850ebda16bb707491251f4b705e4fd > prerequisite-patch-id: b7ab65512ac9acfb2dd482b0271b399467afc56d > prerequisite-patch-id: 0354922fbf2508a89f3e9d9a4e274fc98deb2e93 > prerequisite-patch-id: b71389e82026ffc19cbb717bba1a014ad6cab6da > prerequisite-patch-id: fbb0201f89bf6c41d0585af867bdeec8d51649b2 > prerequisite-patch-id: e3b4b5b69b4eadf87ed97beb8c03a471e7628cb9 > prerequisite-patch-id: 3fbc9745cf3f28872b3e63f6d1f6e2fd7598be8a > prerequisite-patch-id: ba2b190c2e54cfb505a282e688c2222712f0acd7 > prerequisite-patch-id: 47e5ca730748bb7bf9248a9e711045d8c1028199 > prerequisite-patch-id: dcd9f87f00290d2f9be83e404f8883eb90c5fb1c > prerequisite-patch-id: d4629be1a61585ab821da2d2850f246761f72f25 > prerequisite-patch-id: f740190f4b94e57cbf3659f220d94483713341a1 > prerequisite-patch-id: 301c2e530e2af4568267e19247d4a49ac2a9871d > -- > 2.43.0 >
On 2023-12-07 13:14:37+0100, Joel Granados wrote: > On Wed, Dec 06, 2023 at 06:53:10AM +0100, Thomas Weißschuh wrote: > > On 2023-12-05 14:50:01-0800, Luis Chamberlain wrote: > > > On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > > > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > > > return -EROFS; > > > > > > > > > > > > /* Am I creating a permanently empty directory? */ > > > > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > > > > + if (header->ctl_table == sysctl_mount_point || > > > > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > > > > return -EINVAL; > > > > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > > > > > > > While you're at it. > > > > > > > > This hunk is completely gone in v3/the code that you merged. > > > > > > It is worse in that it is not obvious: > > > > > > + if (table == sysctl_mount_point) > > > + sysctl_set_perm_empty_ctl_header(head); > > > > > > > Which kind of unsafety do you envision here? > > > > > > Making the code obvious during patch review hy this is needed / > > > special, and if we special case this, why not remove enum, and make it > > > specific to only that one table. The catch is that it is not > > > immediately obvious that we actually call > > > sysctl_set_perm_empty_ctl_header() in other places, and it begs the > > > question if this can be cleaned up somehow. > > > > Making it specific won't work because the flag needs to be transferred > > from the leaf table to the table representing the directory. > > > > What do you think of the aproach taken in the attached patch? > > (On top of current sysctl-next, including my series) > What would this new patch be fixing again? I could not follow ? This patch improves upon and replaces the patch you asked to submit on its own: "sysctl: move sysctl type to ctl_table_header". The current code and my original patch have to work around the fact that the "empty" flag is first registered on a *leaf* ctl_table and from there has to be transferred to the *directory* ctl_table somehow. Which is confusing, at least it was for me and evidently also Luis. The new code just directly sets the flag on the directory ctl_table and gets rid of some now-dead code. I should have written a proper changelog... > Additionally, this might be another reason to set this patch aside :) I hope we get this one in, it seems cleaner now. If you agree I can send it as a proper standalone preparation patch. > > > > Note: Current sysctl-next ist still based on v6.6. > > > From 2fb9887fb2a5024c2620f2d694bc6dcc32afde67 Mon Sep 17 00:00:00 2001 > > From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> > > Date: Wed, 6 Dec 2023 06:17:22 +0100 > > Subject: [PATCH] sysctl: simplify handling of permanently empty directories > > > > --- > > fs/proc/proc_sysctl.c | 76 +++++++++++++++++++----------------------- > > include/linux/sysctl.h | 13 ++------ > > 2 files changed, 36 insertions(+), 53 deletions(-) > [..]
Hey Thomas. I had some available cycles today and wanted to look at the patch that you sent. I could not apply it on top of 6.7-rc6. Have you thought about taking this out of your "constification" series and posting it as something that comes before the const stuff? Best On Wed, Dec 06, 2023 at 06:53:10AM +0100, Thomas Weißschuh wrote: > On 2023-12-05 14:50:01-0800, Luis Chamberlain wrote: > > On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > > return -EROFS; > > > > > > > > > > /* Am I creating a permanently empty directory? */ > > > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > > > + if (header->ctl_table == sysctl_mount_point || > > > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > > > return -EINVAL; > > > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > > > > > While you're at it. > > > > > > This hunk is completely gone in v3/the code that you merged. > > > > It is worse in that it is not obvious: > > > > + if (table == sysctl_mount_point) > > + sysctl_set_perm_empty_ctl_header(head); > > > > > Which kind of unsafety do you envision here? > > > > Making the code obvious during patch review hy this is needed / > > special, and if we special case this, why not remove enum, and make it > > specific to only that one table. The catch is that it is not > > immediately obvious that we actually call > > sysctl_set_perm_empty_ctl_header() in other places, and it begs the > > question if this can be cleaned up somehow. > > Making it specific won't work because the flag needs to be transferred > from the leaf table to the table representing the directory. > > What do you think of the aproach taken in the attached patch? > (On top of current sysctl-next, including my series) > > Note: Current sysctl-next ist still based on v6.6. > From 2fb9887fb2a5024c2620f2d694bc6dcc32afde67 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> > Date: Wed, 6 Dec 2023 06:17:22 +0100 > Subject: [PATCH] sysctl: simplify handling of permanently empty directories > > --- > fs/proc/proc_sysctl.c | 76 +++++++++++++++++++----------------------- > include/linux/sysctl.h | 13 ++------ > 2 files changed, 36 insertions(+), 53 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index c92e9b972ada..c4d6d09b0e68 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -17,6 +17,7 @@ > #include <linux/bpf-cgroup.h> > #include <linux/mount.h> > #include <linux/kmemleak.h> > +#include <linux/cleanup.h> > #include "internal.h" > > #define list_for_each_table_entry(entry, header) \ > @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; > static const struct file_operations proc_sys_dir_file_operations; > static const struct inode_operations proc_sys_dir_operations; > > -/* Support for permanently empty directories */ > -static const struct ctl_table sysctl_mount_point[] = { > - { } > -}; > - > -/** > - * register_sysctl_mount_point() - registers a sysctl mount point > - * @path: path for the mount point > - * > - * Used to create a permanently empty directory to serve as mount point. > - * There are some subtle but important permission checks this allows in the > - * case of unprivileged mounts. > - */ > -struct ctl_table_header *register_sysctl_mount_point(const char *path) > -{ > - return register_sysctl(path, sysctl_mount_point); > -} > -EXPORT_SYMBOL(register_sysctl_mount_point); > - > -#define sysctl_is_perm_empty_ctl_header(hptr) \ > - (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_set_perm_empty_ctl_header(hptr) \ > - (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_clear_perm_empty_ctl_header(hptr) \ > - (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT) > - > void proc_sys_poll_notify(struct ctl_table_poll *poll) > { > if (!poll) > @@ -199,8 +174,6 @@ static void init_header(struct ctl_table_header *head, > head->set = set; > head->parent = NULL; > head->node = node; > - if (table == sysctl_mount_point) > - sysctl_set_perm_empty_ctl_header(head); > INIT_HLIST_HEAD(&head->inodes); > if (node) { > const struct ctl_table *entry; > @@ -228,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > /* Is this a permanently empty directory? */ > - if (sysctl_is_perm_empty_ctl_header(dir_h)) > + if (dir->permanently_empty) > return -EROFS; > > - /* Am I creating a permanently empty directory? */ > - if (header->ctl_table_size > 0 && > - sysctl_is_perm_empty_ctl_header(header)) { > - if (!RB_EMPTY_ROOT(&dir->root)) > - return -EINVAL; > - sysctl_set_perm_empty_ctl_header(dir_h); > - } > - > dir_h->nreg++; > header->parent = dir; > err = insert_links(header); > @@ -254,8 +219,6 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > erase_header(header); > put_links(header); > fail_links: > - if (header->ctl_table == sysctl_mount_point) > - sysctl_clear_perm_empty_ctl_header(dir_h); > header->parent = NULL; > drop_sysctl_table(dir_h); > return err; > @@ -442,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > struct ctl_table_header *head, const struct ctl_table *table) > { > struct ctl_table_root *root = head->root; > + struct ctl_dir *ctl_dir; > struct inode *inode; > struct proc_inode *ei; > > @@ -475,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > inode->i_mode |= S_IFDIR; > inode->i_op = &proc_sys_dir_operations; > inode->i_fop = &proc_sys_dir_file_operations; > - if (sysctl_is_perm_empty_ctl_header(head)) > + > + ctl_dir = container_of(head, struct ctl_dir, header); > + if (ctl_dir->permanently_empty) > make_empty_dir_inode(inode); > } > > @@ -1214,8 +1180,7 @@ static bool get_links(struct ctl_dir *dir, > struct ctl_table_header *tmp_head; > const struct ctl_table *entry, *link; > > - if (header->ctl_table_size == 0 || > - sysctl_is_perm_empty_ctl_header(header)) > + if (header->ctl_table_size == 0 || dir->permanently_empty) > return true; > > /* Are there links available for every entry in table? */ > @@ -1536,6 +1501,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) > } > EXPORT_SYMBOL(unregister_sysctl_table); > > +/** > + * register_sysctl_mount_point() - registers a sysctl mount point > + * @path: path for the mount point > + * > + * Used to create a permanently empty directory to serve as mount point. > + * There are some subtle but important permission checks this allows in the > + * case of unprivileged mounts. > + */ > +struct ctl_table_header *register_sysctl_mount_point(const char *path) > +{ > + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); > + > + if (IS_ERR(dir)) > + return NULL; > + > + guard(spinlock)(&sysctl_lock); > + > + if (!RB_EMPTY_ROOT(&dir->root)) { > + drop_sysctl_table(&dir->header); > + return NULL; > + } > + > + dir->permanently_empty = true; > + return &dir->header; > +} > +EXPORT_SYMBOL(register_sysctl_mount_point); > + > void setup_sysctl_set(struct ctl_table_set *set, > struct ctl_table_root *root, > int (*is_seen)(struct ctl_table_set *)) > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index 7c96d5abafc7..329e68d484ed 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -177,23 +177,14 @@ struct ctl_table_header { > struct ctl_dir *parent; > struct ctl_node *node; > struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */ > - /** > - * enum type - Enumeration to differentiate between ctl target types > - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > - * empty directory target to serve > - * as mount point. > - */ > - enum { > - SYSCTL_TABLE_TYPE_DEFAULT, > - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > - } type; > }; > > struct ctl_dir { > /* Header must be at the start of ctl_dir */ > struct ctl_table_header header; > struct rb_root root; > + /* Permanently empty directory target to serve as mount point. */ > + bool permanently_empty; > }; > > struct ctl_table_set { > > base-commit: a6fd07f80ab7bd94edb4d56c35e61117ffb9957e > prerequisite-patch-id: 0000000000000000000000000000000000000000 > prerequisite-patch-id: 13932e9add940cb65c71e04b5efdfcd3622fd27e > prerequisite-patch-id: 2e4d88f7b8aaa805598f0e87a3ea726825bb4264 > prerequisite-patch-id: 674a680d9cb138cd34cfd0e1a4ec3a5d1c220078 > prerequisite-patch-id: e27c92582aa20b1dfb122c172b336dbaf9d6508a > prerequisite-patch-id: 9b409a34ab6a4d8d8c5225ba9a72db3116e3c8b3 > prerequisite-patch-id: 86ff15a81d850ebda16bb707491251f4b705e4fd > prerequisite-patch-id: b7ab65512ac9acfb2dd482b0271b399467afc56d > prerequisite-patch-id: 0354922fbf2508a89f3e9d9a4e274fc98deb2e93 > prerequisite-patch-id: b71389e82026ffc19cbb717bba1a014ad6cab6da > prerequisite-patch-id: fbb0201f89bf6c41d0585af867bdeec8d51649b2 > prerequisite-patch-id: e3b4b5b69b4eadf87ed97beb8c03a471e7628cb9 > prerequisite-patch-id: 3fbc9745cf3f28872b3e63f6d1f6e2fd7598be8a > prerequisite-patch-id: ba2b190c2e54cfb505a282e688c2222712f0acd7 > prerequisite-patch-id: 47e5ca730748bb7bf9248a9e711045d8c1028199 > prerequisite-patch-id: dcd9f87f00290d2f9be83e404f8883eb90c5fb1c > prerequisite-patch-id: d4629be1a61585ab821da2d2850f246761f72f25 > prerequisite-patch-id: f740190f4b94e57cbf3659f220d94483713341a1 > prerequisite-patch-id: 301c2e530e2af4568267e19247d4a49ac2a9871d > -- > 2.43.0 >
Hi Joel, On 2023-12-21 13:09:19+0100, Joel Granados wrote: > I had some available cycles today and wanted to look at the patch that > you sent. I could not apply it on top of 6.7-rc6. Have you thought about > taking this out of your "constification" series and posting it as > something that comes before the const stuff? Yes, I am planning to submit this standalone. But I need to do some more investigation to properly handle the existing comments. FYI this out-of-series patch is meant to be applied on top of the rest of the series. Another question: When did you write this mail? The Date header says it's from the 21st but I only received it today (the 23rd). All other mail headers also say that it was only sent on 23rd. The same seems to happen with your other mails, too. Thomas > On Wed, Dec 06, 2023 at 06:53:10AM +0100, Thomas Weißschuh wrote: > > On 2023-12-05 14:50:01-0800, Luis Chamberlain wrote: > > > On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > > > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > > > return -EROFS; > > > > > > > > > > > > /* Am I creating a permanently empty directory? */ > > > > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > > > > + if (header->ctl_table == sysctl_mount_point || > > > > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > > > > return -EINVAL; > > > > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > > > > > > > While you're at it. > > > > > > > > This hunk is completely gone in v3/the code that you merged. > > > > > > It is worse in that it is not obvious: > > > > > > + if (table == sysctl_mount_point) > > > + sysctl_set_perm_empty_ctl_header(head); > > > > > > > Which kind of unsafety do you envision here? > > > > > > Making the code obvious during patch review hy this is needed / > > > special, and if we special case this, why not remove enum, and make it > > > specific to only that one table. The catch is that it is not > > > immediately obvious that we actually call > > > sysctl_set_perm_empty_ctl_header() in other places, and it begs the > > > question if this can be cleaned up somehow. > > > > Making it specific won't work because the flag needs to be transferred > > from the leaf table to the table representing the directory. > > > > What do you think of the aproach taken in the attached patch? > > (On top of current sysctl-next, including my series) > > > > Note: Current sysctl-next ist still based on v6.6. > > > From 2fb9887fb2a5024c2620f2d694bc6dcc32afde67 Mon Sep 17 00:00:00 2001 > > From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> > > Date: Wed, 6 Dec 2023 06:17:22 +0100 > > Subject: [PATCH] sysctl: simplify handling of permanently empty directories > > > > --- > > fs/proc/proc_sysctl.c | 76 +++++++++++++++++++----------------------- > > include/linux/sysctl.h | 13 ++------ > > 2 files changed, 36 insertions(+), 53 deletions(-) > > > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > > index c92e9b972ada..c4d6d09b0e68 100644 > > --- a/fs/proc/proc_sysctl.c > > +++ b/fs/proc/proc_sysctl.c > > @@ -17,6 +17,7 @@ > > #include <linux/bpf-cgroup.h> > > #include <linux/mount.h> > > #include <linux/kmemleak.h> > > +#include <linux/cleanup.h> > > #include "internal.h" > > > > #define list_for_each_table_entry(entry, header) \ > > @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; > > static const struct file_operations proc_sys_dir_file_operations; > > static const struct inode_operations proc_sys_dir_operations; > > > > -/* Support for permanently empty directories */ > > -static const struct ctl_table sysctl_mount_point[] = { > > - { } > > -}; > > - > > -/** > > - * register_sysctl_mount_point() - registers a sysctl mount point > > - * @path: path for the mount point > > - * > > - * Used to create a permanently empty directory to serve as mount point. > > - * There are some subtle but important permission checks this allows in the > > - * case of unprivileged mounts. > > - */ > > -struct ctl_table_header *register_sysctl_mount_point(const char *path) > > -{ > > - return register_sysctl(path, sysctl_mount_point); > > -} > > -EXPORT_SYMBOL(register_sysctl_mount_point); > > - > > -#define sysctl_is_perm_empty_ctl_header(hptr) \ > > - (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > > -#define sysctl_set_perm_empty_ctl_header(hptr) \ > > - (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > > -#define sysctl_clear_perm_empty_ctl_header(hptr) \ > > - (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT) > > - > > void proc_sys_poll_notify(struct ctl_table_poll *poll) > > { > > if (!poll) > > @@ -199,8 +174,6 @@ static void init_header(struct ctl_table_header *head, > > head->set = set; > > head->parent = NULL; > > head->node = node; > > - if (table == sysctl_mount_point) > > - sysctl_set_perm_empty_ctl_header(head); > > INIT_HLIST_HEAD(&head->inodes); > > if (node) { > > const struct ctl_table *entry; > > @@ -228,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > > > /* Is this a permanently empty directory? */ > > - if (sysctl_is_perm_empty_ctl_header(dir_h)) > > + if (dir->permanently_empty) > > return -EROFS; > > > > - /* Am I creating a permanently empty directory? */ > > - if (header->ctl_table_size > 0 && > > - sysctl_is_perm_empty_ctl_header(header)) { > > - if (!RB_EMPTY_ROOT(&dir->root)) > > - return -EINVAL; > > - sysctl_set_perm_empty_ctl_header(dir_h); > > - } > > - > > dir_h->nreg++; > > header->parent = dir; > > err = insert_links(header); > > @@ -254,8 +219,6 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > erase_header(header); > > put_links(header); > > fail_links: > > - if (header->ctl_table == sysctl_mount_point) > > - sysctl_clear_perm_empty_ctl_header(dir_h); > > header->parent = NULL; > > drop_sysctl_table(dir_h); > > return err; > > @@ -442,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > > struct ctl_table_header *head, const struct ctl_table *table) > > { > > struct ctl_table_root *root = head->root; > > + struct ctl_dir *ctl_dir; > > struct inode *inode; > > struct proc_inode *ei; > > > > @@ -475,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > > inode->i_mode |= S_IFDIR; > > inode->i_op = &proc_sys_dir_operations; > > inode->i_fop = &proc_sys_dir_file_operations; > > - if (sysctl_is_perm_empty_ctl_header(head)) > > + > > + ctl_dir = container_of(head, struct ctl_dir, header); > > + if (ctl_dir->permanently_empty) > > make_empty_dir_inode(inode); > > } > > > > @@ -1214,8 +1180,7 @@ static bool get_links(struct ctl_dir *dir, > > struct ctl_table_header *tmp_head; > > const struct ctl_table *entry, *link; > > > > - if (header->ctl_table_size == 0 || > > - sysctl_is_perm_empty_ctl_header(header)) > > + if (header->ctl_table_size == 0 || dir->permanently_empty) > > return true; > > > > /* Are there links available for every entry in table? */ > > @@ -1536,6 +1501,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) > > } > > EXPORT_SYMBOL(unregister_sysctl_table); > > > > +/** > > + * register_sysctl_mount_point() - registers a sysctl mount point > > + * @path: path for the mount point > > + * > > + * Used to create a permanently empty directory to serve as mount point. > > + * There are some subtle but important permission checks this allows in the > > + * case of unprivileged mounts. > > + */ > > +struct ctl_table_header *register_sysctl_mount_point(const char *path) > > +{ > > + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); > > + > > + if (IS_ERR(dir)) > > + return NULL; > > + > > + guard(spinlock)(&sysctl_lock); > > + > > + if (!RB_EMPTY_ROOT(&dir->root)) { > > + drop_sysctl_table(&dir->header); > > + return NULL; > > + } > > + > > + dir->permanently_empty = true; > > + return &dir->header; > > +} > > +EXPORT_SYMBOL(register_sysctl_mount_point); > > + > > void setup_sysctl_set(struct ctl_table_set *set, > > struct ctl_table_root *root, > > int (*is_seen)(struct ctl_table_set *)) > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > > index 7c96d5abafc7..329e68d484ed 100644 > > --- a/include/linux/sysctl.h > > +++ b/include/linux/sysctl.h > > @@ -177,23 +177,14 @@ struct ctl_table_header { > > struct ctl_dir *parent; > > struct ctl_node *node; > > struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */ > > - /** > > - * enum type - Enumeration to differentiate between ctl target types > > - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > > - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > > - * empty directory target to serve > > - * as mount point. > > - */ > > - enum { > > - SYSCTL_TABLE_TYPE_DEFAULT, > > - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > > - } type; > > }; > > > > struct ctl_dir { > > /* Header must be at the start of ctl_dir */ > > struct ctl_table_header header; > > struct rb_root root; > > + /* Permanently empty directory target to serve as mount point. */ > > + bool permanently_empty; > > }; > > > > struct ctl_table_set { > > > > base-commit: a6fd07f80ab7bd94edb4d56c35e61117ffb9957e > > prerequisite-patch-id: 0000000000000000000000000000000000000000 > > prerequisite-patch-id: 13932e9add940cb65c71e04b5efdfcd3622fd27e > > prerequisite-patch-id: 2e4d88f7b8aaa805598f0e87a3ea726825bb4264 > > prerequisite-patch-id: 674a680d9cb138cd34cfd0e1a4ec3a5d1c220078 > > prerequisite-patch-id: e27c92582aa20b1dfb122c172b336dbaf9d6508a > > prerequisite-patch-id: 9b409a34ab6a4d8d8c5225ba9a72db3116e3c8b3 > > prerequisite-patch-id: 86ff15a81d850ebda16bb707491251f4b705e4fd > > prerequisite-patch-id: b7ab65512ac9acfb2dd482b0271b399467afc56d > > prerequisite-patch-id: 0354922fbf2508a89f3e9d9a4e274fc98deb2e93 > > prerequisite-patch-id: b71389e82026ffc19cbb717bba1a014ad6cab6da > > prerequisite-patch-id: fbb0201f89bf6c41d0585af867bdeec8d51649b2 > > prerequisite-patch-id: e3b4b5b69b4eadf87ed97beb8c03a471e7628cb9 > > prerequisite-patch-id: 3fbc9745cf3f28872b3e63f6d1f6e2fd7598be8a > > prerequisite-patch-id: ba2b190c2e54cfb505a282e688c2222712f0acd7 > > prerequisite-patch-id: 47e5ca730748bb7bf9248a9e711045d8c1028199 > > prerequisite-patch-id: dcd9f87f00290d2f9be83e404f8883eb90c5fb1c > > prerequisite-patch-id: d4629be1a61585ab821da2d2850f246761f72f25 > > prerequisite-patch-id: f740190f4b94e57cbf3659f220d94483713341a1 > > prerequisite-patch-id: 301c2e530e2af4568267e19247d4a49ac2a9871d > > -- > > 2.43.0 > > > > > -- > > Joel Granados
On Sat, Dec 23, 2023 at 02:04:27PM +0100, Thomas Weißschuh wrote: > Hi Joel, > > On 2023-12-21 13:09:19+0100, Joel Granados wrote: > > I had some available cycles today and wanted to look at the patch that > > you sent. I could not apply it on top of 6.7-rc6. Have you thought about > > taking this out of your "constification" series and posting it as > > something that comes before the const stuff? > > Yes, I am planning to submit this standalone. But I need to do some more > investigation to properly handle the existing comments. Understood. > > FYI this out-of-series patch is meant to be applied on top of the rest > of the series. Ok. > > > Another question: > > When did you write this mail? > > The Date header says it's from the 21st but I only received it today > (the 23rd). All other mail headers also say that it was only sent on > 23rd. The same seems to happen with your other mails, too. Sometimes I do the review even if I don't have connection. Everything goes out once I get back on the network. So there is the timestamp of when I "sent" but was on standby in my outgoing mails 21st. And then the actual sending occurred some days later when I was back on the network. > > Thomas > > > On Wed, Dec 06, 2023 at 06:53:10AM +0100, Thomas Weißschuh wrote: > > > On 2023-12-05 14:50:01-0800, Luis Chamberlain wrote: > > > > On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > > > > > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote: > > > > > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote: > > > > > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > > > > return -EROFS; > > > > > > > > > > > > > > /* Am I creating a permanently empty directory? */ > > > > > > > - if (sysctl_is_perm_empty_ctl_header(header)) { > > > > > > > + if (header->ctl_table == sysctl_mount_point || > > > > > > > + sysctl_is_perm_empty_ctl_header(header)) { > > > > > > > if (!RB_EMPTY_ROOT(&dir->root)) > > > > > > > return -EINVAL; > > > > > > > sysctl_set_perm_empty_ctl_header(dir_h); > > > > > > > > > > > > While you're at it. > > > > > > > > > > This hunk is completely gone in v3/the code that you merged. > > > > > > > > It is worse in that it is not obvious: > > > > > > > > + if (table == sysctl_mount_point) > > > > + sysctl_set_perm_empty_ctl_header(head); > > > > > > > > > Which kind of unsafety do you envision here? > > > > > > > > Making the code obvious during patch review hy this is needed / > > > > special, and if we special case this, why not remove enum, and make it > > > > specific to only that one table. The catch is that it is not > > > > immediately obvious that we actually call > > > > sysctl_set_perm_empty_ctl_header() in other places, and it begs the > > > > question if this can be cleaned up somehow. > > > > > > Making it specific won't work because the flag needs to be transferred > > > from the leaf table to the table representing the directory. > > > > > > What do you think of the aproach taken in the attached patch? > > > (On top of current sysctl-next, including my series) > > > > > > Note: Current sysctl-next ist still based on v6.6. > > > > > From 2fb9887fb2a5024c2620f2d694bc6dcc32afde67 Mon Sep 17 00:00:00 2001 > > > From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> > > > Date: Wed, 6 Dec 2023 06:17:22 +0100 > > > Subject: [PATCH] sysctl: simplify handling of permanently empty directories > > > > > > --- > > > fs/proc/proc_sysctl.c | 76 +++++++++++++++++++----------------------- > > > include/linux/sysctl.h | 13 ++------ > > > 2 files changed, 36 insertions(+), 53 deletions(-) > > > > > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > > > index c92e9b972ada..c4d6d09b0e68 100644 > > > --- a/fs/proc/proc_sysctl.c > > > +++ b/fs/proc/proc_sysctl.c > > > @@ -17,6 +17,7 @@ > > > #include <linux/bpf-cgroup.h> > > > #include <linux/mount.h> > > > #include <linux/kmemleak.h> > > > +#include <linux/cleanup.h> > > > #include "internal.h" > > > > > > #define list_for_each_table_entry(entry, header) \ > > > @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; > > > static const struct file_operations proc_sys_dir_file_operations; > > > static const struct inode_operations proc_sys_dir_operations; > > > > > > -/* Support for permanently empty directories */ > > > -static const struct ctl_table sysctl_mount_point[] = { > > > - { } > > > -}; > > > - > > > -/** > > > - * register_sysctl_mount_point() - registers a sysctl mount point > > > - * @path: path for the mount point > > > - * > > > - * Used to create a permanently empty directory to serve as mount point. > > > - * There are some subtle but important permission checks this allows in the > > > - * case of unprivileged mounts. > > > - */ > > > -struct ctl_table_header *register_sysctl_mount_point(const char *path) > > > -{ > > > - return register_sysctl(path, sysctl_mount_point); > > > -} > > > -EXPORT_SYMBOL(register_sysctl_mount_point); > > > - > > > -#define sysctl_is_perm_empty_ctl_header(hptr) \ > > > - (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > > > -#define sysctl_set_perm_empty_ctl_header(hptr) \ > > > - (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > > > -#define sysctl_clear_perm_empty_ctl_header(hptr) \ > > > - (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT) > > > - > > > void proc_sys_poll_notify(struct ctl_table_poll *poll) > > > { > > > if (!poll) > > > @@ -199,8 +174,6 @@ static void init_header(struct ctl_table_header *head, > > > head->set = set; > > > head->parent = NULL; > > > head->node = node; > > > - if (table == sysctl_mount_point) > > > - sysctl_set_perm_empty_ctl_header(head); > > > INIT_HLIST_HEAD(&head->inodes); > > > if (node) { > > > const struct ctl_table *entry; > > > @@ -228,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > > > > > > > /* Is this a permanently empty directory? */ > > > - if (sysctl_is_perm_empty_ctl_header(dir_h)) > > > + if (dir->permanently_empty) > > > return -EROFS; > > > > > > - /* Am I creating a permanently empty directory? */ > > > - if (header->ctl_table_size > 0 && > > > - sysctl_is_perm_empty_ctl_header(header)) { > > > - if (!RB_EMPTY_ROOT(&dir->root)) > > > - return -EINVAL; > > > - sysctl_set_perm_empty_ctl_header(dir_h); > > > - } > > > - > > > dir_h->nreg++; > > > header->parent = dir; > > > err = insert_links(header); > > > @@ -254,8 +219,6 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > erase_header(header); > > > put_links(header); > > > fail_links: > > > - if (header->ctl_table == sysctl_mount_point) > > > - sysctl_clear_perm_empty_ctl_header(dir_h); > > > header->parent = NULL; > > > drop_sysctl_table(dir_h); > > > return err; > > > @@ -442,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > > > struct ctl_table_header *head, const struct ctl_table *table) > > > { > > > struct ctl_table_root *root = head->root; > > > + struct ctl_dir *ctl_dir; > > > struct inode *inode; > > > struct proc_inode *ei; > > > > > > @@ -475,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > > > inode->i_mode |= S_IFDIR; > > > inode->i_op = &proc_sys_dir_operations; > > > inode->i_fop = &proc_sys_dir_file_operations; > > > - if (sysctl_is_perm_empty_ctl_header(head)) > > > + > > > + ctl_dir = container_of(head, struct ctl_dir, header); > > > + if (ctl_dir->permanently_empty) > > > make_empty_dir_inode(inode); > > > } > > > > > > @@ -1214,8 +1180,7 @@ static bool get_links(struct ctl_dir *dir, > > > struct ctl_table_header *tmp_head; > > > const struct ctl_table *entry, *link; > > > > > > - if (header->ctl_table_size == 0 || > > > - sysctl_is_perm_empty_ctl_header(header)) > > > + if (header->ctl_table_size == 0 || dir->permanently_empty) > > > return true; > > > > > > /* Are there links available for every entry in table? */ > > > @@ -1536,6 +1501,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) > > > } > > > EXPORT_SYMBOL(unregister_sysctl_table); > > > > > > +/** > > > + * register_sysctl_mount_point() - registers a sysctl mount point > > > + * @path: path for the mount point > > > + * > > > + * Used to create a permanently empty directory to serve as mount point. > > > + * There are some subtle but important permission checks this allows in the > > > + * case of unprivileged mounts. > > > + */ > > > +struct ctl_table_header *register_sysctl_mount_point(const char *path) > > > +{ > > > + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); > > > + > > > + if (IS_ERR(dir)) > > > + return NULL; > > > + > > > + guard(spinlock)(&sysctl_lock); > > > + > > > + if (!RB_EMPTY_ROOT(&dir->root)) { > > > + drop_sysctl_table(&dir->header); > > > + return NULL; > > > + } > > > + > > > + dir->permanently_empty = true; > > > + return &dir->header; > > > +} > > > +EXPORT_SYMBOL(register_sysctl_mount_point); > > > + > > > void setup_sysctl_set(struct ctl_table_set *set, > > > struct ctl_table_root *root, > > > int (*is_seen)(struct ctl_table_set *)) > > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > > > index 7c96d5abafc7..329e68d484ed 100644 > > > --- a/include/linux/sysctl.h > > > +++ b/include/linux/sysctl.h > > > @@ -177,23 +177,14 @@ struct ctl_table_header { > > > struct ctl_dir *parent; > > > struct ctl_node *node; > > > struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */ > > > - /** > > > - * enum type - Enumeration to differentiate between ctl target types > > > - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > > > - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > > > - * empty directory target to serve > > > - * as mount point. > > > - */ > > > - enum { > > > - SYSCTL_TABLE_TYPE_DEFAULT, > > > - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > > > - } type; > > > }; > > > > > > struct ctl_dir { > > > /* Header must be at the start of ctl_dir */ > > > struct ctl_table_header header; > > > struct rb_root root; > > > + /* Permanently empty directory target to serve as mount point. */ > > > + bool permanently_empty; > > > }; > > > > > > struct ctl_table_set { > > > > > > base-commit: a6fd07f80ab7bd94edb4d56c35e61117ffb9957e > > > prerequisite-patch-id: 0000000000000000000000000000000000000000 > > > prerequisite-patch-id: 13932e9add940cb65c71e04b5efdfcd3622fd27e > > > prerequisite-patch-id: 2e4d88f7b8aaa805598f0e87a3ea726825bb4264 > > > prerequisite-patch-id: 674a680d9cb138cd34cfd0e1a4ec3a5d1c220078 > > > prerequisite-patch-id: e27c92582aa20b1dfb122c172b336dbaf9d6508a > > > prerequisite-patch-id: 9b409a34ab6a4d8d8c5225ba9a72db3116e3c8b3 > > > prerequisite-patch-id: 86ff15a81d850ebda16bb707491251f4b705e4fd > > > prerequisite-patch-id: b7ab65512ac9acfb2dd482b0271b399467afc56d > > > prerequisite-patch-id: 0354922fbf2508a89f3e9d9a4e274fc98deb2e93 > > > prerequisite-patch-id: b71389e82026ffc19cbb717bba1a014ad6cab6da > > > prerequisite-patch-id: fbb0201f89bf6c41d0585af867bdeec8d51649b2 > > > prerequisite-patch-id: e3b4b5b69b4eadf87ed97beb8c03a471e7628cb9 > > > prerequisite-patch-id: 3fbc9745cf3f28872b3e63f6d1f6e2fd7598be8a > > > prerequisite-patch-id: ba2b190c2e54cfb505a282e688c2222712f0acd7 > > > prerequisite-patch-id: 47e5ca730748bb7bf9248a9e711045d8c1028199 > > > prerequisite-patch-id: dcd9f87f00290d2f9be83e404f8883eb90c5fb1c > > > prerequisite-patch-id: d4629be1a61585ab821da2d2850f246761f72f25 > > > prerequisite-patch-id: f740190f4b94e57cbf3659f220d94483713341a1 > > > prerequisite-patch-id: 301c2e530e2af4568267e19247d4a49ac2a9871d > > > -- > > > 2.43.0 > > > > > > > > > -- > > > > Joel Granados > >
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 689a30196d0c..a398cc77637f 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -31,7 +31,7 @@ static const struct inode_operations proc_sys_dir_operations; /* Support for permanently empty directories */ static struct ctl_table sysctl_mount_point[] = { - {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY } + { } }; /** @@ -49,11 +49,11 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path) EXPORT_SYMBOL(register_sysctl_mount_point); #define sysctl_is_perm_empty_ctl_header(hptr) \ - (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) + (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) #define sysctl_set_perm_empty_ctl_header(hptr) \ - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) + (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) #define sysctl_clear_perm_empty_ctl_header(hptr) \ - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT) + (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT) void proc_sys_poll_notify(struct ctl_table_poll *poll) { @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) return -EROFS; /* Am I creating a permanently empty directory? */ - if (sysctl_is_perm_empty_ctl_header(header)) { + if (header->ctl_table == sysctl_mount_point || + sysctl_is_perm_empty_ctl_header(header)) { if (!RB_EMPTY_ROOT(&dir->root)) return -EINVAL; sysctl_set_perm_empty_ctl_header(dir_h); diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index ada36ef8cecb..061ea65104be 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -137,17 +137,6 @@ struct ctl_table { void *data; int maxlen; umode_t mode; - /** - * enum type - Enumeration to differentiate between ctl target types - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently - * empty directory target to serve - * as mount point. - */ - enum { - SYSCTL_TABLE_TYPE_DEFAULT, - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY - } type; proc_handler *proc_handler; /* Callback for text formatting */ struct ctl_table_poll *poll; void *extra1; @@ -188,6 +177,17 @@ struct ctl_table_header { struct ctl_dir *parent; struct ctl_node *node; struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */ + /** + * enum type - Enumeration to differentiate between ctl target types + * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations + * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently + * empty directory target to serve + * as mount point. + */ + enum { + SYSCTL_TABLE_TYPE_DEFAULT, + SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY + } type; }; struct ctl_dir {
In a future commit the sysctl core will only use "const struct ctl_table". As a preparation for that move this mutable field from "struct ctl_table" to "struct ctl_table_header". This is also more correct in general as this is in fact a property of the header and not the table itself. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- fs/proc/proc_sysctl.c | 11 ++++++----- include/linux/sysctl.h | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 16 deletions(-)