Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables

From: "Thomas Weißschuh" <linux@weissschuh.net>
To: Joel Granados <j.granados@samsung.com>
Cc: Kees Cook <keescook@chromium.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Iurii Zaikin <yzaikin@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables
Date: Mon, 11 Dec 2023 12:25:10 +0100	[thread overview]
Message-ID: <8509a36b-ac23-4fcd-b797-f8915662d5e1@t-8ch.de> (raw)
In-Reply-To: <20231208095926.aavsjrtqbb5rygmb@localhost>

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

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.

> 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.

Before sending it I'd like to get feedback on the internal rework of the
is_empty detection from you and/or Luis.

https://git.sr.ht/~t-8ch/linux/commit/ea27507070f3c47be6febebe451bbb88f6ea707e
or the attached patch.

> [..]

[-- Attachment #2: ea27507070f3c47be6febebe451bbb88f6ea707e.patch --]
[-- Type: text/plain, Size: 6381 bytes --]

From ea27507070f3c47be6febebe451bbb88f6ea707e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.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@weissschuh.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

