From: "Steven Rostedt (Google)" <[email protected]>
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/[email protected]/
Link: https://lore.kernel.org/linux-trace-kernel/[email protected]
Cc: Masami Hiramatsu <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Ajay Kaher <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
fs/tracefs/event_inode.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index fdff53d5a1f8..5edf0b96758b 100644
--- 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;
--
2.43.0
On Tue, Jan 16, 2024 at 05:55:32PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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/[email protected]/
> Link: https://lore.kernel.org/linux-trace-kernel/[email protected]
>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Ajay Kaher <[email protected]>
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
> ---
> fs/tracefs/event_inode.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index fdff53d5a1f8..5edf0b96758b 100644
> --- 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;
Regrettably, this leads to find failing on 6.8-rc1 (see xfs/55[89] in
fstests):
# find /sys/kernel/debug/tracing/ >/dev/null
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’.
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’.
There were no such reports on 6.7.0; AFAICT find(1) is tripping over
parent and child subdirectory having the same dev/i_ino. Changing this
line to the following:
/* All directories will NOT have the same inode number */
inode->i_ino = (unsigned long)inode;
makes the messages about filesystem loops go away, though I don't think
leaking raw kernel pointers is an awesome idea.
--D
> +
> ti = get_tracefs(inode);
> ti->flags |= TRACEFS_EVENT_INODE;
>
> --
> 2.43.0
>
>
>
On Mon, 22 Jan 2024 at 13:59, Darrick J. Wong <[email protected]> wrote:
>
> though I don't think
> leaking raw kernel pointers is an awesome idea.
Yeah, I wasn't all that comfortable even with trying to hash it
(because I think the number of source bits is small enough that even
with a crypto hash, it's trivially brute-forceable).
See
https://lore.kernel.org/all/[email protected]/
for the current patch under discussion (and it contains a link _to_
said discussion).
Linus
On Mon, Jan 22, 2024 at 02:02:28PM -0800, Linus Torvalds wrote:
> On Mon, 22 Jan 2024 at 13:59, Darrick J. Wong <[email protected]> wrote:
> >
> > though I don't think
> > leaking raw kernel pointers is an awesome idea.
>
> Yeah, I wasn't all that comfortable even with trying to hash it
> (because I think the number of source bits is small enough that even
> with a crypto hash, it's trivially brute-forceable).
>
> See
>
> https://lore.kernel.org/all/[email protected]/
>
> for the current patch under discussion (and it contains a link _to_
> said discussion).
Ah, cool, thank you!
--D
> Linus