From: Steven Rostedt <rostedt@goodmis.org>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org,
Masami Hiramatsu <mhiramat@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Andrew Morton <akpm@linux-foundation.org>,
Christian Brauner <brauner@kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>,
Ajay Kaher <ajay.kaher@broadcom.com>,
Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [for-linus][PATCH 1/3] eventfs: Have the inodes all for files and directories all be the same
Date: Mon, 22 Jan 2024 10:06:30 -0500 [thread overview]
Message-ID: <20240122100630.6a400dd3@gandalf.local.home> (raw)
In-Reply-To: <CAMuHMdXKiorg-jiuKoZpfZyDJ3Ynrfb8=X+c7x0Eewxn-YRdCA@mail.gmail.com>
On Mon, 22 Jan 2024 11:38:52 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Stephen,
I don't know who "Stephen" is, but I'll reply to this message.
>
> On Wed, Jan 17, 2024 at 3:37 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> >
> > The dentries and inodes are created in the readdir for the sole purpose of
> > getting a consistent inode number. Linus stated that is unnecessary, and
> > that all inodes can have the same inode number. For a virtual file system
> > they are pretty meaningless.
> >
> > Instead use a single unique inode number for all files and one for all
> > directories.
> >
> > Link: https://lore.kernel.org/all/20240116133753.2808d45e@gandalf.local.home/
Yeah, Linus wanted me to try this first and see if there's any regressions.
Well, I guess you just answered that.
The above link has me saying to Linus:
It was me being paranoid that using the same inode number would break user
space. If that is not a concern, then I'm happy to just make it either the
same, or maybe just hash the ei and name that it is associated with.
> > Link: https://lore.kernel.org/linux-trace-kernel/20240116211353.412180363@goodmis.org
> >
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > Cc: Ajay Kaher <ajay.kaher@broadcom.com>
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>
> Thanks for your patch, which is now commit 53c41052ba312176 ("eventfs:
> Have the inodes all for files and directories all be the same") in
> v6.8-rc1, to which I have bisected the issue below.
>
> > --- a/fs/tracefs/event_inode.c
> > +++ b/fs/tracefs/event_inode.c
> > @@ -32,6 +32,10 @@
> > */
> > static DEFINE_MUTEX(eventfs_mutex);
> >
> > +/* Choose something "unique" ;-) */
> > +#define EVENTFS_FILE_INODE_INO 0x12c4e37
> > +#define EVENTFS_DIR_INODE_INO 0x134b2f5
> > +
> > /*
> > * The eventfs_inode (ei) itself is protected by SRCU. It is released from
> > * its parent's list and will have is_freed set (under eventfs_mutex).
> > @@ -352,6 +356,9 @@ static struct dentry *create_file(const char *name, umode_t mode,
> > inode->i_fop = fop;
> > inode->i_private = data;
> >
> > + /* All files will have the same inode number */
> > + inode->i_ino = EVENTFS_FILE_INODE_INO;
> > +
> > ti = get_tracefs(inode);
> > ti->flags |= TRACEFS_EVENT_INODE;
> > d_instantiate(dentry, inode);
> > @@ -388,6 +395,9 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent
> > inode->i_op = &eventfs_root_dir_inode_operations;
> > inode->i_fop = &eventfs_file_operations;
> >
> > + /* All directories will have the same inode number */
> > + inode->i_ino = EVENTFS_DIR_INODE_INO;
> > +
> > ti = get_tracefs(inode);
> > ti->flags |= TRACEFS_EVENT_INODE;
>
> This confuses "find".
> Running "find /sys/" now prints lots of error messages to stderr:
>
> find: File system loop detected;
> ‘/sys/kernel/debug/tracing/events/initcall/initcall_finish’ is part of
> the same file system loop as
> ‘/sys/kernel/debug/tracing/events/initcall’.
So at a minimum, the directories need to have unique inode numbers.
> find: File system loop detected;
> ‘/sys/kernel/debug/tracing/events/initcall/initcall_start’ is part of
> the same file system loop as
> ‘/sys/kernel/debug/tracing/events/initcall’.
> find: File system loop detected;
> ‘/sys/kernel/debug/tracing/events/initcall/initcall_level’ is part of
> the same file system loop as
> ‘/sys/kernel/debug/tracing/events/initcall’.
> [...]
Does this fix it for you? It hashes the eventfs_inode data structure after
adding some salt to it.
Kees,
I'm using the eventfs_inode pointer to create a unique value for the inode.
But it's being salted, hashed and then truncated. As it is very easy to
read inodes (although by default, only root has access to read these
inodes), the inode numbers themselves shouldn't be able to leak kernel
addresses via the results of these inode numbers, would it?
-- Steve
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 6795fda2af19..d54897b84596 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -19,6 +19,7 @@
#include <linux/namei.h>
#include <linux/workqueue.h>
#include <linux/security.h>
+#include <linux/siphash.h>
#include <linux/tracefs.h>
#include <linux/kref.h>
#include <linux/delay.h>
@@ -36,6 +37,31 @@ static DEFINE_MUTEX(eventfs_mutex);
#define EVENTFS_FILE_INODE_INO 0x12c4e37
#define EVENTFS_DIR_INODE_INO 0x134b2f5
+/* Used for making inode numbers */
+static siphash_key_t inode_key;
+
+/* Copied from scripts/kconfig/symbol.c */
+static unsigned strhash(const char *s)
+{
+ /* fnv32 hash */
+ unsigned hash = 2166136261U;
+ for (; *s; s++)
+ hash = (hash ^ *s) * 0x01000193;
+ return hash;
+}
+
+/* Just try to make something consistent and unique */
+static int eventfs_dir_ino(struct event_inode *ei, const char *name)
+{
+ unsigned long sip = (unsigned long)ei;
+
+ sip += strhash(name) + EVENTFS_DIR_INODE_INO;
+ sip = siphash_1u32((int)sip, &inode_key);
+
+ /* keep it positive */
+ return sip & ((1U << 31) - 1);
+}
+
/*
* The eventfs_inode (ei) itself is protected by SRCU. It is released from
* its parent's list and will have is_freed set (under eventfs_mutex).
@@ -396,7 +422,7 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent
inode->i_fop = &eventfs_file_operations;
/* All directories will have the same inode number */
- inode->i_ino = EVENTFS_DIR_INODE_INO;
+ inode->i_ino = eventfs_dir_ino(ei, ei->name);
ti = get_tracefs(inode);
ti->flags |= TRACEFS_EVENT_INODE;
@@ -802,7 +828,7 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
name = ei_child->name;
- ino = EVENTFS_DIR_INODE_INO;
+ ino = eventfs_dir_ino(ei_child, name);
if (!dir_emit(ctx, name, strlen(name), ino, DT_DIR))
goto out_dec;
@@ -932,6 +958,9 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
if (IS_ERR(dentry))
return ERR_CAST(dentry);
+ if (siphash_key_is_zero(&inode_key))
+ get_random_bytes(&inode_key, sizeof(inode_key));
+
ei = kzalloc(sizeof(*ei), GFP_KERNEL);
if (!ei)
goto fail_ei;
next prev parent reply other threads:[~2024-01-22 15:05 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-17 14:35 [for-linus][PATCH 0/3] eventfs: A few more fixes for 6.8 Steven Rostedt
2024-01-17 14:35 ` [for-linus][PATCH 1/3] eventfs: Have the inodes all for files and directories all be the same Steven Rostedt
2024-01-22 10:38 ` Geert Uytterhoeven
2024-01-22 15:06 ` Steven Rostedt [this message]
2024-01-22 16:23 ` Geert Uytterhoeven
2024-01-22 16:47 ` Steven Rostedt
2024-01-22 17:37 ` Linus Torvalds
2024-01-22 17:39 ` Linus Torvalds
2024-01-22 18:19 ` Linus Torvalds
2024-01-22 18:27 ` Mathieu Desnoyers
2024-01-22 19:37 ` Steven Rostedt
2024-01-22 18:50 ` Kees Cook
2024-01-22 19:44 ` Steven Rostedt
2024-01-22 19:48 ` Steven Rostedt
2024-01-22 21:33 ` Kees Cook
2024-01-25 17:40 ` Christian Brauner
2024-01-25 18:07 ` Steven Rostedt
2024-01-25 18:08 ` Steven Rostedt
2024-01-26 8:07 ` Geert Uytterhoeven
2024-01-26 10:11 ` Christian Brauner
2024-01-26 16:25 ` Steven Rostedt
2024-01-26 19:09 ` Linus Torvalds
2024-01-26 13:16 ` Steven Rostedt
2024-01-26 14:06 ` Steven Rostedt
2024-01-22 17:14 ` Mathieu Desnoyers
2024-01-22 17:50 ` Steven Rostedt
2024-01-22 18:35 ` Mathieu Desnoyers
2024-01-22 19:59 ` Steven Rostedt
2024-01-17 14:35 ` [for-linus][PATCH 2/3] eventfs: Do not create dentries nor inodes in iterate_shared Steven Rostedt
2024-01-17 14:35 ` [for-linus][PATCH 3/3] eventfs: Use kcalloc() instead of kzalloc() Steven Rostedt
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=20240122100630.6a400dd3@gandalf.local.home \
--to=rostedt@goodmis.org \
--cc=ajay.kaher@broadcom.com \
--cc=akpm@linux-foundation.org \
--cc=brauner@kernel.org \
--cc=geert@linux-m68k.org \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mathieu.desnoyers@efficios.com \
--cc=mhiramat@kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
/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).