Message-ID: <20231212090930.y4omk62wenxgo5by@localhost>
Date:   Tue, 12 Dec 2023 10:09:30 +0100
From:   Joel Granados <j.granados@...sung.com>
To:     Thomas Weißschuh <linux@...ssschuh.net>
CC:     Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-hardening@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables

On Mon, Dec 11, 2023 at 12:25:10PM +0100, Thomas Weißschuh wrote:
> On 2023-12-08 10:59:26+0100, Joel Granados wrote:
> > On Thu, Dec 07, 2023 at 08:19:43PM +0100, Thomas Weißschuh wrote:
> > > On 2023-12-07 11:43:57+0100, Joel Granados wrote:
> 
> > [..]
> 
> > > > I suggest you chunk it up with directories in mind. Something similar to
> > > > what I did for [4] where I divided stuff that when for fs/*, kernel/*,
> > > > net/*, arch/* and drivers/*. That will complicate your patch a tad
> > > > because you have to ensure that the tree can be compiled/run for every
> > > > commit. But it will pay off once you push it to the broader public.
> > > 
> > > This will break bisections. All function signatures need to be switched
> 
> > I was suggesting a solution without breaking bisections of course. I can
> > think of a couple of ways to do this in chunks but it might be
> > premature. You can send it and if you get push back because of this then
> > we can deal with chunking it down.
> 
> I'm curious about those ways. I don't see how to split the big commit.
My idea was to do something similar to your originl RFC, where you have
an temporary proc_handler something like proc_hdlr_const (we would need
to work on the name) and move each subsystem to the new handler while
the others stay with the non-const one. At the end, the old proc_handler
function name would disapear and would be completely replaced by the new
proc_hdlr_const.

This is of course extra work and might not be worth it if you don't get
negative feedback related to tree-wide changes. Therefore I stick to my
previous suggestion. Send the big tree-wide patches and only explore
this option if someone screams.
> 
> > I'm still concerned about the header size for those mails. How does the
> > mail look like when you run the get maintainers script on it?
> 
> The full series has 142 recipients in total,
> the biggest patch itself has 124.
This seems low. might just be enough to make into the lists that have
the check.

In any case it depends on the size of the header; if it is greater than
8912 you get a message back similar to this one:
"linux-raid-owne ( 22K) BOUNCE linux-raid@...r.kernel.org: Header field too long (>8192)"

> 
> Before sending it I'd like to get feedback on the internal rework of the
> is_empty detection from you and/or Luis.
Yep, this is still on my todo. With holidays just around the corner,
I'll be a bit slower to get to it. I'll eventually get to it if someone
else does not beat me to it :)

Thx for reminding me.
> 
> https://git.sr.ht/~t-8ch/linux/commit/ea27507070f3c47be6febebe451bbb88f6ea707e
> or the attached patch.
> 
> > [..]

> From ea27507070f3c47be6febebe451bbb88f6ea707e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@...ssschuh.net>
> Date: Sun, 3 Dec 2023 21:56:46 +0100
> Subject: [PATCH] sysctl: move permanently empty flag to ctl_dir
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Simplify the logic by always keeping the permanently_empty flag on the
> ctl_dir.
> The previous logic kept the flag in the leaf ctl_table and from there
> transferred it to the ctl_table from the directory.
> 
> This also removes the need to have a mutable ctl_table and will allow
> the constification of those structs.
> 
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> ---
>  fs/proc/proc_sysctl.c  | 74 +++++++++++++++++++-----------------------
>  include/linux/sysctl.h | 13 ++------
>  2 files changed, 36 insertions(+), 51 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 35c97ad54f34..33f41af58e9b 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 struct ctl_table sysctl_mount_point[] = {
> -	{.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY }
> -};
> -
> -/**
> - * 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->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)	\
> -	(hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT)
> -
>  void proc_sys_poll_notify(struct ctl_table_poll *poll)
>  {
>  	if (!poll)
> @@ -226,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);
> @@ -252,8 +219,6 @@ fail:
>  	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;
> @@ -440,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>  		struct ctl_table_header *head, struct ctl_table *table)
>  {
>  	struct ctl_table_root *root = head->root;
> +	struct ctl_dir *ctl_dir;
>  	struct inode *inode;
>  	struct proc_inode *ei;
>  
> @@ -473,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);
>  	}
>  
> @@ -1211,8 +1179,7 @@ static bool get_links(struct ctl_dir *dir,
>  	struct ctl_table_header *tmp_head;
>  	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? */
> @@ -1533,6 +1500,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 ada36ef8cecb..57cb0060d7d7 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;
> @@ -194,6 +183,8 @@ 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 {
> -- 
> 2.38.5
> 


-- 

Joel Granados

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)

Powered by blists - more mailing lists