| Message ID | 20240116234014.459886712@goodmis.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | eventfs: Create dentries and inodes at dir open | expand |
On Tue, Jan 16, 2024 at 05:55:32PM -0500, Steven Rostedt 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/ > 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> > --- > 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 <djwong@kernel.org> 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/20240122152748.46897388@gandalf.local.home/ 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 <djwong@kernel.org> 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/20240122152748.46897388@gandalf.local.home/ > > for the current patch under discussion (and it contains a link _to_ > said discussion). Ah, cool, thank you! --D > Linus
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;