From: Joel Granados <j.granados@samsung.com>
To: "Thomas Weißschuh" <linux@weissschuh.net>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
Kees Cook <keescook@chromium.org>,
"Gustavo A. R. Silva" <gustavoars@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 13/18] sysctl: move sysctl type to ctl_table_header
Date: Thu, 7 Dec 2023 13:14:37 +0100 [thread overview]
Message-ID: <20231207121437.mfu3gegorsyqvihf@localhost> (raw)
In-Reply-To: <0450d705-3739-4b6d-a1f2-b22d54617de1@t-8ch.de>
[-- Attachment #1: Type: text/plain, Size: 10244 bytes --]
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
>
--
Joel Granados
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
next prev parent reply other threads:[~2023-12-07 12:14 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20231204075237eucas1p27966f7e7da014b5992d3eef89a8fde25@eucas1p2.samsung.com>
2023-12-04 7:52 ` [PATCH v2 00/18] sysctl: constify sysctl ctl_tables Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 01/18] watchdog/core: remove sysctl handlers from public header Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 02/18] sysctl: delete unused define SYSCTL_PERM_EMPTY_DIR Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 03/18] sysctl: drop sysctl_is_perm_empty_ctl_table Thomas Weißschuh
2023-12-07 14:09 ` Joel Granados
2023-12-04 7:52 ` [PATCH v2 04/18] cgroup: bpf: constify ctl_table arguments and fields Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 05/18] seccomp: constify ctl_table arguments of utility functions Thomas Weißschuh
2023-12-04 22:12 ` Kees Cook
2023-12-04 7:52 ` [PATCH v2 06/18] hugetlb: " Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 07/18] utsname: constify ctl_table arguments of utility function Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 08/18] stackleak: don't modify ctl_table argument Thomas Weißschuh
2023-12-04 22:14 ` Kees Cook
2023-12-04 7:52 ` [PATCH v2 09/18] sysctl: treewide: constify ctl_table_root::set_ownership Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 10/18] sysctl: treewide: constify ctl_table_root::permissions Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 11/18] sysctl: treewide: constify ctl_table_header::ctl_table_arg Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 12/18] sysctl: treewide: constify the ctl_table argument of handlers Thomas Weißschuh
2023-12-04 22:17 ` Kees Cook
2023-12-05 9:50 ` kernel test robot
2023-12-05 16:05 ` kernel test robot
2023-12-04 7:52 ` [PATCH v2 13/18] sysctl: move sysctl type to ctl_table_header Thomas Weißschuh
2023-12-05 22:33 ` Luis Chamberlain
2023-12-05 22:41 ` Thomas Weißschuh
2023-12-05 22:50 ` Luis Chamberlain
2023-12-06 5:53 ` Thomas Weißschuh
2023-12-07 12:14 ` Joel Granados [this message]
2023-12-07 19:29 ` Thomas Weißschuh
2023-12-21 12:09 ` Joel Granados
2023-12-23 13:04 ` Thomas Weißschuh
2023-12-24 18:51 ` Joel Granados
2023-12-07 12:05 ` Joel Granados
2023-12-07 11:31 ` Joel Granados
2023-12-04 7:52 ` [PATCH v2 14/18] sysctl: move internal interfaces to const struct ctl_table Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 15/18] sysctl: allow registration of " Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 16/18] const_structs.checkpatch: add ctl_table Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 17/18] sysctl: make ctl_table sysctl_mount_point const Thomas Weißschuh
2023-12-04 7:52 ` [PATCH v2 18/18] sysctl: constify standard sysctl tables Thomas Weißschuh
2023-12-05 5:50 ` [PATCH v2 00/18] sysctl: constify sysctl ctl_tables Luis Chamberlain
2023-12-05 8:04 ` Thomas Weißschuh
2023-12-05 17:16 ` Thomas Weißschuh
2023-12-05 22:27 ` Luis Chamberlain
2023-12-07 11:23 ` Joel Granados
2023-12-07 11:19 ` Joel Granados
2023-12-07 19:23 ` Thomas Weißschuh
2023-12-07 11:05 ` Joel Granados
2023-12-07 10:43 ` Joel Granados
2023-12-07 19:19 ` Thomas Weißschuh
2023-12-08 9:59 ` Joel Granados
2023-12-11 11:25 ` Thomas Weißschuh
2023-12-12 9:09 ` Joel Granados
2023-12-13 7:51 ` Luis Chamberlain
2023-12-15 16:40 ` Thomas Weißschuh
2023-12-15 17:05 ` Julia Lawall
2023-12-17 12:02 ` Joel Granados
2023-12-17 22:10 ` Thomas Weißschuh
2023-12-18 21:21 ` Luis Chamberlain
2023-12-19 19:29 ` Thomas Weißschuh
2023-12-19 20:39 ` Julia Lawall
2023-12-19 21:09 ` Luis Chamberlain
2023-12-19 21:21 ` Julia Lawall
2023-12-20 0:09 ` Luis Chamberlain
2023-12-20 7:39 ` Julia Lawall
2023-12-20 14:34 ` Luis Chamberlain
2023-12-19 23:04 ` Julia Lawall
2023-12-21 12:44 ` Joel Granados
[not found] ` <CGME20231223120907eucas1p20afac63076e1e9d5aee6adaa101c0630@eucas1p2.samsung.com>
2023-12-21 12:36 ` Joel Granados
2023-12-21 12:12 ` Joel Granados
2023-12-13 7:47 ` Luis Chamberlain
2023-12-13 18:18 ` Eric W. Biederman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231207121437.mfu3gegorsyqvihf@localhost \
--to=j.granados@samsung.com \
--cc=gregkh@linuxfoundation.org \
--cc=gustavoars@kernel.org \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@weissschuh.net \
--cc=mcgrof@kernel.org \
--cc=yzaikin@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).