From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:31
This is a backport of all the work that lead up to the work that Linus made
on eventfs. I trust Linus's version more so than the versions in 6.6 and
6.7. There may be plenty of hidden issues due to the design.
This is the update for 6.7. It includes Linus's updates as well as all the
patches leading up to them.
I ran these through my full test suite that I use before sending anyting to
Linus, althouh I did not run my "bisect" test that walks through the
patches. The tests were just run on the end result. I'm currently running my
6.6 version through my tests.
This was created with the following command against v6.7.3:
git log --reverse --no-merges --pretty=oneline v6.7..origin/master fs/tracefs/ | cut -d' ' -f1 |
while read a; do if ! git cherry-pick -x $a; then break; fi ; done
Which adds -x to the cherry pick to add the upstream commit SHAs.
Erick Archer (1):
eventfs: Use kcalloc() instead of kzalloc()
Linus Torvalds (7):
tracefs: remove stale 'update_gid' code
eventfs: Initialize the tracefs inode properly
tracefs: Avoid using the ei->dentry pointer unnecessarily
tracefs: dentry lookup crapectomy
eventfs: Remove unused d_parent pointer field
eventfs: Clean up dentry ops and add revalidate function
eventfs: Get rid of dentry pointers without refcounts
Steven Rostedt (Google) (15):
eventfs: Remove "lookup" parameter from create_dir/file_dentry()
eventfs: Stop using dcache_readdir() for getdents()
tracefs/eventfs: Use root and instance inodes as default ownership
eventfs: Have eventfs_iterate() stop immediately if ei->is_freed is set
eventfs: Do ctx->pos update for all iterations in eventfs_iterate()
eventfs: Read ei->entries before ei->children in eventfs_iterate()
eventfs: Shortcut eventfs_iterate() by skipping entries already read
eventfs: Have the inodes all for files and directories all be the same
eventfs: Do not create dentries nor inodes in iterate_shared
eventfs: Save directory inodes in the eventfs_inode structure
tracefs: Zero out the tracefs_inode when allocating it
eventfs: Warn if an eventfs_inode is freed without is_freed being set
eventfs: Restructure eventfs_inode structure to be more condensed
eventfs: Remove fsnotify*() functions from lookup()
eventfs: Keep all directory links at 1
----
fs/tracefs/event_inode.c | 905 ++++++++++++++++-------------------------------
fs/tracefs/inode.c | 286 +++++++--------
fs/tracefs/internal.h | 48 ++-
3 files changed, 451 insertions(+), 788 deletions(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:30
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
If ei->is_freed is set in eventfs_iterate(), it means that the directory
that is being iterated on is in the process of being freed. Just exit the
loop immediately when that is ever detected, and separate out the return
of the entry->callback() from ei->is_freed.
Link: https://lore.kernel.org/linux-trace-kernel/20240104220048.016261289@goodmis.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit e109deadb73318cf4a3bd61287d969f705df278f)
---
fs/tracefs/event_inode.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
@@ -788,11 +788,12 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)name=entry->name;mutex_lock(&eventfs_mutex);-/* If ei->is_freed, then the event itself may be too */-if(!ei->is_freed)-r=entry->callback(name,&mode,&cdata,&fops);-else-r=-1;+/* If ei->is_freed then just bail here, nothing more to do */+if(ei->is_freed){+mutex_unlock(&eventfs_mutex);+gotoout;+}+r=entry->callback(name,&mode,&cdata,&fops);mutex_unlock(&eventfs_mutex);if(r<=0)continue;
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:30
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The "lookup" parameter is a way to differentiate the call to
create_file/dir_dentry() from when it's just a lookup (no need to up the
dentry refcount) and accessed via a readdir (need to up the refcount).
But reality, it just makes the code more complex. Just up the refcount and
let the caller decide to dput() the result or not.
Link: https://lore.kernel.org/linux-trace-kernel/20240103102553.17a19cea@gandalf.local.home
Link: https://lore.kernel.org/linux-trace-kernel/20240104015435.517502710@goodmis.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ajay Kaher <redacted>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit b0f7e2d739b4aac131ea1662d086a07775097b05)
---
fs/tracefs/event_inode.c | 55 +++++++++++++++-------------------------
1 file changed, 20 insertions(+), 35 deletions(-)
@@ -414,9 +412,7 @@ create_file_dentry(struct eventfs_inode *ei, int idx,}/* If the e_dentry already has a dentry, use it */if(*e_dentry){-/* lookup does not need to up the ref count */-if(!lookup)-dget(*e_dentry);+dget(*e_dentry);mutex_unlock(&eventfs_mutex);return*e_dentry;}
@@ -441,13 +437,12 @@ create_file_dentry(struct eventfs_inode *ei, int idx,*waytobeingfreed,don'treturnit.Ife_dentryisNULL*itmeansitwasalreadyfreed.*/-if(ei->is_freed)+if(ei->is_freed){dentry=NULL;-else+}else{dentry=*e_dentry;-/* The lookup does not need to up the dentry refcount */-if(dentry&&!lookup)dget(dentry);+}mutex_unlock(&eventfs_mutex);returndentry;}
@@ -465,9 +460,6 @@ create_file_dentry(struct eventfs_inode *ei, int idx,}mutex_unlock(&eventfs_mutex);-if(lookup)-dput(dentry);-returndentry;}
@@ -518,11 +509,9 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,returnNULL;}if(ei->dentry){-/* If the dentry already has a dentry, use it */+/* If the eventfs_inode already has a dentry, use it */dentry=ei->dentry;-/* lookup does not need to up the ref count */-if(!lookup)-dget(dentry);+dget(dentry);mutex_unlock(&eventfs_mutex);returndentry;}
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:30
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The ctx->pos was only updated when it added an entry, but the "skip to
current pos" check (c--) happened for every loop regardless of if the
entry was added or not. This inconsistency caused readdir to be incorrect.
It was due to:
for (i = 0; i < ei->nr_entries; i++) {
if (c > 0) {
c--;
continue;
}
mutex_lock(&eventfs_mutex);
/* If ei->is_freed then just bail here, nothing more to do */
if (ei->is_freed) {
mutex_unlock(&eventfs_mutex);
goto out;
}
r = entry->callback(name, &mode, &cdata, &fops);
mutex_unlock(&eventfs_mutex);
[..]
ctx->pos++;
}
But this can cause the iterator to return a file that was already read.
That's because of the way the callback() works. Some events may not have
all files, and the callback can return 0 to tell eventfs to skip the file
for this directory.
for instance, we have:
# ls /sys/kernel/tracing/events/ftrace/function
format hist hist_debug id inject
and
# ls /sys/kernel/tracing/events/sched/sched_switch/
enable filter format hist hist_debug id inject trigger
Where the function directory is missing "enable", "filter" and
"trigger". That's because the callback() for events has:
static int event_callback(const char *name, umode_t *mode, void **data,
const struct file_operations **fops)
{
struct trace_event_file *file = *data;
struct trace_event_call *call = file->event_call;
[..]
/*
* Only event directories that can be enabled should have
* triggers or filters, with the exception of the "print"
* event that can have a "trigger" file.
*/
if (!(call->flags & TRACE_EVENT_FL_IGNORE_ENABLE)) {
if (call->class->reg && strcmp(name, "enable") == 0) {
*mode = TRACE_MODE_WRITE;
*fops = &ftrace_enable_fops;
return 1;
}
if (strcmp(name, "filter") == 0) {
*mode = TRACE_MODE_WRITE;
*fops = &ftrace_event_filter_fops;
return 1;
}
}
if (!(call->flags & TRACE_EVENT_FL_IGNORE_ENABLE) ||
strcmp(trace_event_name(call), "print") == 0) {
if (strcmp(name, "trigger") == 0) {
*mode = TRACE_MODE_WRITE;
*fops = &event_trigger_fops;
return 1;
}
}
[..]
return 0;
}
Where the function event has the TRACE_EVENT_FL_IGNORE_ENABLE set.
This means that the entries array elements for "enable", "filter" and
"trigger" when called on the function event will have the callback return
0 and not 1, to tell eventfs to skip these files for it.
Because the "skip to current ctx->pos" check happened for all entries, but
the ctx->pos++ only happened to entries that exist, it would confuse the
reading of a directory. Which would cause:
# ls /sys/kernel/tracing/events/ftrace/function/
format hist hist hist_debug hist_debug id inject inject
The missing "enable", "filter" and "trigger" caused ls to show "hist",
"hist_debug" and "inject" twice.
Update the ctx->pos for every iteration to keep its update and the "skip"
update consistent. This also means that on error, the ctx->pos needs to be
decremented if it was incremented without adding something.
Link: https://lore.kernel.org/all/20240104150500.38b15a62@gandalf.local.home/
Link: https://lore.kernel.org/linux-trace-kernel/20240104220048.172295263@goodmis.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: 493ec81a8fb8e ("eventfs: Stop using dcache_readdir() for getdents()")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 1e4624eb5a0ecaae0d2c4e3019bece119725bb98)
---
fs/tracefs/event_inode.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
@@ -791,7 +794,7 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)/* If ei->is_freed then just bail here, nothing more to do */if(ei->is_freed){mutex_unlock(&eventfs_mutex);-gotoout;+gotoout_dec;}r=entry->callback(name,&mode,&cdata,&fops);mutex_unlock(&eventfs_mutex);
@@ -800,19 +803,23 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)dentry=create_file_dentry(ei,i,ei_dentry,name,mode,cdata,fops);if(!dentry)-gotoout;+gotoout_dec;ino=dentry->d_inode->i_ino;dput(dentry);if(!dir_emit(ctx,name,strlen(name),ino,DT_REG))-gotoout;-ctx->pos++;+gotoout_dec;}ret=1;out:srcu_read_unlock(&eventfs_srcu,idx);returnret;++out_dec:+/* Incremented ctx->pos without adding something, reset it */+ctx->pos--;+gotoout;}/**
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:30
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The eventfs creates dynamically allocated dentries and inodes. Using the
dcache_readdir() logic for its own directory lookups requires hiding the
cursor of the dcache logic and playing games to allow the dcache_readdir()
to still have access to the cursor while the eventfs saved what it created
and what it needs to release.
Instead, just have eventfs have its own iterate_shared callback function
that will fill in the dent entries. This simplifies the code quite a bit.
Link: https://lore.kernel.org/linux-trace-kernel/20240104015435.682218477@goodmis.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ajay Kaher <redacted>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 493ec81a8fb8e4ada6f223b8b73791a1280d4774)
---
fs/tracefs/event_inode.c | 194 +++++++++++++--------------------------
1 file changed, 64 insertions(+), 130 deletions(-)
@@ -148,11 +146,9 @@ static const struct inode_operations eventfs_file_inode_operations = {};staticconststructfile_operationseventfs_file_operations={-.open=dcache_dir_open_wrapper,.read=generic_read_dir,-.iterate_shared=dcache_readdir_wrapper,+.iterate_shared=eventfs_iterate,.llseek=generic_file_llseek,-.release=eventfs_release,};/* Return the evenfs_inode of the "events" directory */
@@ -643,128 +639,87 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,returnret;}-structdentry_list{-void*cursor;-structdentry**dentries;-};--/**-*eventfs_release-calledtoreleaseeventfsfile/dir-*@inode:inodetobereleased-*@file:filetobereleased(notused)-*/-staticinteventfs_release(structinode*inode,structfile*file)-{-structtracefs_inode*ti;-structdentry_list*dlist=file->private_data;-void*cursor;-inti;--ti=get_tracefs(inode);-if(!(ti->flags&TRACEFS_EVENT_INODE))-return-EINVAL;--if(WARN_ON_ONCE(!dlist))-return-EINVAL;--for(i=0;dlist->dentries&&dlist->dentries[i];i++){-dput(dlist->dentries[i]);-}--cursor=dlist->cursor;-kfree(dlist->dentries);-kfree(dlist);-file->private_data=cursor;-returndcache_dir_close(inode,file);-}--staticintadd_dentries(structdentry***dentries,structdentry*d,intcnt)-{-structdentry**tmp;--tmp=krealloc(*dentries,sizeof(d)*(cnt+2),GFP_NOFS);-if(!tmp)-return-1;-tmp[cnt]=d;-tmp[cnt+1]=NULL;-*dentries=tmp;-return0;-}--/**-*dcache_dir_open_wrapper-eventfsopenwrapper-*@inode:notused-*@file:dirtobeopened(tocreateit'schildren)-*-*Usedtodynamiccreatefile/dirwith-in@file,allthe-*file/dirwillbecreated.Ifalreadycreatedthenreferences-*willbeincreased+/*+*Walkthechildrenofaeventfs_inodetofillingetdents().*/-staticintdcache_dir_open_wrapper(structinode*inode,structfile*file)+staticinteventfs_iterate(structfile*file,structdir_context*ctx){conststructfile_operations*fops;+structinode*f_inode=file_inode(file);conststructeventfs_entry*entry;structeventfs_inode*ei_child;structtracefs_inode*ti;structeventfs_inode*ei;-structdentry_list*dlist;-structdentry**dentries=NULL;-structdentry*parent=file_dentry(file);-structdentry*d;-structinode*f_inode=file_inode(file);-constchar*name=parent->d_name.name;+structdentry*ei_dentry=NULL;+structdentry*dentry;+constchar*name;umode_tmode;-void*data;-intcnt=0;intidx;-intret;-inti;-intr;+intret=-EINVAL;+intino;+inti,r,c;++if(!dir_emit_dots(file,ctx))+return0;ti=get_tracefs(f_inode);if(!(ti->flags&TRACEFS_EVENT_INODE))return-EINVAL;-if(WARN_ON_ONCE(file->private_data))-return-EINVAL;+c=ctx->pos-2;idx=srcu_read_lock(&eventfs_srcu);mutex_lock(&eventfs_mutex);ei=READ_ONCE(ti->private);+if(ei&&!ei->is_freed)+ei_dentry=READ_ONCE(ei->dentry);mutex_unlock(&eventfs_mutex);-if(!ei){-srcu_read_unlock(&eventfs_srcu,idx);-return-EINVAL;-}---data=ei->data;+if(!ei||!ei_dentry)+gotoout;-dlist=kmalloc(sizeof(*dlist),GFP_KERNEL);-if(!dlist){-srcu_read_unlock(&eventfs_srcu,idx);-return-ENOMEM;-}+ret=0;-inode_lock(parent->d_inode);+/*+*Needtocreatethedentriesandinodestohaveaconsistent+*inodenumber.+*/list_for_each_entry_srcu(ei_child,&ei->children,list,srcu_read_lock_held(&eventfs_srcu)){-d=create_dir_dentry(ei,ei_child,parent);-if(d){-ret=add_dentries(&dentries,d,cnt);-dput(d);-if(ret<0)-break;-cnt++;++if(c>0){+c--;+continue;}++if(ei_child->is_freed)+continue;++name=ei_child->name;++dentry=create_dir_dentry(ei,ei_child,ei_dentry);+if(!dentry)+gotoout;+ino=dentry->d_inode->i_ino;+dput(dentry);++if(!dir_emit(ctx,name,strlen(name),ino,DT_DIR))+gotoout;+ctx->pos++;}for(i=0;i<ei->nr_entries;i++){-void*cdata=data;+void*cdata=ei->data;++if(c>0){+c--;+continue;+}+entry=&ei->entries[i];name=entry->name;+mutex_lock(&eventfs_mutex);/* If ei->is_freed, then the event itself may be too */if(!ei->is_freed)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:30
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
In order to apply a shortcut to skip over the current ctx->pos
immediately, by using the ei->entries array, the reading of that array
should be first. Moving the array reading before the linked list reading
will make the shortcut change diff nicer to read.
Link: https://lore.kernel.org/all/CAHk-=wiKwDUDv3+jCsv-uacDcHDVTYsXtBR9=6sGM5mqX+DhOg@mail.gmail.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240104220048.333115095@goodmis.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 704f960dbee2f1634f4b4e16f208cb16eaf41c1e)
---
fs/tracefs/event_inode.c | 46 ++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 23 deletions(-)
@@ -762,23 +762,32 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)ctx->pos++;-if(ei_child->is_freed)-continue;+entry=&ei->entries[i];+name=entry->name;-name=ei_child->name;+mutex_lock(&eventfs_mutex);+/* If ei->is_freed then just bail here, nothing more to do */+if(ei->is_freed){+mutex_unlock(&eventfs_mutex);+gotoout_dec;+}+r=entry->callback(name,&mode,&cdata,&fops);+mutex_unlock(&eventfs_mutex);+if(r<=0)+continue;-dentry=create_dir_dentry(ei,ei_child,ei_dentry);+dentry=create_file_dentry(ei,i,ei_dentry,name,mode,cdata,fops);if(!dentry)gotoout_dec;ino=dentry->d_inode->i_ino;dput(dentry);-if(!dir_emit(ctx,name,strlen(name),ino,DT_DIR))+if(!dir_emit(ctx,name,strlen(name),ino,DT_REG))gotoout_dec;}-for(i=0;i<ei->nr_entries;i++){-void*cdata=ei->data;+list_for_each_entry_srcu(ei_child,&ei->children,list,+srcu_read_lock_held(&eventfs_srcu)){if(c>0){c--;
@@ -787,27 +796,18 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)ctx->pos++;-entry=&ei->entries[i];-name=entry->name;--mutex_lock(&eventfs_mutex);-/* If ei->is_freed then just bail here, nothing more to do */-if(ei->is_freed){-mutex_unlock(&eventfs_mutex);-gotoout_dec;-}-r=entry->callback(name,&mode,&cdata,&fops);-mutex_unlock(&eventfs_mutex);-if(r<=0)+if(ei_child->is_freed)continue;-dentry=create_file_dentry(ei,i,ei_dentry,name,mode,cdata,fops);+name=ei_child->name;++dentry=create_dir_dentry(ei,ei_child,ei_dentry);if(!dentry)gotoout_dec;ino=dentry->d_inode->i_ino;dput(dentry);-if(!dir_emit(ctx,name,strlen(name),ino,DT_REG))+if(!dir_emit(ctx,name,strlen(name),ino,DT_DIR))gotoout_dec;}ret=1;
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:30
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Instead of walking the dentries on mount/remount to update the gid values of
all the dentries if a gid option is specified on mount, just update the root
inode. Add .getattr, .setattr, and .permissions on the tracefs inode
operations to update the permissions of the files and directories.
For all files and directories in the top level instance:
/sys/kernel/tracing/*
It will use the root inode as the default permissions. The inode that
represents: /sys/kernel/tracing (or wherever it is mounted).
When an instance is created:
mkdir /sys/kernel/tracing/instance/foo
The directory "foo" and all its files and directories underneath will use
the default of what foo is when it was created. A remount of tracefs will
not affect it.
If a user were to modify the permissions of any file or directory in
tracefs, it will also no longer be modified by a change in ownership of a
remount.
The events directory, if it is in the top level instance, will use the
tracefs root inode as the default ownership for itself and all the files and
directories below it.
For the events directory in an instance ("foo"), it will keep the ownership
of what it was when it was created, and that will be used as the default
ownership for the files and directories beneath it.
Link: https://lore.kernel.org/linux-trace-kernel/CAHk-=wjVdGkjDXBbvLn2wbZnqP4UsH46E3gqJ9m7UG6DpX2+WA@mail.gmail.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240103215016.1e0c9811@gandalf.local.home
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 8186fff7ab649085e2c60d032d9a20a85af1d87c)
---
fs/tracefs/event_inode.c | 79 +++++++++++++++-
fs/tracefs/inode.c | 198 ++++++++++++++++++++++-----------------
fs/tracefs/internal.h | 3 +
3 files changed, 190 insertions(+), 90 deletions(-)
@@ -115,10 +116,17 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,*Theeventsdirectorydentryisneverfreed,unlessits*partofaninstancethatisdeleted.It'sattristhe*defaultforitschildfilesanddirectories.-*Donotupdateit.It'snotusedforitsownmodeorownership+*Donotupdateit.It'snotusedforitsownmodeorownership.*/-if(!ei->is_events)+if(ei->is_events){+/* But it still needs to know if it was modified */+if(iattr->ia_valid&ATTR_UID)+ei->attr.mode|=EVENTFS_SAVE_UID;+if(iattr->ia_valid&ATTR_GID)+ei->attr.mode|=EVENTFS_SAVE_GID;+}else{update_attr(&ei->attr,iattr);+}}else{name=dentry->d_name.name;
@@ -136,9 +144,66 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,returnret;}+staticvoidupdate_top_events_attr(structeventfs_inode*ei,structdentry*dentry)+{+structinode*inode;++/* Only update if the "events" was on the top level */+if(!ei||!(ei->attr.mode&EVENTFS_TOPLEVEL))+return;++/* Get the tracefs root inode. */+inode=d_inode(dentry->d_sb->s_root);+ei->attr.uid=inode->i_uid;+ei->attr.gid=inode->i_gid;+}++staticvoidset_top_events_ownership(structinode*inode)+{+structtracefs_inode*ti=get_tracefs(inode);+structeventfs_inode*ei=ti->private;+structdentry*dentry;++/* The top events directory doesn't get automatically updated */+if(!ei||!ei->is_events||!(ei->attr.mode&EVENTFS_TOPLEVEL))+return;++dentry=ei->dentry;++update_top_events_attr(ei,dentry);++if(!(ei->attr.mode&EVENTFS_SAVE_UID))+inode->i_uid=ei->attr.uid;++if(!(ei->attr.mode&EVENTFS_SAVE_GID))+inode->i_gid=ei->attr.gid;+}++staticinteventfs_get_attr(structmnt_idmap*idmap,+conststructpath*path,structkstat*stat,+u32request_mask,unsignedintflags)+{+structdentry*dentry=path->dentry;+structinode*inode=d_backing_inode(dentry);++set_top_events_ownership(inode);++generic_fillattr(idmap,request_mask,inode,stat);+return0;+}++staticinteventfs_permission(structmnt_idmap*idmap,+structinode*inode,intmask)+{+set_top_events_ownership(inode);+returngeneric_permission(idmap,inode,mask);+}+staticconststructinode_operationseventfs_root_dir_inode_operations={.lookup=eventfs_root_lookup,.setattr=eventfs_set_attr,+.getattr=eventfs_get_attr,+.permission=eventfs_permission,};staticconststructinode_operationseventfs_file_inode_operations={
@@ -887,6 +954,14 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentryuid=d_inode(dentry->d_parent)->i_uid;gid=d_inode(dentry->d_parent)->i_gid;+/*+*Iftheeventsdirectoryisofthetopinstance,thenparent+*isNULL.Settheattr.modetoreflectthisanditspermissionswill+*defaulttothetracefsrootdentry.+*/+if(!parent)+ei->attr.mode=EVENTFS_TOPLEVEL;+/* This is used as the default ownership of the files and directories */ei->attr.uid=uid;ei->attr.gid=gid;
@@ -183,87 +259,6 @@ struct tracefs_fs_info {structtracefs_mount_optsmount_opts;};-staticvoidchange_gid(structdentry*dentry,kgid_tgid)-{-if(!dentry->d_inode)-return;-dentry->d_inode->i_gid=gid;-}--/*-*Takenfromd_walk,butwithoutheneedforhandlingrenames.-*Nothingcanberenamedwhilewalkingthelist,astracefs-*doesnotsupportrenames.Thisisonlycalledwhenmounting-*orremountingthefilesystem,tosetallthefilesto-*thegivengid.-*/-staticvoidset_gid(structdentry*parent,kgid_tgid)-{-structdentry*this_parent;-structlist_head*next;--this_parent=parent;-spin_lock(&this_parent->d_lock);--change_gid(this_parent,gid);-repeat:-next=this_parent->d_subdirs.next;-resume:-while(next!=&this_parent->d_subdirs){-structtracefs_inode*ti;-structlist_head*tmp=next;-structdentry*dentry=list_entry(tmp,structdentry,d_child);-next=tmp->next;--/* Note, getdents() can add a cursor dentry with no inode */-if(!dentry->d_inode)-continue;--spin_lock_nested(&dentry->d_lock,DENTRY_D_LOCK_NESTED);--change_gid(dentry,gid);--/* If this is the events directory, update that too */-ti=get_tracefs(dentry->d_inode);-if(ti&&(ti->flags&TRACEFS_EVENT_INODE))-eventfs_update_gid(dentry,gid);--if(!list_empty(&dentry->d_subdirs)){-spin_unlock(&this_parent->d_lock);-spin_release(&dentry->d_lock.dep_map,_RET_IP_);-this_parent=dentry;-spin_acquire(&this_parent->d_lock.dep_map,0,1,_RET_IP_);-gotorepeat;-}-spin_unlock(&dentry->d_lock);-}-/*-*Alldoneatthislevel...ascendandresumethesearch.-*/-rcu_read_lock();-ascend:-if(this_parent!=parent){-structdentry*child=this_parent;-this_parent=child->d_parent;--spin_unlock(&child->d_lock);-spin_lock(&this_parent->d_lock);--/* go into the first sibling still alive */-do{-next=child->d_child.next;-if(next==&this_parent->d_subdirs)-gotoascend;-child=list_entry(next,structdentry,d_child);-}while(unlikely(child->d_flags&DCACHE_DENTRY_KILLED));-rcu_read_unlock();-gotoresume;-}-rcu_read_unlock();-spin_unlock(&this_parent->d_lock);-return;-}-staticinttracefs_parse_options(char*data,structtracefs_mount_opts*opts){substring_targs[MAX_OPT_ARGS];
@@ -336,10 +331,8 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)if(!remount||opts->opts&BIT(Opt_uid))inode->i_uid=opts->uid;-if(!remount||opts->opts&BIT(Opt_gid)){-/* Set all the group ids to the mount option */-set_gid(sb->s_root,opts->gid);-}+if(!remount||opts->opts&BIT(Opt_gid))+inode->i_gid=opts->gid;return0;}
@@ -573,6 +566,26 @@ struct dentry *eventfs_end_creating(struct dentry *dentry)returndentry;}+/* Find the inode that this will use for default */+staticstructinode*instance_inode(structdentry*parent,structinode*inode)+{+structtracefs_inode*ti;++/* If parent is NULL then use root inode */+if(!parent)+returnd_inode(inode->i_sb->s_root);++/* Find the inode that is flagged as an instance or the root inode */+while(!IS_ROOT(parent)){+ti=get_tracefs(d_inode(parent));+if(ti->flags&TRACEFS_INSTANCE_INODE)+break;+parent=parent->d_parent;+}++returnd_inode(parent);+}+/***tracefs_create_file-createafileinthetracefsfilesystem*@name:apointertoastringcontainingthenameofthefiletocreate.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:31
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>
(cherry picked from commit 53c41052ba3121761e6f62a813961164532a214f)
---
fs/tracefs/event_inode.c | 10 ++++++++++
1 file changed, 10 insertions(+)
@@ -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 *parentinode->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;
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:31
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
As the ei->entries array is fixed for the duration of the eventfs_inode,
it can be used to skip over already read entries in eventfs_iterate().
That is, if ctx->pos is greater than zero, there's no reason in doing the
loop across the ei->entries array for the entries less than ctx->pos.
Instead, start the lookup of the entries at the current ctx->pos.
Link: https://lore.kernel.org/all/CAHk-=wiKwDUDv3+jCsv-uacDcHDVTYsXtBR9=6sGM5mqX+DhOg@mail.gmail.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240104220048.494956957@goodmis.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 1de94b52d5e8d8b32f0252f14fad1f1edc2e71f1)
---
fs/tracefs/event_inode.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
@@ -746,21 +746,15 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)if(!ei||!ei_dentry)gotoout;-ret=0;-/**Needtocreatethedentriesandinodestohaveaconsistent*inodenumber.*/-for(i=0;i<ei->nr_entries;i++){-void*cdata=ei->data;--if(c>0){-c--;-continue;-}+ret=0;-ctx->pos++;+/* Start at 'c' to jump over already read entries */+for(i=c;i<ei->nr_entries;i++,ctx->pos++){+void*cdata=ei->data;entry=&ei->entries[i];name=entry->name;
@@ -769,7 +763,7 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)/* If ei->is_freed then just bail here, nothing more to do */if(ei->is_freed){mutex_unlock(&eventfs_mutex);-gotoout_dec;+gotoout;}r=entry->callback(name,&mode,&cdata,&fops);mutex_unlock(&eventfs_mutex);
@@ -778,14 +772,17 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)dentry=create_file_dentry(ei,i,ei_dentry,name,mode,cdata,fops);if(!dentry)-gotoout_dec;+gotoout;ino=dentry->d_inode->i_ino;dput(dentry);if(!dir_emit(ctx,name,strlen(name),ino,DT_REG))-gotoout_dec;+gotoout;}+/* Subtract the skipped entries above */+c-=min((unsignedint)c,(unsignedint)ei->nr_entries);+list_for_each_entry_srcu(ei_child,&ei->children,list,srcu_read_lock_held(&eventfs_srcu)){
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:31
From: Erick Archer <redacted>
As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.
So, use the purpose specific kcalloc() function instead of the argument
size * count in the kzalloc() function.
[1] https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Link: https://lore.kernel.org/linux-trace-kernel/20240115181658.4562-1-erick.archer@gmx.com
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Link: https://github.com/KSPP/linux/issues/162
Signed-off-by: Erick Archer <redacted>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 1057066009c4325bb1d8430c9274894d0860e7c3)
---
fs/tracefs/event_inode.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -97,7 +97,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,/* Preallocate the children mode array if necessary */if(!(dentry->d_inode->i_mode&S_IFDIR)){if(!ei->entry_attrs){-ei->entry_attrs=kzalloc(sizeof(*ei->entry_attrs)*ei->nr_entries,+ei->entry_attrs=kcalloc(ei->nr_entries,sizeof(*ei->entry_attrs),GFP_NOFS);if(!ei->entry_attrs){ret=-ENOMEM;
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:31
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The original eventfs code added a wrapper around the dcache_readdir open
callback and created all the dentries and inodes at open, and increment
their ref count. A wrapper was added around the dcache_readdir release
function to decrement all the ref counts of those created inodes and
dentries. But this proved to be buggy[1] for when a kprobe was created
during a dir read, it would create a dentry between the open and the
release, and because the release would decrement all ref counts of all
files and directories, that would include the kprobe directory that was
not there to have its ref count incremented in open. This would cause the
ref count to go to negative and later crash the kernel.
To solve this, the dentries and inodes that were created and had their ref
count upped in open needed to be saved. That list needed to be passed from
the open to the release, so that the release would only decrement the ref
counts of the entries that were incremented in the open.
Unfortunately, the dcache_readdir logic was already using the
file->private_data, which is the only field that can be used to pass
information from the open to the release. What was done was the eventfs
created another descriptor that had a void pointer to save the
dcache_readdir pointer, and it wrapped all the callbacks, so that it could
save the list of entries that had their ref counts incremented in the
open, and pass it to the release. The wrapped callbacks would just put
back the dcache_readdir pointer and call the functions it used so it could
still use its data[2].
But Linus had an issue with the "hijacking" of the file->private_data
(unfortunately this discussion was on a security list, so no public link).
Which we finally agreed on doing everything within the iterate_shared
callback and leave the dcache_readdir out of it[3]. All the information
needed for the getents() could be created then.
But this ended up being buggy too[4]. The iterate_shared callback was not
the right place to create the dentries and inodes. Even Christian Brauner
had issues with that[5].
An attempt was to go back to creating the inodes and dentries at
the open, create an array to store the information in the
file->private_data, and pass that information to the other callbacks.[6]
The difference between that and the original method, is that it does not
use dcache_readdir. It also does not up the ref counts of the dentries and
pass them. Instead, it creates an array of a structure that saves the
dentry's name and inode number. That information is used in the
iterate_shared callback, and the array is freed in the dir release. The
dentries and inodes created in the open are not used for the iterate_share
or release callbacks. Just their names and inode numbers.
Linus did not like that either[7] and just wanted to remove the dentries
being created in iterate_shared and use the hard coded inode numbers.
[ All this while Linus enjoyed an unexpected vacation during the merge
window due to lack of power. ]
[1] https://lore.kernel.org/linux-trace-kernel/20230919211804.230edf1e@gandalf.local.home/
[2] https://lore.kernel.org/linux-trace-kernel/20230922163446.1431d4fa@gandalf.local.home/
[3] https://lore.kernel.org/linux-trace-kernel/20240104015435.682218477@goodmis.org/
[4] https://lore.kernel.org/all/202401152142.bfc28861-oliver.sang@intel.com/
[5] https://lore.kernel.org/all/20240111-unzahl-gefegt-433acb8a841d@brauner/
[6] https://lore.kernel.org/all/20240116114711.7e8637be@gandalf.local.home/
[7] https://lore.kernel.org/all/20240116170154.5bf0a250@gandalf.local.home/
Link: https://lore.kernel.org/linux-trace-kernel/20240116211353.573784051@goodmis.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Ajay Kaher <ajay.kaher@broadcom.com>
Fixes: 493ec81a8fb8 ("eventfs: Stop using dcache_readdir() for getdents()")
Reported-by: kernel test robot <redacted>
Closes: https://lore.kernel.org/oe-lkp/202401152142.bfc28861-oliver.sang@intel.com
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 852e46e239ee6db3cd220614cf8bce96e79227c2)
---
fs/tracefs/event_inode.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:32
From: Linus Torvalds <torvalds@linux-foundation.org>
The 'eventfs_update_gid()' function is no longer called, so remove it
(and the helper function it uses).
Link: https://lore.kernel.org/all/CAHk-=wj+DsZZ=2iTUkJ-Nojs9fjYMvPs1NuoM3yK7aTDtJfPYQ@mail.gmail.com/
Fixes: 8186fff7ab64 ("tracefs/eventfs: Use root and instance inodes as default ownership")
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 29142dc92c37d3259a33aef15b03e6ee25b0d188)
---
fs/tracefs/event_inode.c | 38 --------------------------------------
fs/tracefs/internal.h | 1 -
2 files changed, 39 deletions(-)
@@ -281,44 +281,6 @@ static void update_inode_attr(struct dentry *dentry, struct inode *inode,inode->i_gid=attr->gid;}-staticvoidupdate_gid(structeventfs_inode*ei,kgid_tgid,intlevel)-{-structeventfs_inode*ei_child;--/* at most we have events/system/event */-if(WARN_ON_ONCE(level>3))-return;--ei->attr.gid=gid;--if(ei->entry_attrs){-for(inti=0;i<ei->nr_entries;i++){-ei->entry_attrs[i].gid=gid;-}-}--/*-*Onlyeventfs_inodewithdentriesareupdated,makesure-*alleventfs_inodesareupdated.Ifoneofthechildren-*donothaveadentry,thisfunctionmusttraverseit.-*/-list_for_each_entry_srcu(ei_child,&ei->children,list,-srcu_read_lock_held(&eventfs_srcu)){-if(!ei_child->dentry)-update_gid(ei_child,gid,level+1);-}-}--voideventfs_update_gid(structdentry*dentry,kgid_tgid)-{-structeventfs_inode*ei=dentry->d_fsdata;-intidx;--idx=srcu_read_lock(&eventfs_srcu);-update_gid(ei,gid,0);-srcu_read_unlock(&eventfs_srcu,idx);-}-/***create_file-createafileinthetracefsfilesystem*@name:thenameofthefiletocreate.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:32
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The eventfs inodes and directories are allocated when referenced. But this
leaves the issue of keeping consistent inode numbers and the number is
only saved in the inode structure itself. When the inode is no longer
referenced, it can be freed. When the file that the inode was representing
is referenced again, the inode is once again created, but the inode number
needs to be the same as it was before.
Just making the inode numbers the same for all files is fine, but that
does not work with directories. The find command will check for loops via
the inode number and having the same inode number for directories triggers:
# find /sys/kernel/tracing
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'.
[..]
Linus pointed out that the eventfs_inode structure ends with a single
32bit int, and on 64 bit machines, there's likely a 4 byte hole due to
alignment. We can use this hole to store the inode number for the
eventfs_inode. All directories in eventfs are represented by an
eventfs_inode and that data structure can hold its inode number.
That last int was also purposely placed at the end of the structure to
prevent holes from within. Now that there's a 4 byte number to hold the
inode, both the inode number and the last integer can be moved up in the
structure for better cache locality, where the llist and rcu fields can be
moved to the end as they are only used when the eventfs_inode is being
deleted.
Link: https://lore.kernel.org/all/CAMuHMdXKiorg-jiuKoZpfZyDJ3Ynrfb8=X+c7x0Eewxn-YRdCA@mail.gmail.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240122152748.46897388@gandalf.local.home
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Fixes: 53c41052ba31 ("eventfs: Have the inodes all for files and directories all be the same")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Reviewed-by: Kees Cook <redacted>
(cherry picked from commit 834bf76add3e6168038150f162cbccf1fd492a67)
---
fs/tracefs/event_inode.c | 14 +++++++++++---
fs/tracefs/internal.h | 7 ++++---
2 files changed, 15 insertions(+), 6 deletions(-)
@@ -34,7 +34,15 @@ static DEFINE_MUTEX(eventfs_mutex);/* Choose something "unique" ;-) */#define EVENTFS_FILE_INODE_INO 0x12c4e37-#define EVENTFS_DIR_INODE_INO 0x134b2f5++/* Just try to make something consistent and unique */+staticinteventfs_dir_ino(structeventfs_inode*ei)+{+if(!ei->ino)+ei->ino=get_next_ino();++returnei->ino;+}/**Theeventfs_inode(ei)itselfisprotectedbySRCU.Itisreleasedfrom
@@ -396,7 +404,7 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parentinode->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);ti=get_tracefs(inode);ti->flags|=TRACEFS_EVENT_INODE;
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:32
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
eventfs uses the tracefs_inode and assumes that it's already initialized
to zero. That is, it doesn't set fields to zero (like ti->private) after
getting its tracefs_inode. This causes bugs due to stale values.
Just initialize the entire structure to zero on allocation so there isn't
any more surprises.
This is a partial fix to access to ti->private. The assignment still needs
to be made before the dentry is instantiated.
Link: https://lore.kernel.org/linux-trace-kernel/20240131185512.315825944@goodmis.org
Cc: stable@vger.kernel.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>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Reported-by: kernel test robot <redacted>
Closes: https://lore.kernel.org/oe-lkp/202401291043.e62e89dc-oliver.sang@intel.com
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit d81786f53aec14fd4d56263145a0635afbc64617)
---
fs/tracefs/inode.c | 6 ++++--
fs/tracefs/internal.h | 3 ++-
2 files changed, 6 insertions(+), 3 deletions(-)
@@ -779,7 +777,11 @@ static void init_once(void *foo){structtracefs_inode*ti=(structtracefs_inode*)foo;+/* inode_init_once() calls memset() on the vfs_inode portion */inode_init_once(&ti->vfs_inode);++/* Zero out the rest */+memset_after(ti,0,vfs_inode);}staticint__inittracefs_init(void)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:32
From: Linus Torvalds <torvalds@linux-foundation.org>
The tracefs-specific fields in the inode were not initialized before the
inode was exposed to others through the dentry with 'd_instantiate()'.
Move the field initializations up to before the d_instantiate.
Link: https://lore.kernel.org/linux-trace-kernel/20240131185512.478449628@goodmis.org
Cc: stable@vger.kernel.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>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Reported-by: kernel test robot <redacted>
Closes: https://lore.kernel.org/oe-lkp/202401291043.e62e89dc-oliver.sang@intel.com
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 4fa4b010b83fb2f837b5ef79e38072a79e96e4f1)
---
fs/tracefs/event_inode.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
@@ -370,6 +370,8 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parentti=get_tracefs(inode);ti->flags|=TRACEFS_EVENT_INODE;+/* Only directories have ti->private set to an ei, not files */+ti->private=ei;inc_nlink(inode);d_instantiate(dentry,inode);
@@ -515,7 +517,6 @@ create_file_dentry(struct eventfs_inode *ei, int idx,staticvoideventfs_post_create_dir(structeventfs_inode*ei){structeventfs_inode*ei_child;-structtracefs_inode*ti;lockdep_assert_held(&eventfs_mutex);
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:32
From: Linus Torvalds <torvalds@linux-foundation.org>
The eventfs_find_events() code tries to walk up the tree to find the
event directory that a dentry belongs to, in order to then find the
eventfs inode that is associated with that event directory.
However, it uses an odd combination of walking the dentry parent,
looking up the eventfs inode associated with that, and then looking up
the dentry from there. Repeat.
But the code shouldn't have back-pointers to dentries in the first
place, and it should just walk the dentry parenthood chain directly.
Similarly, 'set_top_events_ownership()' looks up the dentry from the
eventfs inode, but the only reason it wants a dentry is to look up the
superblock in order to look up the root dentry.
But it already has the real filesystem inode, which has that same
superblock pointer. So just pass in the superblock pointer using the
information that's already there, instead of looking up extraneous data
that is irrelevant.
Link: https://lore.kernel.org/linux-trace-kernel/202401291043.e62e89dc-oliver.sang@intel.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240131185512.638645365@goodmis.org
Cc: stable@vger.kernel.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>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: c1504e510238 ("eventfs: Implement eventfs dir creation functions")
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 99c001cb617df409dac275a059d6c3f187a2da7a)
---
fs/tracefs/event_inode.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
@@ -156,33 +156,30 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,returnret;}-staticvoidupdate_top_events_attr(structeventfs_inode*ei,structdentry*dentry)+staticvoidupdate_top_events_attr(structeventfs_inode*ei,structsuper_block*sb){-structinode*inode;+structinode*root;/* Only update if the "events" was on the top level */if(!ei||!(ei->attr.mode&EVENTFS_TOPLEVEL))return;/* Get the tracefs root inode. */-inode=d_inode(dentry->d_sb->s_root);-ei->attr.uid=inode->i_uid;-ei->attr.gid=inode->i_gid;+root=d_inode(sb->s_root);+ei->attr.uid=root->i_uid;+ei->attr.gid=root->i_gid;}staticvoidset_top_events_ownership(structinode*inode){structtracefs_inode*ti=get_tracefs(inode);structeventfs_inode*ei=ti->private;-structdentry*dentry;/* The top events directory doesn't get automatically updated */if(!ei||!ei->is_events||!(ei->attr.mode&EVENTFS_TOPLEVEL))return;-dentry=ei->dentry;--update_top_events_attr(ei,dentry);+update_top_events_attr(ei,inode->i_sb);if(!(ei->attr.mode&EVENTFS_SAVE_UID))inode->i_uid=ei->attr.uid;
@@ -235,8 +232,10 @@ static struct eventfs_inode *eventfs_find_events(struct dentry *dentry)mutex_lock(&eventfs_mutex);do{-/* The parent always has an ei, except for events itself */-ei=dentry->d_parent->d_fsdata;+// The parent is stable because we do not do renames+dentry=dentry->d_parent;+// ... and directories always have d_fsdata+ei=dentry->d_fsdata;/**Iftheeiisbeingfreed,theownershipofthechildren
@@ -246,12 +245,11 @@ static struct eventfs_inode *eventfs_find_events(struct dentry *dentry)ei=NULL;break;}--dentry=ei->dentry;+// Walk upwards until you find the events inode}while(!ei->is_events);mutex_unlock(&eventfs_mutex);-update_top_events_attr(ei,dentry);+update_top_events_attr(ei,dentry->d_sb);returnei;}
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:32
From: Linus Torvalds <torvalds@linux-foundation.org>
In order for the dentries to stay up-to-date with the eventfs changes,
just add a 'd_revalidate' function that checks the 'is_freed' bit.
Also, clean up the dentry release to actually use d_release() rather
than the slightly odd d_iput() function. We don't care about the inode,
all we want to do is to get rid of the refcount to the eventfs data
added by dentry->d_fsdata.
It would probably be cleaner to make eventfs its own filesystem, or at
least set its own dentry ops when looking up eventfs files. But as it
is, only eventfs dentries use d_fsdata, so we don't really need to split
these things up by use.
Another thing that might be worth doing is to make all eventfs lookups
mark their dentries as not worth caching. We could do that with
d_delete(), but the DCACHE_DONTCACHE flag would likely be even better.
As it is, the dentries are all freeable, but they only tend to get freed
at memory pressure rather than more proactively. But that's a separate
issue.
Link: https://lore.kernel.org/linux-trace-kernel/202401291043.e62e89dc-oliver.sang@intel.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240131185513.124644253@goodmis.org
Cc: stable@vger.kernel.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>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: c1504e510238 ("eventfs: Implement eventfs dir creation functions")
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 8dce06e98c70a7fcbb4bca7d90faf40522e65c58)
---
fs/tracefs/event_inode.c | 5 ++---
fs/tracefs/inode.c | 27 ++++++++++++++++++---------
fs/tracefs/internal.h | 3 ++-
3 files changed, 22 insertions(+), 13 deletions(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:32
From: Linus Torvalds <torvalds@linux-foundation.org>
The dentry lookup for eventfs files was very broken, and had lots of
signs of the old situation where the filesystem names were all created
statically in the dentry tree, rather than being looked up dynamically
based on the eventfs data structures.
You could see it in the naming - how it claimed to "create" dentries
rather than just look up the dentries that were given it.
You could see it in various nonsensical and very incorrect operations,
like using "simple_lookup()" on the dentries that were passed in, which
only results in those dentries becoming negative dentries. Which meant
that any other lookup would possibly return ENOENT if it saw that
negative dentry before the data was then later filled in.
You could see it in the immense amount of nonsensical code that didn't
actually just do lookups.
Link: https://lore.kernel.org/linux-trace-kernel/202401291043.e62e89dc-oliver.sang@intel.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240131233227.73db55e1@gandalf.local.home
Cc: stable@vger.kernel.org
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ajay Kaher <ajay.kaher@broadcom.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Fixes: c1504e510238 ("eventfs: Implement eventfs dir creation functions")
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 49304c2b93e4f7468b51ef717cbe637981397115)
---
fs/tracefs/event_inode.c | 275 +++++++--------------------------------
fs/tracefs/inode.c | 69 ----------
fs/tracefs/internal.h | 3 -
3 files changed, 50 insertions(+), 297 deletions(-)
@@ -230,7 +230,6 @@ static struct eventfs_inode *eventfs_find_events(struct dentry *dentry){structeventfs_inode*ei;-mutex_lock(&eventfs_mutex);do{// The parent is stable because we do not do renamesdentry=dentry->d_parent;
@@ -247,7 +246,6 @@ static struct eventfs_inode *eventfs_find_events(struct dentry *dentry)}// Walk upwards until you find the events inode}while(!ei->is_events);-mutex_unlock(&eventfs_mutex);update_top_events_attr(ei,dentry->d_sb);
@@ -307,15 +304,9 @@ static struct dentry *create_file(const char *name, umode_t mode,if(WARN_ON_ONCE(!S_ISREG(mode)))returnNULL;-WARN_ON_ONCE(!parent);-dentry=eventfs_start_creating(name,parent);--if(IS_ERR(dentry))-returndentry;-inode=tracefs_get_inode(dentry->d_sb);if(unlikely(!inode))-returneventfs_failed_creating(dentry);+returnERR_PTR(-ENOMEM);/* If the user updated the directory's attributes, use them */update_inode_attr(dentry,inode,attr,mode);
@@ -329,32 +320,29 @@ static struct dentry *create_file(const char *name, umode_t mode,ti=get_tracefs(inode);ti->flags|=TRACEFS_EVENT_INODE;-d_instantiate(dentry,inode);++d_add(dentry,inode);fsnotify_create(dentry->d_parent->d_inode,dentry);-returneventfs_end_creating(dentry);+returndentry;};/**-*create_dir-createadirinthetracefsfilesystem+*lookup_dir_entry-lookupadirinthetracefsfilesystem+*@dentry:thedirectorytolookup*@ei:theeventfs_inodethatrepresentsthedirectorytocreate-*@parent:parentdentryforthisfile.*-*Thisfunctionwillcreateadentryforadirectoryrepresentedby+*Thisfunctionwilllookupadentryforadirectoryrepresentedby*aeventfs_inode.*/-staticstructdentry*create_dir(structeventfs_inode*ei,structdentry*parent)+staticstructdentry*lookup_dir_entry(structdentry*dentry,+structeventfs_inode*pei,structeventfs_inode*ei){structtracefs_inode*ti;-structdentry*dentry;structinode*inode;-dentry=eventfs_start_creating(ei->name,parent);-if(IS_ERR(dentry))-returndentry;-inode=tracefs_get_inode(dentry->d_sb);if(unlikely(!inode))-returneventfs_failed_creating(dentry);+returnERR_PTR(-ENOMEM);/* If the user updated the directory's attributes, use them */update_inode_attr(dentry,inode,&ei->attr,
@@ -371,11 +359,14 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent/* Only directories have ti->private set to an ei, not files */ti->private=ei;+dentry->d_fsdata=ei;+ei->dentry=dentry;// Remove me!+inc_nlink(inode);-d_instantiate(dentry,inode);+d_add(dentry,inode);inc_nlink(dentry->d_parent->d_inode);fsnotify_mkdir(dentry->d_parent->d_inode,dentry);-returneventfs_end_creating(dentry);+returndentry;}staticvoidfree_ei(structeventfs_inode*ei)
@@ -438,157 +429,21 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry)*addresslocatedat@e_dentry.*/staticstructdentry*-create_file_dentry(structeventfs_inode*ei,intidx,-structdentry*parent,constchar*name,umode_tmode,void*data,+lookup_file_dentry(structdentry*dentry,+structeventfs_inode*ei,intidx,+umode_tmode,void*data,conststructfile_operations*fops){structeventfs_attr*attr=NULL;structdentry**e_dentry=&ei->d_children[idx];-structdentry*dentry;--WARN_ON_ONCE(!inode_is_locked(parent->d_inode));-mutex_lock(&eventfs_mutex);-if(ei->is_freed){-mutex_unlock(&eventfs_mutex);-returnNULL;-}-/* If the e_dentry already has a dentry, use it */-if(*e_dentry){-dget(*e_dentry);-mutex_unlock(&eventfs_mutex);-return*e_dentry;-}--/* ei->entry_attrs are protected by SRCU */if(ei->entry_attrs)attr=&ei->entry_attrs[idx];-mutex_unlock(&eventfs_mutex);--dentry=create_file(name,mode,attr,parent,data,fops);+dentry->d_fsdata=ei;// NOTE: ei of _parent_+lookup_file(dentry,mode,attr,data,fops);-mutex_lock(&eventfs_mutex);--if(IS_ERR_OR_NULL(dentry)){-/*-*Whenthemutexwasreleased,somethingelsecouldhave-*createdthedentryforthise_dentry.Inwhichcase-*usethatone.-*-*Ifei->is_freedisset,thee_dentryiscurrentlyonits-*waytobeingfreed,don'treturnit.Ife_dentryisNULL-*itmeansitwasalreadyfreed.-*/-if(ei->is_freed){-dentry=NULL;-}else{-dentry=*e_dentry;-dget(dentry);-}-mutex_unlock(&eventfs_mutex);-returndentry;-}--if(!*e_dentry&&!ei->is_freed){-*e_dentry=dentry;-dentry->d_fsdata=ei;-}else{-/*-*Shouldneverhappenunlesswegethereduetobeingfreed.-*Otherwiseitmeanstwodentriesexistwiththesamename.-*/-WARN_ON_ONCE(!ei->is_freed);-dentry=NULL;-}-mutex_unlock(&eventfs_mutex);--returndentry;-}--/**-*eventfs_post_create_dir-postcreatedirroutine-*@ei:eventfs_inodeofrecentlycreateddir-*-*Mapthemeta-dataoffileswithinaneventfsdirtotheirparentdentry-*/-staticvoideventfs_post_create_dir(structeventfs_inode*ei)-{-structeventfs_inode*ei_child;--lockdep_assert_held(&eventfs_mutex);--/* srcu lock already held */-/* fill parent-child relation */-list_for_each_entry_srcu(ei_child,&ei->children,list,-srcu_read_lock_held(&eventfs_srcu)){-ei_child->d_parent=ei->dentry;-}-}--/**-*create_dir_dentry-Createadirectorydentryfortheeventfs_inode-*@pei:Theeventfs_inodeparentofei.-*@ei:Theeventfs_inodetocreatethedirectoryfor-*@parent:Thedentryoftheparentofthisdirectory-*-*Thiscreatesandattachesadirectorydentrytotheeventfs_inode@ei.-*/-staticstructdentry*-create_dir_dentry(structeventfs_inode*pei,structeventfs_inode*ei,-structdentry*parent)-{-structdentry*dentry=NULL;--WARN_ON_ONCE(!inode_is_locked(parent->d_inode));--mutex_lock(&eventfs_mutex);-if(pei->is_freed||ei->is_freed){-mutex_unlock(&eventfs_mutex);-returnNULL;-}-if(ei->dentry){-/* If the eventfs_inode already has a dentry, use it */-dentry=ei->dentry;-dget(dentry);-mutex_unlock(&eventfs_mutex);-returndentry;-}-mutex_unlock(&eventfs_mutex);--dentry=create_dir(ei,parent);--mutex_lock(&eventfs_mutex);--if(IS_ERR_OR_NULL(dentry)&&!ei->is_freed){-/*-*Whenthemutexwasreleased,somethingelsecouldhave-*createdthedentryforthise_dentry.Inwhichcase-*usethatone.-*-*Ifei->is_freedisset,thee_dentryiscurrentlyonits-*waytobeingfreed.-*/-dentry=ei->dentry;-if(dentry)-dget(dentry);-mutex_unlock(&eventfs_mutex);-returndentry;-}--if(!ei->dentry&&!ei->is_freed){-ei->dentry=dentry;-eventfs_post_create_dir(ei);-dentry->d_fsdata=ei;-}else{-/*-*Shouldneverhappenunlesswegethereduetobeingfreed.-*Otherwiseitmeanstwodentriesexistwiththesamename.-*/-WARN_ON_ONCE(!ei->is_freed);-dentry=NULL;-}-mutex_unlock(&eventfs_mutex);+*e_dentry=dentry;// Remove mereturndentry;}
@@ -607,79 +462,49 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,structdentry*dentry,unsignedintflags){-conststructfile_operations*fops;-conststructeventfs_entry*entry;structeventfs_inode*ei_child;structtracefs_inode*ti;structeventfs_inode*ei;-structdentry*ei_dentry=NULL;-structdentry*ret=NULL;-structdentry*d;constchar*name=dentry->d_name.name;-umode_tmode;-void*data;-intidx;-inti;-intr;ti=get_tracefs(dir);if(!(ti->flags&TRACEFS_EVENT_INODE))-returnNULL;--/* Grab srcu to prevent the ei from going away */-idx=srcu_read_lock(&eventfs_srcu);+returnERR_PTR(-EIO);-/*-*Grabtheeventfs_mutextoconsistentvaluefromti->private.-*Thiss-*/mutex_lock(&eventfs_mutex);-ei=READ_ONCE(ti->private);-if(ei&&!ei->is_freed)-ei_dentry=READ_ONCE(ei->dentry);-mutex_unlock(&eventfs_mutex);-if(!ei||!ei_dentry)+ei=ti->private;+if(!ei||ei->is_freed)gotoout;-data=ei->data;--list_for_each_entry_srcu(ei_child,&ei->children,list,-srcu_read_lock_held(&eventfs_srcu)){+list_for_each_entry(ei_child,&ei->children,list){if(strcmp(ei_child->name,name)!=0)continue;-ret=simple_lookup(dir,dentry,flags);-if(IS_ERR(ret))+if(ei_child->is_freed)gotoout;-d=create_dir_dentry(ei,ei_child,ei_dentry);-dput(d);+lookup_dir_entry(dentry,ei,ei_child);gotoout;}-for(i=0;i<ei->nr_entries;i++){-entry=&ei->entries[i];-if(strcmp(name,entry->name)==0){-void*cdata=data;-mutex_lock(&eventfs_mutex);-/* If ei->is_freed, then the event itself may be too */-if(!ei->is_freed)-r=entry->callback(name,&mode,&cdata,&fops);-else-r=-1;-mutex_unlock(&eventfs_mutex);-if(r<=0)-continue;-ret=simple_lookup(dir,dentry,flags);-if(IS_ERR(ret))-gotoout;-d=create_file_dentry(ei,i,ei_dentry,name,mode,cdata,fops);-dput(d);-break;-}+for(inti=0;i<ei->nr_entries;i++){+void*data;+umode_tmode;+conststructfile_operations*fops;+conststructeventfs_entry*entry=&ei->entries[i];++if(strcmp(name,entry->name)!=0)+continue;++data=ei->data;+if(entry->callback(name,&mode,&data,&fops)<=0)+gotoout;++lookup_file_dentry(dentry,ei,i,mode,data,fops);+gotoout;}out:-srcu_read_unlock(&eventfs_srcu,idx);-returnret;+mutex_unlock(&eventfs_mutex);+returnNULL;}/*
@@ -495,75 +495,6 @@ struct dentry *tracefs_end_creating(struct dentry *dentry)returndentry;}-/**-*eventfs_start_creating-starttheprocessofcreatingadentry-*@name:Nameofthefilecreatedforthedentry-*@parent:Theparentdentrywherethisdentrywillbecreated-*-*Thisisasimplehelperfunctionforthedynamicallycreatedeventfs-*files.Whenthedirectoryoftheeventfsfilesareaccessed,their-*dentriesarecreatedonthefly.Thisfunctionisusedtostartthat-*process.-*/-structdentry*eventfs_start_creating(constchar*name,structdentry*parent)-{-structdentry*dentry;-interror;--/* Must always have a parent. */-if(WARN_ON_ONCE(!parent))-returnERR_PTR(-EINVAL);--error=simple_pin_fs(&trace_fs_type,&tracefs_mount,-&tracefs_mount_count);-if(error)-returnERR_PTR(error);--if(unlikely(IS_DEADDIR(parent->d_inode)))-dentry=ERR_PTR(-ENOENT);-else-dentry=lookup_one_len(name,parent,strlen(name));--if(!IS_ERR(dentry)&&dentry->d_inode){-dput(dentry);-dentry=ERR_PTR(-EEXIST);-}--if(IS_ERR(dentry))-simple_release_fs(&tracefs_mount,&tracefs_mount_count);--returndentry;-}--/**-*eventfs_failed_creating-cleanupafailedeventfsdentrycreation-*@dentry:Thedentrytocleanup-*-*Ifaftercallingeventfs_start_creating(),afailureisdetected,the-*resourcescreatedbyeventfs_start_creating()needstobecleanedup.In-*thatcase,thisfunctionshouldbecalledtoperformthatcleanup.-*/-structdentry*eventfs_failed_creating(structdentry*dentry)-{-dput(dentry);-simple_release_fs(&tracefs_mount,&tracefs_mount_count);-returnNULL;-}--/**-*eventfs_end_creating-Finishtheprocessofcreatingaeventfsdentry-*@dentry:Thedentrythathassuccessfullybeencreated.-*-*Thisfunctioniscurrentlyjustaplaceholdertomatch-*eventfs_start_creating().Incaseanysynchronizationneedstobeadded,-*thisfunctionwillbeusedtoimplementthatwithouthavingtomodify-*thecallersofeventfs_start_creating().-*/-structdentry*eventfs_end_creating(structdentry*dentry)-{-returndentry;-}-/* Find the inode that this will use for default */staticstructinode*instance_inode(structdentry*parent,structinode*inode){
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:33
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Some of the eventfs_inode structure has holes in it. Rework the structure
to be a bit more condensed, and also remove the no longer used llist
field.
Link: https://lore.kernel.org/linux-trace-kernel/20240201161617.002321438@goodmis.org
Cc: Linus Torvalds <torvalds@linux-foundation.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>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 264424dfdd5cbd92bc5b5ddf93944929fc877fac)
---
fs/tracefs/internal.h | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:33
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
There should never be a case where an evenfs_inode is being freed without
is_freed being set. Add a WARN_ON_ONCE() if it ever happens. That would
mean there was one too many put_ei()s.
Link: https://lore.kernel.org/linux-trace-kernel/20240201161616.843551963@goodmis.org
Cc: Linus Torvalds <torvalds@linux-foundation.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>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 5a49f996046ba947466bc7461e4b19c4d1daf978)
---
fs/tracefs/event_inode.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:33
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The dentries and inodes are created when referenced in the lookup code.
There's no reason to call fsnotify_*() functions when they are created by
a reference. It doesn't make any sense.
Link: https://lore.kernel.org/linux-trace-kernel/20240201002719.GS2087318@ZenIV/
Link: https://lore.kernel.org/linux-trace-kernel/20240201161617.166973329@goodmis.org
Cc: stable@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.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>
Fixes: a376007917776 ("eventfs: Implement functions to create files and dirs when accessed");
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 12d823b31fadf47c8f36ecada7abac5f903cac33)
---
fs/tracefs/event_inode.c | 2 --
1 file changed, 2 deletions(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:33
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The directory link count in eventfs was somewhat bogus. It was only being
updated when a directory child was being looked up and not on creation.
One solution would be to update in get_attr() the link count by iterating
the ei->children list and then adding 2. But that could slow down simple
stat() calls, especially if it's done on all directories in eventfs.
Another solution would be to add a parent pointer to the eventfs_inode
and keep track of the number of sub directories it has on creation. But
this adds overhead for something not really worthwhile.
The solution decided upon is to keep all directory links in eventfs as 1.
This tells user space not to rely on the hard links of directories. Which
in this case it shouldn't.
Link: https://lore.kernel.org/linux-trace-kernel/20240201002719.GS2087318@ZenIV/
Link: https://lore.kernel.org/linux-trace-kernel/20240201161617.339968298@goodmis.org
Cc: stable@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.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>
Fixes: c1504e510238 ("eventfs: Implement eventfs dir creation functions")
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit ca185770db914869ff9fe773bac5e0e5e4165b83)
---
fs/tracefs/event_inode.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
@@ -769,9 +767,17 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentrydentry->d_fsdata=get_ei(ei);-/* directory inodes start off with i_nlink == 2 (for "." entry) */-inc_nlink(inode);+/*+*Keepalleventfsdirectorieswithi_nlink==1.+*Duetothedynamicnatureofthedentrycreationsandnot+*wantingtoaddapointertotheparenteventfs_inodeinthe+*eventfs_inodestructure,keepingthei_nlinkinsyncwiththe+*numberofdirectorieswouldcausetoomuchcomplexityfor+*somethingnotworthmuch.Keepingdirectorylinksat1+*tellsuserspacenottotrustthelinknumber.+*/d_instantiate(dentry,inode);+/* The dentry of the "events" parent does keep track though */inc_nlink(dentry->d_parent->d_inode);fsnotify_mkdir(dentry->d_parent->d_inode,dentry);tracefs_end_creating(dentry);
From: Steven Rostedt <rostedt@goodmis.org> Date: 2024-02-06 11:33:33
From: Linus Torvalds <torvalds@linux-foundation.org>
The eventfs inode had pointers to dentries (and child dentries) without
actually holding a refcount on said pointer. That is fundamentally
broken, and while eventfs tried to then maintain coherence with dentries
going away by hooking into the '.d_iput' callback, that doesn't actually
work since it's not ordered wrt lookups.
There were two reasonms why eventfs tried to keep a pointer to a dentry:
- the creation of a 'events' directory would actually have a stable
dentry pointer that it created with tracefs_start_creating().
And it needed that dentry when tearing it all down again in
eventfs_remove_events_dir().
This use is actually ok, because the special top-level events
directory dentries are actually stable, not just a temporary cache of
the eventfs data structures.
- the 'eventfs_inode' (aka ei) needs to stay around as long as there
are dentries that refer to it.
It then used these dentry pointers as a replacement for doing
reference counting: it would try to make sure that there was only
ever one dentry associated with an event_inode, and keep a child
dentry array around to see which dentries might still refer to the
parent ei.
This gets rid of the invalid dentry pointer use, and renames the one
valid case to a different name to make it clear that it's not just any
random dentry.
The magic child dentry array that is kind of a "reverse reference list"
is simply replaced by having child dentries take a ref to the ei. As
does the directory dentries. That makes the broken use case go away.
Link: https://lore.kernel.org/linux-trace-kernel/202401291043.e62e89dc-oliver.sang@intel.com/
Link: https://lore.kernel.org/linux-trace-kernel/20240131185513.280463000@goodmis.org
Cc: stable@vger.kernel.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>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: c1504e510238 ("eventfs: Implement eventfs dir creation functions")
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
(cherry picked from commit 43aa6f97c2d03a52c1ddb86768575fc84344bdbb)
---
fs/tracefs/event_inode.c | 248 ++++++++++++---------------------------
fs/tracefs/internal.h | 7 +-
2 files changed, 78 insertions(+), 177 deletions(-)
@@ -321,9 +351,12 @@ static struct dentry *lookup_file(struct dentry *dentry,ti=get_tracefs(inode);ti->flags|=TRACEFS_EVENT_INODE;+// Files have their parent's ei as their fsdata+dentry->d_fsdata=get_ei(parent_ei);+d_add(dentry,inode);fsnotify_create(dentry->d_parent->d_inode,dentry);-returndentry;+returnNULL;};/**
@@ -359,22 +392,29 @@ static struct dentry *lookup_dir_entry(struct dentry *dentry,/* Only directories have ti->private set to an ei, not files */ti->private=ei;-dentry->d_fsdata=ei;-ei->dentry=dentry;// Remove me!+dentry->d_fsdata=get_ei(ei);inc_nlink(inode);d_add(dentry,inode);inc_nlink(dentry->d_parent->d_inode);fsnotify_mkdir(dentry->d_parent->d_inode,dentry);-returndentry;+returnNULL;}-staticvoidfree_ei(structeventfs_inode*ei)+staticinlinestructeventfs_inode*alloc_ei(constchar*name){-kfree_const(ei->name);-kfree(ei->d_children);-kfree(ei->entry_attrs);-kfree(ei);+structeventfs_inode*ei=kzalloc(sizeof(*ei),GFP_KERNEL);++if(!ei)+returnNULL;++ei->name=kstrdup_const(name,GFP_KERNEL);+if(!ei->name){+kfree(ei);+returnNULL;+}+kref_init(&ei->kref);+returnei;}/**
@@ -385,39 +425,13 @@ static void free_ei(struct eventfs_inode *ei)*/voideventfs_d_release(structdentry*dentry){-structeventfs_inode*ei;-inti;--mutex_lock(&eventfs_mutex);--ei=dentry->d_fsdata;-if(!ei)-gotoout;--/* This could belong to one of the files of the ei */-if(ei->dentry!=dentry){-for(i=0;i<ei->nr_entries;i++){-if(ei->d_children[i]==dentry)-break;-}-if(WARN_ON_ONCE(i==ei->nr_entries))-gotoout;-ei->d_children[i]=NULL;-}elseif(ei->is_freed){-free_ei(ei);-}else{-ei->dentry=NULL;-}--dentry->d_fsdata=NULL;-out:-mutex_unlock(&eventfs_mutex);+put_ei(dentry->d_fsdata);}/***lookup_file_dentry-createadentryforafileofaneventfs_inode*@ei:theeventfs_inodethatthefilewillbecreatedunder-*@idx:theindexintothed_children[]ofthe@ei+*@idx:theindexintotheentry_attrs[]ofthe@ei*@parent:Theparentdentryofthecreatedfile.*@name:Thenameofthefiletocreate*@mode:Themodeofthefile.
@@ -434,17 +448,11 @@ lookup_file_dentry(struct dentry *dentry,conststructfile_operations*fops){structeventfs_attr*attr=NULL;-structdentry**e_dentry=&ei->d_children[idx];if(ei->entry_attrs)attr=&ei->entry_attrs[idx];-dentry->d_fsdata=ei;// NOTE: ei of _parent_-lookup_file(dentry,mode,attr,data,fops);--*e_dentry=dentry;// Remove me--returndentry;+returnlookup_file(ei,dentry,mode,attr,data,fops);}/**
@@ -685,7 +679,7 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode/* Was the parent freed? */if(list_empty(&ei->list)){-free_ei(ei);+put_ei(ei);ei=NULL;}returnei;
@@ -720,28 +714,20 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentryif(IS_ERR(dentry))returnERR_CAST(dentry);-ei=kzalloc(sizeof(*ei),GFP_KERNEL);+ei=alloc_ei(name);if(!ei)-gotofail_ei;+gotofail;inode=tracefs_get_inode(dentry->d_sb);if(unlikely(!inode))gotofail;-if(size){-ei->d_children=kcalloc(size,sizeof(*ei->d_children),GFP_KERNEL);-if(!ei->d_children)-gotofail;-}--ei->dentry=dentry;+// Note: we have a ref to the dentry from tracefs_start_creating()+ei->events_dir=dentry;ei->entries=entries;ei->nr_entries=size;ei->is_events=1;ei->data=data;-ei->name=kstrdup_const(name,GFP_KERNEL);-if(!ei->name)-gotofail;/* Save the ownership of this directory */uid=d_inode(dentry->d_parent)->i_uid;
@@ -784,72 +770,11 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentryreturnei;fail:-kfree(ei->d_children);-kfree(ei);-fail_ei:+put_ei(ei);tracefs_failed_creating(dentry);returnERR_PTR(-ENOMEM);}-staticLLIST_HEAD(free_list);--staticvoideventfs_workfn(structwork_struct*work)-{-structeventfs_inode*ei,*tmp;-structllist_node*llnode;--llnode=llist_del_all(&free_list);-llist_for_each_entry_safe(ei,tmp,llnode,llist){-/* This dput() matches the dget() from unhook_dentry() */-for(inti=0;i<ei->nr_entries;i++){-if(ei->d_children[i])-dput(ei->d_children[i]);-}-/* This should only get here if it had a dentry */-if(!WARN_ON_ONCE(!ei->dentry))-dput(ei->dentry);-}-}--staticDECLARE_WORK(eventfs_work,eventfs_workfn);--staticvoidfree_rcu_ei(structrcu_head*head)-{-structeventfs_inode*ei=container_of(head,structeventfs_inode,rcu);--if(ei->dentry){-/* Do not free the ei until all references of dentry are gone */-if(llist_add(&ei->llist,&free_list))-queue_work(system_unbound_wq,&eventfs_work);-return;-}--/* If the ei doesn't have a dentry, neither should its children */-for(inti=0;i<ei->nr_entries;i++){-WARN_ON_ONCE(ei->d_children[i]);-}--free_ei(ei);-}--staticvoidunhook_dentry(structdentry*dentry)-{-if(!dentry)-return;-/*-*Needtoaddareferencetothedentrythatisexpectedby-*simple_recursive_removal(),whichwillincludeadput().-*/-dget(dentry);--/*-*Alsoaddareferenceforthedput()ineventfs_workfn().-*Thatisrequiredasthatdput()willfreetheeiafter-*theSRCUgraceperiodisover.-*/-dget(dentry);-}-/***eventfs_remove_rec-removeeventfsdirorfilefromlist*@ei:eventfs_inodetoberemoved.
@@ -875,28 +798,12 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, int level)return;/* search for nested folders or files */-list_for_each_entry_srcu(ei_child,&ei->children,list,-lockdep_is_held(&eventfs_mutex)){-/* Children only have dentry if parent does */-WARN_ON_ONCE(ei_child->dentry&&!ei->dentry);+list_for_each_entry(ei_child,&ei->children,list)eventfs_remove_rec(ei_child,level+1);-}-ei->is_freed=1;--for(inti=0;i<ei->nr_entries;i++){-if(ei->d_children[i]){-/* Children only have dentry if parent does */-WARN_ON_ONCE(!ei->dentry);-unhook_dentry(ei->d_children[i]);-}-}--unhook_dentry(ei->dentry);--list_del_rcu(&ei->list);-call_srcu(&eventfs_srcu,&ei->rcu,free_rcu_ei);+list_del(&ei->list);+put_ei(ei);}/**
On Tue, Feb 06, 2024 at 06:31:58AM -0500, Steven Rostedt wrote:
This is a backport of all the work that lead up to the work that Linus made
on eventfs. I trust Linus's version more so than the versions in 6.6 and
6.7. There may be plenty of hidden issues due to the design.
This is the update for 6.7. It includes Linus's updates as well as all the
patches leading up to them.
I ran these through my full test suite that I use before sending anyting to
Linus, althouh I did not run my "bisect" test that walks through the
patches. The tests were just run on the end result. I'm currently running my
6.6 version through my tests.
This was created with the following command against v6.7.3:
git log --reverse --no-merges --pretty=oneline v6.7..origin/master fs/tracefs/ | cut -d' ' -f1 |
while read a; do if ! git cherry-pick -x $a; then break; fi ; done
Which adds -x to the cherry pick to add the upstream commit SHAs.