[PATCH 0/4] sysctl: move sysctl type to ctl_table_header

[PATCH 0/4] sysctl: move sysctl type to ctl_table_header

[Thomas Weißschuh]

Praparation series to enable constification of struct ctl_table further
down the line.
No functional changes are intended.

These changes have been split out and reworked from my original
const sysctl patchset [0].
I'm resubmitting the patchset in smaller chunks for easier review.
Each split-out series is meant to be useful on its own.

Changes since the original series:
* Explicit initializartion of header->type in init_header()
* Some additional cleanups

[0] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net/

---
Thomas Weißschuh (4):
      sysctl: drop sysctl_is_perm_empty_ctl_table
      sysctl: move sysctl type to ctl_table_header
      sysctl: drop now unnecessary out-of-bounds check
      sysctl: remove unnecessary sentinel element

 fs/proc/proc_sysctl.c  | 19 ++++++++-----------
 include/linux/sysctl.h | 22 +++++++++++-----------
 2 files changed, 19 insertions(+), 22 deletions(-)
---
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
change-id: 20231216-sysctl-empty-dir-71d7631f7bfe

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/4] sysctl: drop sysctl_is_perm_empty_ctl_table

[Thomas Weißschuh]

It is used only twice and those callers are simpler with
sysctl_is_perm_empty_ctl_header().
So use this sibling function.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 fs/proc/proc_sysctl.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 37cde0efee57..2f4d4329d83d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -48,10 +48,8 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path)
 }
 EXPORT_SYMBOL(register_sysctl_mount_point);
 
-#define sysctl_is_perm_empty_ctl_table(tptr)		\
-	(tptr[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
 #define sysctl_is_perm_empty_ctl_header(hptr)		\
-	(sysctl_is_perm_empty_ctl_table(hptr->ctl_table))
+	(hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
 #define sysctl_set_perm_empty_ctl_header(hptr)		\
 	(hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
 #define sysctl_clear_perm_empty_ctl_header(hptr)	\
@@ -233,7 +231,7 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
 
 	/* Am I creating a permanently empty directory? */
 	if (header->ctl_table_size > 0 &&
-	    sysctl_is_perm_empty_ctl_table(header->ctl_table)) {
+	    sysctl_is_perm_empty_ctl_header(header)) {
 		if (!RB_EMPTY_ROOT(&dir->root))
 			return -EINVAL;
 		sysctl_set_perm_empty_ctl_header(dir_h);
@@ -1204,7 +1202,7 @@ static bool get_links(struct ctl_dir *dir,
 	struct ctl_table *entry, *link;
 
 	if (header->ctl_table_size == 0 ||
-	    sysctl_is_perm_empty_ctl_table(header->ctl_table))
+	    sysctl_is_perm_empty_ctl_header(header))
 		return true;
 
 	/* Are there links available for every entry in table? */

-- 
2.43.2

Re: [PATCH 1/4] sysctl: drop sysctl_is_perm_empty_ctl_table

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

On Thu, Feb 22, 2024 at 08:07:36AM +0100, Thomas Weißschuh wrote:
> It is used only twice and those callers are simpler with
> sysctl_is_perm_empty_ctl_header().
> So use this sibling function.
Can you please add a comment relating this to the constification effort.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  fs/proc/proc_sysctl.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 37cde0efee57..2f4d4329d83d 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -48,10 +48,8 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path)
>  }
>  EXPORT_SYMBOL(register_sysctl_mount_point);
>  
> -#define sysctl_is_perm_empty_ctl_table(tptr)		\
> -	(tptr[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
>  #define sysctl_is_perm_empty_ctl_header(hptr)		\
> -	(sysctl_is_perm_empty_ctl_table(hptr->ctl_table))
> +	(hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
>  #define sysctl_set_perm_empty_ctl_header(hptr)		\
>  	(hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
>  #define sysctl_clear_perm_empty_ctl_header(hptr)	\
> @@ -233,7 +231,7 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
>  
>  	/* Am I creating a permanently empty directory? */
>  	if (header->ctl_table_size > 0 &&
> -	    sysctl_is_perm_empty_ctl_table(header->ctl_table)) {
> +	    sysctl_is_perm_empty_ctl_header(header)) {
>  		if (!RB_EMPTY_ROOT(&dir->root))
>  			return -EINVAL;
>  		sysctl_set_perm_empty_ctl_header(dir_h);
> @@ -1204,7 +1202,7 @@ static bool get_links(struct ctl_dir *dir,
>  	struct ctl_table *entry, *link;
>  
>  	if (header->ctl_table_size == 0 ||
> -	    sysctl_is_perm_empty_ctl_table(header->ctl_table))
> +	    sysctl_is_perm_empty_ctl_header(header))
>  		return true;
>  
>  	/* Are there links available for every entry in table? */
> 
> -- 
> 2.43.2
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 2/4] sysctl: move sysctl type to ctl_table_header

[Thomas Weißschuh]

As static initialization of the is not possible anymore move it into
init_header() where all the other header fields are also initialized.

Reduce memory consumption as there are less instances of
ctl_table_header than ctl_table.

Removing this mutable member also opens the way to constify static
instances of ctl_table.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 fs/proc/proc_sysctl.c  | 10 ++++++----
 include/linux/sysctl.h | 22 +++++++++++-----------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 2f4d4329d83d..fde7a2f773f0 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)
 {
@@ -208,6 +208,8 @@ static void init_header(struct ctl_table_header *head,
 			node++;
 		}
 	}
+	if (table == sysctl_mount_point)
+		sysctl_set_perm_empty_ctl_header(head);
 }
 
 static void erase_header(struct ctl_table_header *head)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index ee7d33b89e9e..c87f73c06cb9 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.2

Re: [PATCH 2/4] sysctl: move sysctl type to ctl_table_header

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 4291 bytes --]

On Thu, Feb 22, 2024 at 08:07:37AM +0100, Thomas Weißschuh wrote:
> As static initialization of the is not possible anymore move it into
> init_header() where all the other header fields are also initialized.
Please say what was done. Something like: "Moved the
SYSCTL_TABLE_TYPE_{DEFAULT,PERMANENTLY_EMPTY} enumerations from
ctl_table to ctl_table_header."

And the then you can mention the why: "Removing the mutable memeber from
ctl_table opens the ...."

> 
> Reduce memory consumption as there are less instances of
> ctl_table_header than ctl_table.
This is indeed true, but the main reasoning behind this is
constification. Right? If you want to leave this comment, I would
suggest you add something that talks about the amount of bytes saved.
Something like : "Reduce memory consumption by sizeof(int) for every
ctl_table entry in ctl_table_header". Or something similar.

> 
> Removing this mutable member also opens the way to constify static
> instances of ctl_table.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  fs/proc/proc_sysctl.c  | 10 ++++++----
>  include/linux/sysctl.h | 22 +++++++++++-----------
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 2f4d4329d83d..fde7a2f773f0 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)
>  {
> @@ -208,6 +208,8 @@ static void init_header(struct ctl_table_header *head,
>  			node++;
>  		}
>  	}
> +	if (table == sysctl_mount_point)
> +		sysctl_set_perm_empty_ctl_header(head);
>  }
>  
>  static void erase_header(struct ctl_table_header *head)
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index ee7d33b89e9e..c87f73c06cb9 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.2
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 3/4] sysctl: drop now unnecessary out-of-bounds check

[Thomas Weißschuh]

The type field is now part of the header so
sysctl_is_perm_empty_ctl_header() can safely be executed even without
any ctl_tables.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 fs/proc/proc_sysctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index fde7a2f773f0..4cdf98c6a9a4 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -232,8 +232,7 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
 		return -EROFS;
 
 	/* Am I creating a permanently empty directory? */
-	if (header->ctl_table_size > 0 &&
-	    sysctl_is_perm_empty_ctl_header(header)) {
+	if (sysctl_is_perm_empty_ctl_header(header)) {
 		if (!RB_EMPTY_ROOT(&dir->root))
 			return -EINVAL;
 		sysctl_set_perm_empty_ctl_header(dir_h);

-- 
2.43.2

Re: [PATCH 3/4] sysctl: drop now unnecessary out-of-bounds check

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On Thu, Feb 22, 2024 at 08:07:38AM +0100, Thomas Weißschuh wrote:
> The type field is now part of the header so
> sysctl_is_perm_empty_ctl_header() can safely be executed even without
> any ctl_tables.

Only comments on the commit message.
1. Can you please put this and your 4/4 patch together. Since 4/4 comes
   is a result of this 3/4 patch, then they can be in one.
2. Please re-write the commit message to state what you did: Something
   like : "Remove the now unneeded check for ctl_table_size; it is safe
   to do so as it is now part of ctl_table_header.
3. Remember to mention the removal of the sentinel when you merge 3/4
   and 4/4

> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  fs/proc/proc_sysctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index fde7a2f773f0..4cdf98c6a9a4 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -232,8 +232,7 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
>  		return -EROFS;
>  
>  	/* Am I creating a permanently empty directory? */
> -	if (header->ctl_table_size > 0 &&
> -	    sysctl_is_perm_empty_ctl_header(header)) {
> +	if (sysctl_is_perm_empty_ctl_header(header)) {
>  		if (!RB_EMPTY_ROOT(&dir->root))
>  			return -EINVAL;
>  		sysctl_set_perm_empty_ctl_header(dir_h);
> 
> -- 
> 2.43.2
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 4/4] sysctl: remove unnecessary sentinel element

[Thomas Weißschuh]

The previous commits made this sentinel element unnecessary, remove it.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 fs/proc/proc_sysctl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 4cdf98c6a9a4..7c0e27dc3d9d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -30,9 +30,7 @@ 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 struct ctl_table sysctl_mount_point[] = {
-	{ }
-};
+static struct ctl_table sysctl_mount_point[] = { };
 
 /**
  * register_sysctl_mount_point() - registers a sysctl mount point

-- 
2.43.2

Re: [PATCH 0/4] sysctl: move sysctl type to ctl_table_header

[Luis Chamberlain]

On Thu, Feb 22, 2024 at 08:07:35AM +0100, Thomas Weißschuh wrote:
> Praparation series to enable constification of struct ctl_table further
> down the line.
> No functional changes are intended.
> 
> These changes have been split out and reworked from my original
> const sysctl patchset [0].
> I'm resubmitting the patchset in smaller chunks for easier review.
> Each split-out series is meant to be useful on its own.
> 
> Changes since the original series:
> * Explicit initializartion of header->type in init_header()
> * Some additional cleanups
> 
> [0] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net/

This all looks super sexy to me, but I'd love Eric's feedback on patch
series before merging as he may know some other facts over sysctl_mount_point
I might be missing.

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 0/4] sysctl: move sysctl type to ctl_table_header

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]

On Thu, Feb 22, 2024 at 08:07:35AM +0100, Thomas Weißschuh wrote:
> Praparation series to enable constification of struct ctl_table further
> down the line.
> No functional changes are intended.
Took me some time but I finally got around to it. I just had comments
regarding the commit messages. I'll add this to sysctl testing to get
the ball rolling; I'll add it to sysctl-next after you modify the commit
messages.

Thx.

> 
> These changes have been split out and reworked from my original
> const sysctl patchset [0].
> I'm resubmitting the patchset in smaller chunks for easier review.
> Each split-out series is meant to be useful on its own.
> 
> Changes since the original series:
> * Explicit initializartion of header->type in init_header()
> * Some additional cleanups
> 
> [0] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net/
> 
> ---
> Thomas Weißschuh (4):
>       sysctl: drop sysctl_is_perm_empty_ctl_table
>       sysctl: move sysctl type to ctl_table_header
>       sysctl: drop now unnecessary out-of-bounds check
>       sysctl: remove unnecessary sentinel element
> 
>  fs/proc/proc_sysctl.c  | 19 ++++++++-----------
>  include/linux/sysctl.h | 22 +++++++++++-----------
>  2 files changed, 19 insertions(+), 22 deletions(-)
> ---
> base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
> change-id: 20231216-sysctl-empty-dir-71d7631f7bfe
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]