From: "Steven Rostedt (Google)" rostedt@goodmis.org

This reverts commit 6abb8c223ce12078a0f2c129656a13338dfe960b.

The eventfs was not designed properly and may have some hidden bugs in it. Linus rewrote it properly and I trust his version more than this one. Revert the backported patches for 6.6 and re-apply all the changes to make it equivalent to Linus's version.

Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org --- fs/tracefs/inode.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index 0292c6a2bed9..891653ba9cf3 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -509,15 +509,20 @@ struct dentry *eventfs_start_creating(const char *name, struct dentry *parent) struct dentry *dentry; int error;

- /* Must always have a parent. */ - if (WARN_ON_ONCE(!parent)) - return ERR_PTR(-EINVAL); - error = simple_pin_fs(&trace_fs_type, &tracefs_mount, &tracefs_mount_count); if (error) return ERR_PTR(error);

+ /* + * If the parent is not specified, we create it in the root. + * We need the root dentry to do this, which is in the super + * block. A pointer to that is in the struct vfsmount that we + * have around. + */ + if (!parent) + parent = tracefs_mount->mnt_root; + if (unlikely(IS_DEADDIR(parent->d_inode))) dentry = ERR_PTR(-ENOENT); else

From: "Steven Rostedt (Google)" rostedt@goodmis.org

This reverts commit 055907ad2c14838c90d63297f7bab8d180a5d844.

The eventfs was not designed properly and may have some hidden bugs in it. Linus rewrote it properly and I trust his version more than this one. Revert the backported patches for 6.6 and re-apply all the changes to make it equivalent to Linus's version.

Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org --- fs/tracefs/event_inode.c | 71 +++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 5fcfb634fec2..7aa92b8ebc51 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -54,10 +54,12 @@ struct eventfs_file { /* * Union - used for deletion * @llist: for calling dput() if needed after RCU + * @del_list: list of eventfs_file to delete * @rcu: eventfs_file to delete in RCU */ union { struct llist_node llist; + struct list_head del_list; struct rcu_head rcu; }; void *data; @@ -274,6 +276,7 @@ static void free_ef(struct eventfs_file *ef) */ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry) { + struct tracefs_inode *ti_parent; struct eventfs_inode *ei; struct eventfs_file *ef;

@@ -294,6 +297,10 @@ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)

mutex_lock(&eventfs_mutex);

+ ti_parent = get_tracefs(dentry->d_parent->d_inode); + if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE)) + goto out; + ef = dentry->d_fsdata; if (!ef) goto out; @@ -866,29 +873,30 @@ static void unhook_dentry(struct dentry *dentry) { if (!dentry) return; - /* - * Need to add a reference to the dentry that is expected by - * simple_recursive_removal(), which will include a dput(). - */ - dget(dentry);

- /* - * Also add a reference for the dput() in eventfs_workfn(). - * That is required as that dput() will free the ei after - * the SRCU grace period is over. - */ + /* Keep the dentry from being freed yet (see eventfs_workfn()) */ dget(dentry); + + dentry->d_fsdata = NULL; + d_invalidate(dentry); + mutex_lock(&eventfs_mutex); + /* dentry should now have at least a single reference */ + WARN_ONCE((int)d_count(dentry) < 1, + "dentry %px (%s) less than one reference (%d) after invalidate\n", + dentry, dentry->d_name.name, d_count(dentry)); + mutex_unlock(&eventfs_mutex); }

/** * eventfs_remove_rec - remove eventfs dir or file from list * @ef: eventfs_file to be removed. + * @head: to create list of eventfs_file to be deleted * @level: to check recursion depth * * The helper function eventfs_remove_rec() is used to clean up and free the * associated data from eventfs for both of the added functions. */ -static void eventfs_remove_rec(struct eventfs_file *ef, int level) +static void eventfs_remove_rec(struct eventfs_file *ef, struct list_head *head, int level) { struct eventfs_file *ef_child;

@@ -908,16 +916,14 @@ static void eventfs_remove_rec(struct eventfs_file *ef, int level) /* search for nested folders or files */ list_for_each_entry_srcu(ef_child, &ef->ei->e_top_files, list, lockdep_is_held(&eventfs_mutex)) { - eventfs_remove_rec(ef_child, level + 1); + eventfs_remove_rec(ef_child, head, level + 1); } }

ef->is_freed = 1;

- unhook_dentry(ef->dentry); - list_del_rcu(&ef->list); - call_srcu(&eventfs_srcu, &ef->rcu, free_rcu_ef); + list_add_tail(&ef->del_list, head); }

/** @@ -928,22 +934,28 @@ static void eventfs_remove_rec(struct eventfs_file *ef, int level) */ void eventfs_remove(struct eventfs_file *ef) { - struct dentry *dentry; + struct eventfs_file *tmp; + LIST_HEAD(ef_del_list);

if (!ef) return;

+ /* + * Move the deleted eventfs_inodes onto the ei_del_list + * which will also set the is_freed value. Note, this has to be + * done under the eventfs_mutex, but the deletions of + * the dentries must be done outside the eventfs_mutex. + * Hence moving them to this temporary list. + */ mutex_lock(&eventfs_mutex); - dentry = ef->dentry; - eventfs_remove_rec(ef, 0); + eventfs_remove_rec(ef, &ef_del_list, 0); mutex_unlock(&eventfs_mutex);

- /* - * If any of the ei children has a dentry, then the ei itself - * must have a dentry. - */ - if (dentry) - simple_recursive_removal(dentry, NULL); + list_for_each_entry_safe(ef, tmp, &ef_del_list, del_list) { + unhook_dentry(ef->dentry); + list_del(&ef->del_list); + call_srcu(&eventfs_srcu, &ef->rcu, free_rcu_ef); + } }

/** @@ -954,8 +966,6 @@ void eventfs_remove(struct eventfs_file *ef) */ void eventfs_remove_events_dir(struct dentry *dentry) { - struct eventfs_file *ef_child; - struct eventfs_inode *ei; struct tracefs_inode *ti;

if (!dentry || !dentry->d_inode) @@ -965,11 +975,6 @@ void eventfs_remove_events_dir(struct dentry *dentry) if (!ti || !(ti->flags & TRACEFS_EVENT_INODE)) return;

- mutex_lock(&eventfs_mutex); - ei = ti->private; - list_for_each_entry_srcu(ef_child, &ei->e_top_files, list, - lockdep_is_held(&eventfs_mutex)) { - eventfs_remove_rec(ef_child, 0); - } - mutex_unlock(&eventfs_mutex); + d_invalidate(dentry); + dput(dentry); }

From: "Steven Rostedt (Google)" rostedt@goodmis.org

This reverts commit 9aaee3eebc91dd9ccebf6b6bc8a5f59d04ef718b.

The eventfs was not designed properly and may have some hidden bugs in it. Linus rewrote it properly and I trust his version more than this one. Revert the backported patches for 6.6 and re-apply all the changes to make it equivalent to Linus's version.

Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org --- fs/tracefs/event_inode.c | 107 ++++++--------------------------------- 1 file changed, 16 insertions(+), 91 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 6a3f7502310c..a64d8fa39e54 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -40,8 +40,6 @@ struct eventfs_inode { * @data: something that the caller will want to get to later on * @is_freed: Flag set if the eventfs is on its way to be freed * @mode: the permission that the file or directory should have - * @uid: saved uid if changed - * @gid: saved gid if changed */ struct eventfs_file { const char *name; @@ -63,22 +61,11 @@ struct eventfs_file { void *data; unsigned int is_freed:1; unsigned int mode:31; - kuid_t uid; - kgid_t gid; };

static DEFINE_MUTEX(eventfs_mutex); DEFINE_STATIC_SRCU(eventfs_srcu);

-/* Mode is unsigned short, use the upper bits for flags */ -enum { - EVENTFS_SAVE_MODE = BIT(16), - EVENTFS_SAVE_UID = BIT(17), - EVENTFS_SAVE_GID = BIT(18), -}; - -#define EVENTFS_MODE_MASK (EVENTFS_SAVE_MODE - 1) - static struct dentry *eventfs_root_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags); @@ -86,54 +73,8 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file); static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx); static int eventfs_release(struct inode *inode, struct file *file);

-static void update_attr(struct eventfs_file *ef, struct iattr *iattr) -{ - unsigned int ia_valid = iattr->ia_valid; - - if (ia_valid & ATTR_MODE) { - ef->mode = (ef->mode & ~EVENTFS_MODE_MASK) | - (iattr->ia_mode & EVENTFS_MODE_MASK) | - EVENTFS_SAVE_MODE; - } - if (ia_valid & ATTR_UID) { - ef->mode |= EVENTFS_SAVE_UID; - ef->uid = iattr->ia_uid; - } - if (ia_valid & ATTR_GID) { - ef->mode |= EVENTFS_SAVE_GID; - ef->gid = iattr->ia_gid; - } -} - -static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry, - struct iattr *iattr) -{ - struct eventfs_file *ef; - int ret; - - mutex_lock(&eventfs_mutex); - ef = dentry->d_fsdata; - /* The LSB is set when the eventfs_inode is being freed */ - if (((unsigned long)ef & 1UL) || ef->is_freed) { - /* Do not allow changes if the event is about to be removed. */ - mutex_unlock(&eventfs_mutex); - return -ENODEV; - } - - ret = simple_setattr(idmap, dentry, iattr); - if (!ret) - update_attr(ef, iattr); - mutex_unlock(&eventfs_mutex); - return ret; -} - static const struct inode_operations eventfs_root_dir_inode_operations = { .lookup = eventfs_root_lookup, - .setattr = eventfs_set_attr, -}; - -static const struct inode_operations eventfs_file_inode_operations = { - .setattr = eventfs_set_attr, };

static const struct file_operations eventfs_file_operations = { @@ -144,20 +85,10 @@ static const struct file_operations eventfs_file_operations = { .release = eventfs_release, };

-static void update_inode_attr(struct inode *inode, struct eventfs_file *ef) -{ - inode->i_mode = ef->mode & EVENTFS_MODE_MASK; - - if (ef->mode & EVENTFS_SAVE_UID) - inode->i_uid = ef->uid; - - if (ef->mode & EVENTFS_SAVE_GID) - inode->i_gid = ef->gid; -} - /** * create_file - create a file in the tracefs filesystem - * @ef: the eventfs_file + * @name: the name of the file to create. + * @mode: the permission that the file should have. * @parent: parent dentry for this file. * @data: something that the caller will want to get to later on. * @fop: struct file_operations that should be used for this file. @@ -173,7 +104,7 @@ static void update_inode_attr(struct inode *inode, struct eventfs_file *ef) * If tracefs is not enabled in the kernel, the value -%ENODEV will be * returned. */ -static struct dentry *create_file(struct eventfs_file *ef, +static struct dentry *create_file(const char *name, umode_t mode, struct dentry *parent, void *data, const struct file_operations *fop) { @@ -181,13 +112,13 @@ static struct dentry *create_file(struct eventfs_file *ef, struct dentry *dentry; struct inode *inode;

- if (!(ef->mode & S_IFMT)) - ef->mode |= S_IFREG; + if (!(mode & S_IFMT)) + mode |= S_IFREG;

- if (WARN_ON_ONCE(!S_ISREG(ef->mode))) + if (WARN_ON_ONCE(!S_ISREG(mode))) return NULL;

- dentry = eventfs_start_creating(ef->name, parent); + dentry = eventfs_start_creating(name, parent);

if (IS_ERR(dentry)) return dentry; @@ -196,10 +127,7 @@ static struct dentry *create_file(struct eventfs_file *ef, if (unlikely(!inode)) return eventfs_failed_creating(dentry);

- /* If the user updated the directory's attributes, use them */ - update_inode_attr(inode, ef); - - inode->i_op = &eventfs_file_inode_operations; + inode->i_mode = mode; inode->i_fop = fop; inode->i_private = data;

@@ -212,7 +140,7 @@ static struct dentry *create_file(struct eventfs_file *ef,

/** * create_dir - create a dir in the tracefs filesystem - * @ei: the eventfs_inode that represents the directory to create + * @name: the name of the file to create. * @parent: parent dentry for this file. * @data: something that the caller will want to get to later on. * @@ -227,14 +155,13 @@ static struct dentry *create_file(struct eventfs_file *ef, * If tracefs is not enabled in the kernel, the value -%ENODEV will be * returned. */ -static struct dentry *create_dir(struct eventfs_file *ef, - struct dentry *parent, void *data) +static struct dentry *create_dir(const char *name, struct dentry *parent, void *data) { struct tracefs_inode *ti; struct dentry *dentry; struct inode *inode;

- dentry = eventfs_start_creating(ef->name, parent); + dentry = eventfs_start_creating(name, parent); if (IS_ERR(dentry)) return dentry;

@@ -242,8 +169,7 @@ static struct dentry *create_dir(struct eventfs_file *ef, if (unlikely(!inode)) return eventfs_failed_creating(dentry);

- update_inode_attr(inode, ef); - + inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO; inode->i_op = &eventfs_root_dir_inode_operations; inode->i_fop = &eventfs_file_operations; inode->i_private = data; @@ -380,9 +306,10 @@ create_dentry(struct eventfs_file *ef, struct dentry *parent, bool lookup) inode_lock(parent->d_inode);

if (ef->ei) - dentry = create_dir(ef, parent, ef->data); + dentry = create_dir(ef->name, parent, ef->data); else - dentry = create_file(ef, parent, ef->data, ef->fop); + dentry = create_file(ef->name, ef->mode, parent, + ef->data, ef->fop);

if (!lookup) inode_unlock(parent->d_inode); @@ -548,7 +475,6 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file) if (d) { struct dentry **tmp;

- tmp = krealloc(dentries, sizeof(d) * (cnt + 2), GFP_KERNEL); if (!tmp) break; @@ -623,14 +549,13 @@ static struct eventfs_file *eventfs_prepare_ef(const char *name, umode_t mode, return ERR_PTR(-ENOMEM); } INIT_LIST_HEAD(&ef->ei->e_top_files); - ef->mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO; } else { ef->ei = NULL; - ef->mode = mode; }

ef->iop = iop; ef->fop = fop; + ef->mode = mode; ef->data = data; return ef; }

From: "Steven Rostedt (Google)" rostedt@goodmis.org

Instead of having a descriptor for every file represented in the eventfs directory, only have the directory itself represented. Change the API to send in a list of entries that represent all the files in the directory (but not other directories). The entry list contains a name and a callback function that will be used to create the files when they are accessed.

struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent, const struct eventfs_entry *entries, int size, void *data);

is used for the top level eventfs directory, and returns an eventfs_inode that will be used by:

struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent, const struct eventfs_entry *entries, int size, void *data);

where both of the above take an array of struct eventfs_entry entries for every file that is in the directory.

The entries are defined by:

typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data, const struct file_operations **fops);

struct eventfs_entry { const char *name; eventfs_callback callback; };

Where the name is the name of the file and the callback gets called when the file is being created. The callback passes in the name (in case the same callback is used for multiple files), a pointer to the mode, data and fops. The data will be pointing to the data that was passed in eventfs_create_dir() or eventfs_create_events_dir() but may be overridden to point to something else, as it will be used to point to the inode->i_private that is created. The information passed back from the callback is used to create the dentry/inode.

If the callback fills the data and the file should be created, it must return a positive number. On zero or negative, the file is ignored.

This logic may also be used as a prototype to convert entire pseudo file systems into just-in-time allocation.

The "show_events_dentry" file has been updated to show the directories, and any files they have.

With just the eventfs_file allocations:

Before after deltas for meminfo (in kB):

MemFree: -14360 MemAvailable: -14260 Buffers: 40 Cached: 24 Active: 44 Inactive: 48 Inactive(anon): 28 Active(file): 44 Inactive(file): 20 Dirty: -4 AnonPages: 28 Mapped: 4 KReclaimable: 132 Slab: 1604 SReclaimable: 132 SUnreclaim: 1472 Committed_AS: 12

Before after deltas for slabinfo:

<slab>: <objects> [ * <size> = <total>]

ext4_inode_cache 27 [* 1184 = 31968 ] extent_status 102 [* 40 = 4080 ] tracefs_inode_cache 144 [* 656 = 94464 ] buffer_head 39 [* 104 = 4056 ] shmem_inode_cache 49 [* 800 = 39200 ] filp -53 [* 256 = -13568 ] dentry 251 [* 192 = 48192 ] lsm_file_cache 277 [* 32 = 8864 ] vm_area_struct -14 [* 184 = -2576 ] trace_event_file 1748 [* 88 = 153824 ] kmalloc-1k 35 [* 1024 = 35840 ] kmalloc-256 49 [* 256 = 12544 ] kmalloc-192 -28 [* 192 = -5376 ] kmalloc-128 -30 [* 128 = -3840 ] kmalloc-96 10581 [* 96 = 1015776 ] kmalloc-64 3056 [* 64 = 195584 ] kmalloc-32 1291 [* 32 = 41312 ] kmalloc-16 2310 [* 16 = 36960 ] kmalloc-8 9216 [* 8 = 73728 ]

Free memory dropped by 14,360 kB Available memory dropped by 14,260 kB Total slab additions in size: 1,771,032 bytes

With this change:

Before after deltas for meminfo (in kB):

MemFree: -12084 MemAvailable: -11976 Buffers: 32 Cached: 32 Active: 72 Inactive: 168 Inactive(anon): 176 Active(file): 72 Inactive(file): -8 Dirty: 24 AnonPages: 196 Mapped: 8 KReclaimable: 148 Slab: 836 SReclaimable: 148 SUnreclaim: 688 Committed_AS: 324

Before after deltas for slabinfo:

<slab>: <objects> [ * <size> = <total>]

tracefs_inode_cache 144 [* 656 = 94464 ] shmem_inode_cache -23 [* 800 = -18400 ] filp -92 [* 256 = -23552 ] dentry 179 [* 192 = 34368 ] lsm_file_cache -3 [* 32 = -96 ] vm_area_struct -13 [* 184 = -2392 ] trace_event_file 1748 [* 88 = 153824 ] kmalloc-1k -49 [* 1024 = -50176 ] kmalloc-256 -27 [* 256 = -6912 ] kmalloc-128 1864 [* 128 = 238592 ] kmalloc-64 4685 [* 64 = 299840 ] kmalloc-32 -72 [* 32 = -2304 ] kmalloc-16 256 [* 16 = 4096 ] total = 721352

Free memory dropped by 12,084 kB Available memory dropped by 11,976 kB Total slab additions in size: 721,352 bytes

That's over 2 MB in savings per instance for free and available memory, and over 1 MB in savings per instance of slab memory.

Link: https://lore.kernel.org/linux-trace-kernel/20231003184059.4924468e@gandalf.l... Link: https://lore.kernel.org/linux-trace-kernel/20231004165007.43d79161@gandalf.l...

Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Mark Rutland mark.rutland@arm.com Cc: Andrew Morton akpm@linux-foundation.org Cc: Ajay Kaher akaher@vmware.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 5790b1fb3d672d9a1fe3881a7181dfdbe741568f) --- fs/tracefs/event_inode.c | 847 ++++++++++++++++++----------------- fs/tracefs/inode.c | 2 +- fs/tracefs/internal.h | 37 +- include/linux/trace_events.h | 2 +- include/linux/tracefs.h | 29 +- kernel/trace/trace.c | 7 +- kernel/trace/trace.h | 4 +- kernel/trace/trace_events.c | 313 +++++++++---- 8 files changed, 705 insertions(+), 536 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 8c8d64e76103..eab18b157ef5 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -2,8 +2,9 @@ /* * event_inode.c - part of tracefs, a pseudo file system for activating tracing * - * Copyright (C) 2020-23 VMware Inc, author: Steven Rostedt (VMware) rostedt@goodmis.org + * Copyright (C) 2020-23 VMware Inc, author: Steven Rostedt rostedt@goodmis.org * Copyright (C) 2020-23 VMware Inc, author: Ajay Kaher akaher@vmware.com + * Copyright (C) 2023 Google, author: Steven Rostedt rostedt@goodmis.org * * eventfs is used to dynamically create inodes and dentries based on the * meta data provided by the tracing system. @@ -23,46 +24,6 @@ #include <linux/delay.h> #include "internal.h"

-struct eventfs_inode { - struct list_head e_top_files; -}; - -/* - * struct eventfs_file - hold the properties of the eventfs files and - * directories. - * @name: the name of the file or directory to create - * @d_parent: holds parent's dentry - * @dentry: once accessed holds dentry - * @list: file or directory to be added to parent directory - * @ei: list of files and directories within directory - * @fop: file_operations for file or directory - * @iop: inode_operations for file or directory - * @data: something that the caller will want to get to later on - * @mode: the permission that the file or directory should have - */ -struct eventfs_file { - const char *name; - struct dentry *d_parent; - struct dentry *dentry; - struct list_head list; - struct eventfs_inode *ei; - const struct file_operations *fop; - const struct inode_operations *iop; - /* - * Union - used for deletion - * @del_list: list of eventfs_file to delete - * @rcu: eventfs_file to delete in RCU - * @is_freed: node is freed if one of the above is set - */ - union { - struct list_head del_list; - struct rcu_head rcu; - unsigned long is_freed; - }; - void *data; - umode_t mode; -}; - static DEFINE_MUTEX(eventfs_mutex); DEFINE_STATIC_SRCU(eventfs_srcu);

@@ -93,16 +54,9 @@ static const struct file_operations eventfs_file_operations = { * @data: something that the caller will want to get to later on. * @fop: struct file_operations that should be used for this file. * - * This is the basic "create a file" function for tracefs. It allows for a - * wide range of flexibility in creating a file. - * - * This function will return a pointer to a dentry if it succeeds. This - * pointer must be passed to the tracefs_remove() function when the file is - * to be removed (no automatic cleanup happens if your module is unloaded, - * you are responsible here.) If an error occurs, %NULL will be returned. - * - * If tracefs is not enabled in the kernel, the value -%ENODEV will be - * returned. + * This function creates a dentry that represents a file in the eventsfs_inode + * directory. The inode.i_private pointer will point to @data in the open() + * call. */ static struct dentry *create_file(const char *name, umode_t mode, struct dentry *parent, void *data, @@ -118,6 +72,7 @@ static struct dentry *create_file(const char *name, umode_t mode, if (WARN_ON_ONCE(!S_ISREG(mode))) return NULL;

+ WARN_ON_ONCE(!parent); dentry = eventfs_start_creating(name, parent);

if (IS_ERR(dentry)) @@ -142,20 +97,11 @@ static struct dentry *create_file(const char *name, umode_t mode, * create_dir - create a dir in the tracefs filesystem * @name: the name of the file to create. * @parent: parent dentry for this file. - * @data: something that the caller will want to get to later on. - * - * This is the basic "create a dir" function for eventfs. It allows for a - * wide range of flexibility in creating a dir. - * - * This function will return a pointer to a dentry if it succeeds. This - * pointer must be passed to the tracefs_remove() function when the file is - * to be removed (no automatic cleanup happens if your module is unloaded, - * you are responsible here.) If an error occurs, %NULL will be returned. * - * If tracefs is not enabled in the kernel, the value -%ENODEV will be - * returned. + * This function will create a dentry for a directory represented by + * a eventfs_inode. */ -static struct dentry *create_dir(const char *name, struct dentry *parent, void *data) +static struct dentry *create_dir(const char *name, struct dentry *parent) { struct tracefs_inode *ti; struct dentry *dentry; @@ -172,7 +118,6 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void * inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO; inode->i_op = &eventfs_root_dir_inode_operations; inode->i_fop = &eventfs_file_operations; - inode->i_private = data;

ti = get_tracefs(inode); ti->flags |= TRACEFS_EVENT_INODE; @@ -185,18 +130,18 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void * }

/** - * eventfs_set_ef_status_free - set the ef->status to free + * eventfs_set_ei_status_free - remove the dentry reference from an eventfs_inode * @ti: the tracefs_inode of the dentry - * @dentry: dentry who's status to be freed + * @dentry: dentry which has the reference to remove. * - * eventfs_set_ef_status_free will be called if no more - * references remain + * Remove the association between a dentry from an eventfs_inode. */ -void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry) +void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) { struct tracefs_inode *ti_parent; + struct eventfs_inode *ei_child, *tmp; struct eventfs_inode *ei; - struct eventfs_file *ef, *tmp; + int i;

/* The top level events directory may be freed by this */ if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) { @@ -207,9 +152,9 @@ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry) ei = ti->private;

/* Record all the top level files */ - list_for_each_entry_srcu(ef, &ei->e_top_files, list, + list_for_each_entry_srcu(ei_child, &ei->children, list, lockdep_is_held(&eventfs_mutex)) { - list_add_tail(&ef->del_list, &ef_del_list); + list_add_tail(&ei_child->del_list, &ef_del_list); }

/* Nothing should access this, but just in case! */ @@ -218,11 +163,13 @@ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry) mutex_unlock(&eventfs_mutex);

/* Now safely free the top level files and their children */ - list_for_each_entry_safe(ef, tmp, &ef_del_list, del_list) { - list_del(&ef->del_list); - eventfs_remove(ef); + list_for_each_entry_safe(ei_child, tmp, &ef_del_list, del_list) { + list_del(&ei_child->del_list); + eventfs_remove_dir(ei_child); }

+ kfree_const(ei->name); + kfree(ei->d_children); kfree(ei); return; } @@ -233,68 +180,162 @@ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry) if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE)) goto out;

- ef = dentry->d_fsdata; - if (!ef) + ei = dentry->d_fsdata; + if (!ei) goto out;

/* - * If ef was freed, then the LSB bit is set for d_fsdata. + * If ei was freed, then the LSB bit is set for d_fsdata. * But this should not happen, as it should still have a * ref count that prevents it. Warn in case it does. */ - if (WARN_ON_ONCE((unsigned long)ef & 1)) + if (WARN_ON_ONCE((unsigned long)ei & 1)) goto out;

+ /* 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)) + goto out; + ei->d_children[i] = NULL; + } else { + ei->dentry = NULL; + } + dentry->d_fsdata = NULL; - ef->dentry = NULL; -out: + out: mutex_unlock(&eventfs_mutex); }

+/** + * create_file_dentry - create a dentry for a file of an eventfs_inode + * @ei: the eventfs_inode that the file will be created under + * @e_dentry: a pointer to the d_children[] of the @ei + * @parent: The parent dentry of the created file. + * @name: The name of the file to create + * @mode: The mode of the file. + * @data: The data to use to set the inode of the file with on open() + * @fops: The fops of the file to be created. + * @lookup: If called by the lookup routine, in which case, dput() the created dentry. + * + * Create a dentry for a file of an eventfs_inode @ei and place it into the + * address located at @e_dentry. If the @e_dentry already has a dentry, then + * just do a dget() on it and return. Otherwise create the dentry and attach it. + */ +static struct dentry * +create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry, + struct dentry *parent, const char *name, umode_t mode, void *data, + const struct file_operations *fops, bool lookup) +{ + struct dentry *dentry; + bool invalidate = false; + + mutex_lock(&eventfs_mutex); + /* 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); + mutex_unlock(&eventfs_mutex); + return *e_dentry; + } + mutex_unlock(&eventfs_mutex); + + /* The lookup already has the parent->d_inode locked */ + if (!lookup) + inode_lock(parent->d_inode); + + dentry = create_file(name, mode, parent, data, fops); + + if (!lookup) + inode_unlock(parent->d_inode); + + mutex_lock(&eventfs_mutex); + + if (IS_ERR_OR_NULL(dentry)) { + /* + * When the mutex was released, something else could have + * created the dentry for this e_dentry. In which case + * use that one. + * + * Note, with the mutex held, the e_dentry cannot have content + * and the ei->is_freed be true at the same time. + */ + WARN_ON_ONCE(ei->is_freed); + dentry = *e_dentry; + /* The lookup does not need to up the dentry refcount */ + if (dentry && !lookup) + dget(dentry); + mutex_unlock(&eventfs_mutex); + return dentry; + } + + if (!*e_dentry && !ei->is_freed) { + *e_dentry = dentry; + dentry->d_fsdata = ei; + } else { + /* + * Should never happen unless we get here due to being freed. + * Otherwise it means two dentries exist with the same name. + */ + WARN_ON_ONCE(!ei->is_freed); + invalidate = true; + } + mutex_unlock(&eventfs_mutex); + + if (invalidate) + d_invalidate(dentry); + + if (lookup || invalidate) + dput(dentry); + + return invalidate ? NULL : dentry; +} + /** * eventfs_post_create_dir - post create dir routine - * @ef: eventfs_file of recently created dir + * @ei: eventfs_inode of recently created dir * * Map the meta-data of files within an eventfs dir to their parent dentry */ -static void eventfs_post_create_dir(struct eventfs_file *ef) +static void eventfs_post_create_dir(struct eventfs_inode *ei) { - struct eventfs_file *ef_child; + struct eventfs_inode *ei_child; struct tracefs_inode *ti;

/* srcu lock already held */ /* fill parent-child relation */ - list_for_each_entry_srcu(ef_child, &ef->ei->e_top_files, list, + list_for_each_entry_srcu(ei_child, &ei->children, list, srcu_read_lock_held(&eventfs_srcu)) { - ef_child->d_parent = ef->dentry; + ei_child->d_parent = ei->dentry; }

- ti = get_tracefs(ef->dentry->d_inode); - ti->private = ef->ei; + ti = get_tracefs(ei->dentry->d_inode); + ti->private = ei; }

/** - * create_dentry - helper function to create dentry - * @ef: eventfs_file of file or directory to create - * @parent: parent dentry - * @lookup: true if called from lookup routine + * create_dir_dentry - Create a directory dentry for the eventfs_inode + * @ei: The eventfs_inode to create the directory for + * @parent: The dentry of the parent of this directory + * @lookup: True if this is called by the lookup code * - * Used to create a dentry for file/dir, executes post dentry creation routine + * This creates and attaches a directory dentry to the eventfs_inode @ei. */ static struct dentry * -create_dentry(struct eventfs_file *ef, struct dentry *parent, bool lookup) +create_dir_dentry(struct eventfs_inode *ei, struct dentry *parent, bool lookup) { bool invalidate = false; - struct dentry *dentry; + struct dentry *dentry = NULL;

mutex_lock(&eventfs_mutex); - if (ef->is_freed) { - mutex_unlock(&eventfs_mutex); - return NULL; - } - if (ef->dentry) { - dentry = ef->dentry; - /* On dir open, up the ref count */ + if (ei->dentry) { + /* If the dentry already has a dentry, use it */ + dentry = ei->dentry; + /* lookup does not need to up the ref count */ if (!lookup) dget(dentry); mutex_unlock(&eventfs_mutex); @@ -302,42 +343,44 @@ create_dentry(struct eventfs_file *ef, struct dentry *parent, bool lookup) } mutex_unlock(&eventfs_mutex);

+ /* The lookup already has the parent->d_inode locked */ if (!lookup) inode_lock(parent->d_inode);

- if (ef->ei) - dentry = create_dir(ef->name, parent, ef->data); - else - dentry = create_file(ef->name, ef->mode, parent, - ef->data, ef->fop); + dentry = create_dir(ei->name, parent);

if (!lookup) inode_unlock(parent->d_inode);

mutex_lock(&eventfs_mutex); - if (IS_ERR_OR_NULL(dentry)) { - /* If the ef was already updated get it */ - dentry = ef->dentry; + + if (IS_ERR_OR_NULL(dentry) && !ei->is_freed) { + /* + * When the mutex was released, something else could have + * created the dentry for this e_dentry. In which case + * use that one. + * + * Note, with the mutex held, the e_dentry cannot have content + * and the ei->is_freed be true at the same time. + */ + dentry = ei->dentry; if (dentry && !lookup) dget(dentry); mutex_unlock(&eventfs_mutex); return dentry; }

- if (!ef->dentry && !ef->is_freed) { - ef->dentry = dentry; - if (ef->ei) - eventfs_post_create_dir(ef); - dentry->d_fsdata = ef; + if (!ei->dentry && !ei->is_freed) { + ei->dentry = dentry; + eventfs_post_create_dir(ei); + dentry->d_fsdata = ei; } else { - /* A race here, should try again (unless freed) */ - invalidate = true; - /* * Should never happen unless we get here due to being freed. * Otherwise it means two dentries exist with the same name. */ - WARN_ON_ONCE(!ef->is_freed); + WARN_ON_ONCE(!ei->is_freed); + invalidate = true; } mutex_unlock(&eventfs_mutex); if (invalidate) @@ -349,50 +392,85 @@ create_dentry(struct eventfs_file *ef, struct dentry *parent, bool lookup) return invalidate ? NULL : dentry; }

-static bool match_event_file(struct eventfs_file *ef, const char *name) -{ - bool ret; - - mutex_lock(&eventfs_mutex); - ret = !ef->is_freed && strcmp(ef->name, name) == 0; - mutex_unlock(&eventfs_mutex); - - return ret; -} - /** * eventfs_root_lookup - lookup routine to create file/dir * @dir: in which a lookup is being done * @dentry: file/dir dentry - * @flags: to pass as flags parameter to simple lookup + * @flags: Just passed to simple_lookup() * - * Used to create a dynamic file/dir within @dir. Use the eventfs_inode - * list of meta data to find the information needed to create the file/dir. + * Used to create dynamic file/dir with-in @dir, search with-in @ei + * list, if @dentry found go ahead and create the file/dir */ + static struct dentry *eventfs_root_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) { + const struct file_operations *fops; + const struct eventfs_entry *entry; + struct eventfs_inode *ei_child; struct tracefs_inode *ti; struct eventfs_inode *ei; - struct eventfs_file *ef; + struct dentry *ei_dentry = NULL; struct dentry *ret = NULL; + const char *name = dentry->d_name.name; + bool created = false; + umode_t mode; + void *data; int idx; + int i; + int r;

ti = get_tracefs(dir); if (!(ti->flags & TRACEFS_EVENT_INODE)) return NULL;

- ei = ti->private; + /* Grab srcu to prevent the ei from going away */ idx = srcu_read_lock(&eventfs_srcu); - list_for_each_entry_srcu(ef, &ei->e_top_files, list, + + /* + * Grab the eventfs_mutex to consistent value from ti->private. + * This s + */ + mutex_lock(&eventfs_mutex); + ei = READ_ONCE(ti->private); + if (ei) + ei_dentry = READ_ONCE(ei->dentry); + mutex_unlock(&eventfs_mutex); + + if (!ei || !ei_dentry) + goto out; + + data = ei->data; + + list_for_each_entry_srcu(ei_child, &ei->children, list, srcu_read_lock_held(&eventfs_srcu)) { - if (!match_event_file(ef, dentry->d_name.name)) + if (strcmp(ei_child->name, name) != 0) continue; ret = simple_lookup(dir, dentry, flags); - create_dentry(ef, ef->d_parent, true); + create_dir_dentry(ei_child, ei_dentry, true); + created = true; break; } + + if (created) + goto out; + + for (i = 0; i < ei->nr_entries; i++) { + entry = &ei->entries[i]; + if (strcmp(name, entry->name) == 0) { + void *cdata = data; + r = entry->callback(name, &mode, &cdata, &fops); + if (r <= 0) + continue; + ret = simple_lookup(dir, dentry, flags); + create_file_dentry(ei, &ei->d_children[i], + ei_dentry, name, mode, cdata, + fops, true); + break; + } + } + out: srcu_read_unlock(&eventfs_srcu, idx); return ret; } @@ -432,29 +510,48 @@ static int eventfs_release(struct inode *inode, struct file *file) return dcache_dir_close(inode, file); }

+static int add_dentries(struct dentry ***dentries, struct dentry *d, int cnt) +{ + struct dentry **tmp; + + tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_KERNEL); + if (!tmp) + return -1; + tmp[cnt] = d; + tmp[cnt + 1] = NULL; + *dentries = tmp; + return 0; +} + /** * dcache_dir_open_wrapper - eventfs open wrapper * @inode: not used - * @file: dir to be opened (to create its child) + * @file: dir to be opened (to create it's children) * - * Used to dynamically create the file/dir within @file. @file is really a - * directory and all the files/dirs of the children within @file will be - * created. If any of the files/dirs have already been created, their - * reference count will be incremented. + * Used to dynamic create file/dir with-in @file, all the + * file/dir will be created. If already created then references + * will be increased */ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file) { + const struct file_operations *fops; + const struct eventfs_entry *entry; + struct eventfs_inode *ei_child; struct tracefs_inode *ti; struct eventfs_inode *ei; - struct eventfs_file *ef; struct dentry_list *dlist; struct dentry **dentries = NULL; - struct dentry *dentry = file_dentry(file); + struct dentry *parent = file_dentry(file); struct dentry *d; struct inode *f_inode = file_inode(file); + const char *name = parent->d_name.name; + umode_t mode; + void *data; int cnt = 0; int idx; int ret; + int i; + int r;

ti = get_tracefs(f_inode); if (!(ti->flags & TRACEFS_EVENT_INODE)) @@ -463,25 +560,51 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file) if (WARN_ON_ONCE(file->private_data)) return -EINVAL;

+ idx = srcu_read_lock(&eventfs_srcu); + + mutex_lock(&eventfs_mutex); + ei = READ_ONCE(ti->private); + mutex_unlock(&eventfs_mutex); + + if (!ei) { + srcu_read_unlock(&eventfs_srcu, idx); + return -EINVAL; + } + + + data = ei->data; + dlist = kmalloc(sizeof(*dlist), GFP_KERNEL); - if (!dlist) + if (!dlist) { + srcu_read_unlock(&eventfs_srcu, idx); return -ENOMEM; + }

- ei = ti->private; - idx = srcu_read_lock(&eventfs_srcu); - list_for_each_entry_srcu(ef, &ei->e_top_files, list, + list_for_each_entry_srcu(ei_child, &ei->children, list, srcu_read_lock_held(&eventfs_srcu)) { - d = create_dentry(ef, dentry, false); + d = create_dir_dentry(ei_child, parent, false); if (d) { - struct dentry **tmp; + ret = add_dentries(&dentries, d, cnt); + if (ret < 0) + break; + cnt++; + } + }

- tmp = krealloc(dentries, sizeof(d) * (cnt + 2), GFP_KERNEL); - if (!tmp) + for (i = 0; i < ei->nr_entries; i++) { + void *cdata = data; + entry = &ei->entries[i]; + name = entry->name; + r = entry->callback(name, &mode, &cdata, &fops); + if (r <= 0) + continue; + d = create_file_dentry(ei, &ei->d_children[i], + parent, name, mode, cdata, fops, false); + if (d) { + ret = add_dentries(&dentries, d, cnt); + if (ret < 0) break; - tmp[cnt] = d; - tmp[cnt + 1] = NULL; cnt++; - dentries = tmp; } } srcu_read_unlock(&eventfs_srcu, idx); @@ -514,63 +637,90 @@ static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx) }

/** - * eventfs_prepare_ef - helper function to prepare eventfs_file - * @name: the name of the file/directory to create. - * @mode: the permission that the file should have. - * @fop: struct file_operations that should be used for this file/directory. - * @iop: struct inode_operations that should be used for this file/directory. - * @data: something that the caller will want to get to later on. The - * inode.i_private pointer will point to this value on the open() call. + * eventfs_create_dir - Create the eventfs_inode for this directory + * @name: The name of the directory to create. + * @parent: The eventfs_inode of the parent directory. + * @entries: A list of entries that represent the files under this directory + * @size: The number of @entries + * @data: The default data to pass to the files (an entry may override it). + * + * This function creates the descriptor to represent a directory in the + * eventfs. This descriptor is an eventfs_inode, and it is returned to be + * used to create other children underneath. + * + * The @entries is an array of eventfs_entry structures which has: + * const char *name + * eventfs_callback callback; + * + * The name is the name of the file, and the callback is a pointer to a function + * that will be called when the file is reference (either by lookup or by + * reading a directory). The callback is of the prototype: * - * This function allocates and fills the eventfs_file structure. + * int callback(const char *name, umode_t *mode, void **data, + * const struct file_operations **fops); + * + * When a file needs to be created, this callback will be called with + * name = the name of the file being created (so that the same callback + * may be used for multiple files). + * mode = a place to set the file's mode + * data = A pointer to @data, and the callback may replace it, which will + * cause the file created to pass the new data to the open() call. + * fops = the fops to use for the created file. */ -static struct eventfs_file *eventfs_prepare_ef(const char *name, umode_t mode, - const struct file_operations *fop, - const struct inode_operations *iop, - void *data) +struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent, + const struct eventfs_entry *entries, + int size, void *data) { - struct eventfs_file *ef; + struct eventfs_inode *ei;

- ef = kzalloc(sizeof(*ef), GFP_KERNEL); - if (!ef) + if (!parent) + return ERR_PTR(-EINVAL); + + ei = kzalloc(sizeof(*ei), GFP_KERNEL); + if (!ei) return ERR_PTR(-ENOMEM);

- ef->name = kstrdup(name, GFP_KERNEL); - if (!ef->name) { - kfree(ef); + ei->name = kstrdup_const(name, GFP_KERNEL); + if (!ei->name) { + kfree(ei); return ERR_PTR(-ENOMEM); }

- if (S_ISDIR(mode)) { - ef->ei = kzalloc(sizeof(*ef->ei), GFP_KERNEL); - if (!ef->ei) { - kfree(ef->name); - kfree(ef); + if (size) { + ei->d_children = kzalloc(sizeof(*ei->d_children) * size, GFP_KERNEL); + if (!ei->d_children) { + kfree_const(ei->name); + kfree(ei); return ERR_PTR(-ENOMEM); } - INIT_LIST_HEAD(&ef->ei->e_top_files); - } else { - ef->ei = NULL; }

- ef->iop = iop; - ef->fop = fop; - ef->mode = mode; - ef->data = data; - return ef; + ei->entries = entries; + ei->nr_entries = size; + ei->data = data; + INIT_LIST_HEAD(&ei->children); + + mutex_lock(&eventfs_mutex); + list_add_tail(&ei->list, &parent->children); + ei->d_parent = parent->dentry; + mutex_unlock(&eventfs_mutex); + + return ei; }

/** - * eventfs_create_events_dir - create the trace event structure - * @name: the name of the directory to create. - * @parent: parent dentry for this file. This should be a directory dentry - * if set. If this parameter is NULL, then the directory will be - * created in the root of the tracefs filesystem. + * eventfs_create_events_dir - create the top level events directory + * @name: The name of the top level directory to create. + * @parent: Parent dentry for this file in the tracefs directory. + * @entries: A list of entries that represent the files under this directory + * @size: The number of @entries + * @data: The default data to pass to the files (an entry may override it). * * This function creates the top of the trace event directory. */ -struct dentry *eventfs_create_events_dir(const char *name, - struct dentry *parent) +struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent, + const struct eventfs_entry *entries, + int size, void *data) { struct dentry *dentry = tracefs_start_creating(name, parent); struct eventfs_inode *ei; @@ -581,19 +731,32 @@ struct dentry *eventfs_create_events_dir(const char *name, return NULL;

if (IS_ERR(dentry)) - return dentry; + return (struct eventfs_inode *)dentry;

ei = kzalloc(sizeof(*ei), GFP_KERNEL); if (!ei) - return ERR_PTR(-ENOMEM); + goto fail; + inode = tracefs_get_inode(dentry->d_sb); - if (unlikely(!inode)) { - kfree(ei); - tracefs_failed_creating(dentry); - return ERR_PTR(-ENOMEM); + if (unlikely(!inode)) + goto fail; + + if (size) { + ei->d_children = kzalloc(sizeof(*ei->d_children) * size, GFP_KERNEL); + if (!ei->d_children) + goto fail; }

- INIT_LIST_HEAD(&ei->e_top_files); + ei->dentry = dentry; + ei->entries = entries; + ei->nr_entries = size; + ei->data = data; + ei->name = kstrdup_const(name, GFP_KERNEL); + if (!ei->name) + goto fail; + + INIT_LIST_HEAD(&ei->children); + INIT_LIST_HEAD(&ei->list);

ti = get_tracefs(inode); ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE; @@ -608,193 +771,41 @@ struct dentry *eventfs_create_events_dir(const char *name, d_instantiate(dentry, inode); inc_nlink(dentry->d_parent->d_inode); fsnotify_mkdir(dentry->d_parent->d_inode, dentry); - return tracefs_end_creating(dentry); -} + tracefs_end_creating(dentry);

-/** - * eventfs_add_subsystem_dir - add eventfs subsystem_dir to list to create later - * @name: the name of the file to create. - * @parent: parent dentry for this dir. - * - * This function adds eventfs subsystem dir to list. - * And all these dirs are created on the fly when they are looked up, - * and the dentry and inodes will be removed when they are done. - */ -struct eventfs_file *eventfs_add_subsystem_dir(const char *name, - struct dentry *parent) -{ - struct tracefs_inode *ti_parent; - struct eventfs_inode *ei_parent; - struct eventfs_file *ef; + /* Will call dput when the directory is removed */ + dget(dentry);

- if (security_locked_down(LOCKDOWN_TRACEFS)) - return NULL; - - if (!parent) - return ERR_PTR(-EINVAL); - - ti_parent = get_tracefs(parent->d_inode); - ei_parent = ti_parent->private; + return ei;

- ef = eventfs_prepare_ef(name, S_IFDIR, NULL, NULL, NULL); - if (IS_ERR(ef)) - return ef; - - mutex_lock(&eventfs_mutex); - list_add_tail(&ef->list, &ei_parent->e_top_files); - ef->d_parent = parent; - mutex_unlock(&eventfs_mutex); - return ef; + fail: + kfree(ei->d_children); + kfree(ei); + tracefs_failed_creating(dentry); + return ERR_PTR(-ENOMEM); }

-/** - * eventfs_add_dir - add eventfs dir to list to create later - * @name: the name of the file to create. - * @ef_parent: parent eventfs_file for this dir. - * - * This function adds eventfs dir to list. - * And all these dirs are created on the fly when they are looked up, - * and the dentry and inodes will be removed when they are done. - */ -struct eventfs_file *eventfs_add_dir(const char *name, - struct eventfs_file *ef_parent) +static void free_ei(struct rcu_head *head) { - struct eventfs_file *ef; - - if (security_locked_down(LOCKDOWN_TRACEFS)) - return NULL; + struct eventfs_inode *ei = container_of(head, struct eventfs_inode, rcu);

- if (!ef_parent) - return ERR_PTR(-EINVAL); - - ef = eventfs_prepare_ef(name, S_IFDIR, NULL, NULL, NULL); - if (IS_ERR(ef)) - return ef; - - mutex_lock(&eventfs_mutex); - list_add_tail(&ef->list, &ef_parent->ei->e_top_files); - ef->d_parent = ef_parent->dentry; - mutex_unlock(&eventfs_mutex); - return ef; -} - -/** - * eventfs_add_events_file - add the data needed to create a file for later reference - * @name: the name of the file to create. - * @mode: the permission that the file should have. - * @parent: parent dentry for this file. - * @data: something that the caller will want to get to later on. - * @fop: struct file_operations that should be used for this file. - * - * This function is used to add the information needed to create a - * dentry/inode within the top level events directory. The file created - * will have the @mode permissions. The @data will be used to fill the - * inode.i_private when the open() call is done. The dentry and inodes are - * all created when they are referenced, and removed when they are no - * longer referenced. - */ -int eventfs_add_events_file(const char *name, umode_t mode, - struct dentry *parent, void *data, - const struct file_operations *fop) -{ - struct tracefs_inode *ti; - struct eventfs_inode *ei; - struct eventfs_file *ef; - - if (security_locked_down(LOCKDOWN_TRACEFS)) - return -ENODEV; - - if (!parent) - return -EINVAL; - - if (!(mode & S_IFMT)) - mode |= S_IFREG; - - if (!parent->d_inode) - return -EINVAL; - - ti = get_tracefs(parent->d_inode); - if (!(ti->flags & TRACEFS_EVENT_INODE)) - return -EINVAL; - - ei = ti->private; - ef = eventfs_prepare_ef(name, mode, fop, NULL, data); - - if (IS_ERR(ef)) - return -ENOMEM; - - mutex_lock(&eventfs_mutex); - list_add_tail(&ef->list, &ei->e_top_files); - ef->d_parent = parent; - mutex_unlock(&eventfs_mutex); - return 0; -} - -/** - * eventfs_add_file - add eventfs file to list to create later - * @name: the name of the file to create. - * @mode: the permission that the file should have. - * @ef_parent: parent eventfs_file for this file. - * @data: something that the caller will want to get to later on. - * @fop: struct file_operations that should be used for this file. - * - * This function is used to add the information needed to create a - * file within a subdirectory of the events directory. The file created - * will have the @mode permissions. The @data will be used to fill the - * inode.i_private when the open() call is done. The dentry and inodes are - * all created when they are referenced, and removed when they are no - * longer referenced. - */ -int eventfs_add_file(const char *name, umode_t mode, - struct eventfs_file *ef_parent, - void *data, - const struct file_operations *fop) -{ - struct eventfs_file *ef; - - if (security_locked_down(LOCKDOWN_TRACEFS)) - return -ENODEV; - - if (!ef_parent) - return -EINVAL; - - if (!(mode & S_IFMT)) - mode |= S_IFREG; - - ef = eventfs_prepare_ef(name, mode, fop, NULL, data); - if (IS_ERR(ef)) - return -ENOMEM; - - mutex_lock(&eventfs_mutex); - list_add_tail(&ef->list, &ef_parent->ei->e_top_files); - ef->d_parent = ef_parent->dentry; - mutex_unlock(&eventfs_mutex); - return 0; -} - -static void free_ef(struct rcu_head *head) -{ - struct eventfs_file *ef = container_of(head, struct eventfs_file, rcu); - - kfree(ef->name); - kfree(ef->ei); - kfree(ef); + kfree_const(ei->name); + kfree(ei->d_children); + kfree(ei); }

/** * eventfs_remove_rec - remove eventfs dir or file from list - * @ef: eventfs_file to be removed. - * @head: to create list of eventfs_file to be deleted - * @level: to check recursion depth + * @ei: eventfs_inode to be removed. * - * The helper function eventfs_remove_rec() is used to clean up and free the - * associated data from eventfs for both of the added functions. + * This function recursively remove eventfs_inode which + * contains info of file or dir. */ -static void eventfs_remove_rec(struct eventfs_file *ef, struct list_head *head, int level) +static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, int level) { - struct eventfs_file *ef_child; + struct eventfs_inode *ei_child;

- if (!ef) + if (!ei) return; /* * Check recursion depth. It should never be greater than 3: @@ -806,62 +817,68 @@ static void eventfs_remove_rec(struct eventfs_file *ef, struct list_head *head, if (WARN_ON_ONCE(level > 3)) return;

- if (ef->ei) { - /* search for nested folders or files */ - list_for_each_entry_srcu(ef_child, &ef->ei->e_top_files, list, - lockdep_is_held(&eventfs_mutex)) { - eventfs_remove_rec(ef_child, head, level + 1); - } + /* search for nested folders or files */ + list_for_each_entry_srcu(ei_child, &ei->children, list, + lockdep_is_held(&eventfs_mutex)) { + eventfs_remove_rec(ei_child, head, level + 1); }

- list_del_rcu(&ef->list); - list_add_tail(&ef->del_list, head); + list_del_rcu(&ei->list); + list_add_tail(&ei->del_list, head); }

+static void unhook_dentry(struct dentry **dentry, struct dentry **list) +{ + if (*dentry) { + unsigned long ptr = (unsigned long)*list; + + /* Keep the dentry from being freed yet */ + dget(*dentry); + + /* + * Paranoid: The dget() above should prevent the dentry + * from being freed and calling eventfs_set_ei_status_free(). + * But just in case, set the link list LSB pointer to 1 + * and have eventfs_set_ei_status_free() check that to + * make sure that if it does happen, it will not think + * the d_fsdata is an eventfs_inode. + * + * For this to work, no eventfs_inode should be allocated + * on a odd space, as the ef should always be allocated + * to be at least word aligned. Check for that too. + */ + WARN_ON_ONCE(ptr & 1); + + (*dentry)->d_fsdata = (void *)(ptr | 1); + *list = *dentry; + *dentry = NULL; + } +} /** * eventfs_remove - remove eventfs dir or file from list - * @ef: eventfs_file to be removed. + * @ei: eventfs_inode to be removed. * * This function acquire the eventfs_mutex lock and call eventfs_remove_rec() */ -void eventfs_remove(struct eventfs_file *ef) +void eventfs_remove_dir(struct eventfs_inode *ei) { - struct eventfs_file *tmp; - LIST_HEAD(ef_del_list); + struct eventfs_inode *tmp; + LIST_HEAD(ei_del_list); struct dentry *dentry_list = NULL; struct dentry *dentry; + int i;

- if (!ef) + if (!ei) return;

mutex_lock(&eventfs_mutex); - eventfs_remove_rec(ef, &ef_del_list, 0); - list_for_each_entry_safe(ef, tmp, &ef_del_list, del_list) { - if (ef->dentry) { - unsigned long ptr = (unsigned long)dentry_list; - - /* Keep the dentry from being freed yet */ - dget(ef->dentry); - - /* - * Paranoid: The dget() above should prevent the dentry - * from being freed and calling eventfs_set_ef_status_free(). - * But just in case, set the link list LSB pointer to 1 - * and have eventfs_set_ef_status_free() check that to - * make sure that if it does happen, it will not think - * the d_fsdata is an event_file. - * - * For this to work, no event_file should be allocated - * on a odd space, as the ef should always be allocated - * to be at least word aligned. Check for that too. - */ - WARN_ON_ONCE(ptr & 1); - - ef->dentry->d_fsdata = (void *)(ptr | 1); - dentry_list = ef->dentry; - ef->dentry = NULL; - } - call_srcu(&eventfs_srcu, &ef->rcu, free_ef); + eventfs_remove_rec(ei, &ei_del_list, 0); + + list_for_each_entry_safe(ei, tmp, &ei_del_list, del_list) { + for (i = 0; i < ei->nr_entries; i++) + unhook_dentry(&ei->d_children[i], &dentry_list); + unhook_dentry(&ei->dentry, &dentry_list); + call_srcu(&eventfs_srcu, &ei->rcu, free_ei); } mutex_unlock(&eventfs_mutex);

@@ -876,8 +893,8 @@ void eventfs_remove(struct eventfs_file *ef) mutex_lock(&eventfs_mutex); /* dentry should now have at least a single reference */ WARN_ONCE((int)d_count(dentry) < 1, - "dentry %p less than one reference (%d) after invalidate\n", - dentry, d_count(dentry)); + "dentry %px (%s) less than one reference (%d) after invalidate\n", + dentry, dentry->d_name.name, d_count(dentry)); mutex_unlock(&eventfs_mutex); dput(dentry); } diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index 891653ba9cf3..34ffb2f8114e 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -385,7 +385,7 @@ static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode)

ti = get_tracefs(inode); if (ti && ti->flags & TRACEFS_EVENT_INODE) - eventfs_set_ef_status_free(ti, dentry); + eventfs_set_ei_status_free(ti, dentry); iput(inode); }

diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 4f2e49e2197b..298d3ecaf621 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -13,6 +13,41 @@ struct tracefs_inode { struct inode vfs_inode; };

+/* + * struct eventfs_inode - hold the properties of the eventfs directories. + * @list: link list into the parent directory + * @entries: the array of entries representing the files in the directory + * @name: the name of the directory to create + * @children: link list into the child eventfs_inode + * @dentry: the dentry of the directory + * @d_parent: pointer to the parent's dentry + * @d_children: The array of dentries to represent the files when created + * @data: The private data to pass to the callbacks + * @nr_entries: The number of items in @entries + */ +struct eventfs_inode { + struct list_head list; + const struct eventfs_entry *entries; + const char *name; + struct list_head children; + struct dentry *dentry; + struct dentry *d_parent; + struct dentry **d_children; + void *data; + /* + * Union - used for deletion + * @del_list: list of eventfs_inode to delete + * @rcu: eventfs_indoe to delete in RCU + * @is_freed: node is freed if one of the above is set + */ + union { + struct list_head del_list; + struct rcu_head rcu; + unsigned long is_freed; + }; + int nr_entries; +}; + static inline struct tracefs_inode *get_tracefs(const struct inode *inode) { return container_of(inode, struct tracefs_inode, vfs_inode); @@ -25,6 +60,6 @@ struct inode *tracefs_get_inode(struct super_block *sb); struct dentry *eventfs_start_creating(const char *name, struct dentry *parent); struct dentry *eventfs_failed_creating(struct dentry *dentry); struct dentry *eventfs_end_creating(struct dentry *dentry); -void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry); +void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry);

#endif /* _TRACEFS_INTERNAL_H */ diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index cf9f0c61796e..696f8dc4aa53 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -652,7 +652,7 @@ struct trace_event_file { struct list_head list; struct trace_event_call *event_call; struct event_filter __rcu *filter; - struct eventfs_file *ef; + struct eventfs_inode *ei; struct trace_array *tr; struct trace_subsystem_dir *system; struct list_head triggers; diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h index 009072792fa3..0c39704455d9 100644 --- a/include/linux/tracefs.h +++ b/include/linux/tracefs.h @@ -23,26 +23,25 @@ struct file_operations;

struct eventfs_file;

-struct dentry *eventfs_create_events_dir(const char *name, - struct dentry *parent); +typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data, + const struct file_operations **fops);

-struct eventfs_file *eventfs_add_subsystem_dir(const char *name, - struct dentry *parent); +struct eventfs_entry { + const char *name; + eventfs_callback callback; +};

-struct eventfs_file *eventfs_add_dir(const char *name, - struct eventfs_file *ef_parent); +struct eventfs_inode;

-int eventfs_add_file(const char *name, umode_t mode, - struct eventfs_file *ef_parent, void *data, - const struct file_operations *fops); +struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent, + const struct eventfs_entry *entries, + int size, void *data);

-int eventfs_add_events_file(const char *name, umode_t mode, - struct dentry *parent, void *data, - const struct file_operations *fops); +struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent, + const struct eventfs_entry *entries, + int size, void *data);

-void eventfs_remove(struct eventfs_file *ef); - -void eventfs_remove_events_dir(struct dentry *dentry); +void eventfs_remove_dir(struct eventfs_inode *ei);

struct dentry *tracefs_create_file(const char *name, umode_t mode, struct dentry *parent, void *data, diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index fc00356a5a0a..33a6ea7145c2 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -9758,7 +9758,6 @@ static __init void create_trace_instances(struct dentry *d_tracer) static void init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer) { - struct trace_event_file *file; int cpu;

trace_create_file("available_tracers", TRACE_MODE_READ, d_tracer, @@ -9791,11 +9790,7 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer) trace_create_file("trace_marker", 0220, d_tracer, tr, &tracing_mark_fops);

- file = __find_event_file(tr, "ftrace", "print"); - if (file && file->ef) - eventfs_add_file("trigger", TRACE_MODE_WRITE, file->ef, - file, &event_trigger_fops); - tr->trace_marker_file = file; + tr->trace_marker_file = __find_event_file(tr, "ftrace", "print");

trace_create_file("trace_marker_raw", 0220, d_tracer, tr, &tracing_mark_raw_fops); diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index 51c0a970339e..02b727a54648 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -381,7 +381,7 @@ struct trace_array { struct dentry *dir; struct dentry *options; struct dentry *percpu_dir; - struct dentry *event_dir; + struct eventfs_inode *event_dir; struct trace_options *topts; struct list_head systems; struct list_head events; @@ -1345,7 +1345,7 @@ struct trace_subsystem_dir { struct list_head list; struct event_subsystem *subsystem; struct trace_array *tr; - struct eventfs_file *ef; + struct eventfs_inode *ei; int ref_count; int nr_events; }; diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index 82cb22ad6d61..6595317c3830 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c @@ -984,7 +984,7 @@ static void remove_subsystem(struct trace_subsystem_dir *dir) return;

if (!--dir->nr_events) { - eventfs_remove(dir->ef); + eventfs_remove_dir(dir->ei); list_del(&dir->list); __put_system_dir(dir); } @@ -1013,7 +1013,7 @@ void event_file_put(struct trace_event_file *file)

static void remove_event_file_dir(struct trace_event_file *file) { - eventfs_remove(file->ef); + eventfs_remove_dir(file->ei); list_del(&file->list); remove_subsystem(file->system); free_event_filter(file->filter); @@ -2302,14 +2302,40 @@ create_new_subsystem(const char *name) return NULL; }

-static struct eventfs_file * +int system_callback(const char *name, umode_t *mode, void **data, + const struct file_operations **fops) +{ + if (strcmp(name, "filter") == 0) + *fops = &ftrace_subsystem_filter_fops; + + else if (strcmp(name, "enable") == 0) + *fops = &ftrace_system_enable_fops; + + else + return 0; + + *mode = TRACE_MODE_WRITE; + return 1; +} + +static struct eventfs_inode * event_subsystem_dir(struct trace_array *tr, const char *name, - struct trace_event_file *file, struct dentry *parent) + struct trace_event_file *file, struct eventfs_inode *parent) { struct event_subsystem *system, *iter; struct trace_subsystem_dir *dir; - struct eventfs_file *ef; - int res; + struct eventfs_inode *ei; + int nr_entries; + static struct eventfs_entry system_entries[] = { + { + .name = "filter", + .callback = system_callback, + }, + { + .name = "enable", + .callback = system_callback, + } + };

/* First see if we did not already create this dir */ list_for_each_entry(dir, &tr->systems, list) { @@ -2317,7 +2343,7 @@ event_subsystem_dir(struct trace_array *tr, const char *name, if (strcmp(system->name, name) == 0) { dir->nr_events++; file->system = dir; - return dir->ef; + return dir->ei; } }

@@ -2341,39 +2367,29 @@ event_subsystem_dir(struct trace_array *tr, const char *name, } else __get_system(system);

- ef = eventfs_add_subsystem_dir(name, parent); - if (IS_ERR(ef)) { + /* ftrace only has directories no files */ + if (strcmp(name, "ftrace") == 0) + nr_entries = 0; + else + nr_entries = ARRAY_SIZE(system_entries); + + ei = eventfs_create_dir(name, parent, system_entries, nr_entries, dir); + if (!ei) { pr_warn("Failed to create system directory %s\n", name); __put_system(system); goto out_free; }

- dir->ef = ef; + dir->ei = ei; dir->tr = tr; dir->ref_count = 1; dir->nr_events = 1; dir->subsystem = system; file->system = dir;

- /* the ftrace system is special, do not create enable or filter files */ - if (strcmp(name, "ftrace") != 0) { - - res = eventfs_add_file("filter", TRACE_MODE_WRITE, - dir->ef, dir, - &ftrace_subsystem_filter_fops); - if (res) { - kfree(system->filter); - system->filter = NULL; - pr_warn("Could not create tracefs '%s/filter' entry\n", name); - } - - eventfs_add_file("enable", TRACE_MODE_WRITE, dir->ef, dir, - &ftrace_system_enable_fops); - } - list_add(&dir->list, &tr->systems);

- return dir->ef; + return dir->ei;

out_free: kfree(dir); @@ -2422,15 +2438,134 @@ event_define_fields(struct trace_event_call *call) return ret; }

+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; + + if (strcmp(name, "format") == 0) { + *mode = TRACE_MODE_READ; + *fops = &ftrace_event_format_fops; + *data = call; + return 1; + } + + /* + * 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; + } + } + +#ifdef CONFIG_PERF_EVENTS + if (call->event.type && call->class->reg && + strcmp(name, "id") == 0) { + *mode = TRACE_MODE_READ; + *data = (void *)(long)call->event.type; + *fops = &ftrace_event_id_fops; + return 1; + } +#endif + +#ifdef CONFIG_HIST_TRIGGERS + if (strcmp(name, "hist") == 0) { + *mode = TRACE_MODE_READ; + *fops = &event_hist_fops; + return 1; + } +#endif +#ifdef CONFIG_HIST_TRIGGERS_DEBUG + if (strcmp(name, "hist_debug") == 0) { + *mode = TRACE_MODE_READ; + *fops = &event_hist_debug_fops; + return 1; + } +#endif +#ifdef CONFIG_TRACE_EVENT_INJECT + if (call->event.type && call->class->reg && + strcmp(name, "inject") == 0) { + *mode = 0200; + *fops = &event_inject_fops; + return 1; + } +#endif + return 0; +} + static int -event_create_dir(struct dentry *parent, struct trace_event_file *file) +event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file) { struct trace_event_call *call = file->event_call; - struct eventfs_file *ef_subsystem = NULL; struct trace_array *tr = file->tr; - struct eventfs_file *ef; + struct eventfs_inode *e_events; + struct eventfs_inode *ei; const char *name; + int nr_entries; int ret; + static struct eventfs_entry event_entries[] = { + { + .name = "enable", + .callback = event_callback, + }, + { + .name = "filter", + .callback = event_callback, + }, + { + .name = "trigger", + .callback = event_callback, + }, + { + .name = "format", + .callback = event_callback, + }, +#ifdef CONFIG_PERF_EVENTS + { + .name = "id", + .callback = event_callback, + }, +#endif +#ifdef CONFIG_HIST_TRIGGERS + { + .name = "hist", + .callback = event_callback, + }, +#endif +#ifdef CONFIG_HIST_TRIGGERS_DEBUG + { + .name = "hist_debug", + .callback = event_callback, + }, +#endif +#ifdef CONFIG_TRACE_EVENT_INJECT + { + .name = "inject", + .callback = event_callback, + }, +#endif + };

/* * If the trace point header did not define TRACE_SYSTEM @@ -2440,29 +2575,20 @@ event_create_dir(struct dentry *parent, struct trace_event_file *file) if (WARN_ON_ONCE(strcmp(call->class->system, TRACE_SYSTEM) == 0)) return -ENODEV;

- ef_subsystem = event_subsystem_dir(tr, call->class->system, file, parent); - if (!ef_subsystem) + e_events = event_subsystem_dir(tr, call->class->system, file, parent); + if (!e_events) return -ENOMEM;

+ nr_entries = ARRAY_SIZE(event_entries); + name = trace_event_name(call); - ef = eventfs_add_dir(name, ef_subsystem); - if (IS_ERR(ef)) { + ei = eventfs_create_dir(name, e_events, event_entries, nr_entries, file); + if (IS_ERR(ei)) { pr_warn("Could not create tracefs '%s' directory\n", name); return -1; }

- file->ef = ef; - - if (call->class->reg && !(call->flags & TRACE_EVENT_FL_IGNORE_ENABLE)) - eventfs_add_file("enable", TRACE_MODE_WRITE, file->ef, file, - &ftrace_enable_fops); - -#ifdef CONFIG_PERF_EVENTS - if (call->event.type && call->class->reg) - eventfs_add_file("id", TRACE_MODE_READ, file->ef, - (void *)(long)call->event.type, - &ftrace_event_id_fops); -#endif + file->ei = ei;

ret = event_define_fields(call); if (ret < 0) { @@ -2470,35 +2596,6 @@ event_create_dir(struct dentry *parent, struct trace_event_file *file) return ret; }

- /* - * Only event directories that can be enabled should have - * triggers or filters. - */ - if (!(call->flags & TRACE_EVENT_FL_IGNORE_ENABLE)) { - eventfs_add_file("filter", TRACE_MODE_WRITE, file->ef, - file, &ftrace_event_filter_fops); - - eventfs_add_file("trigger", TRACE_MODE_WRITE, file->ef, - file, &event_trigger_fops); - } - -#ifdef CONFIG_HIST_TRIGGERS - eventfs_add_file("hist", TRACE_MODE_READ, file->ef, file, - &event_hist_fops); -#endif -#ifdef CONFIG_HIST_TRIGGERS_DEBUG - eventfs_add_file("hist_debug", TRACE_MODE_READ, file->ef, file, - &event_hist_debug_fops); -#endif - eventfs_add_file("format", TRACE_MODE_READ, file->ef, call, - &ftrace_event_format_fops); - -#ifdef CONFIG_TRACE_EVENT_INJECT - if (call->event.type && call->class->reg) - eventfs_add_file("inject", 0200, file->ef, file, - &event_inject_fops); -#endif - return 0; }

@@ -3644,30 +3741,65 @@ static __init int setup_trace_event(char *str) } __setup("trace_event=", setup_trace_event);

+static int events_callback(const char *name, umode_t *mode, void **data, + const struct file_operations **fops) +{ + if (strcmp(name, "enable") == 0) { + *mode = TRACE_MODE_WRITE; + *fops = &ftrace_tr_enable_fops; + return 1; + } + + if (strcmp(name, "header_page") == 0) + *data = ring_buffer_print_page_header; + + else if (strcmp(name, "header_event") == 0) + *data = ring_buffer_print_entry_header; + + else + return 0; + + *mode = TRACE_MODE_READ; + *fops = &ftrace_show_header_fops; + return 1; +} + /* Expects to have event_mutex held when called */ static int create_event_toplevel_files(struct dentry *parent, struct trace_array *tr) { - struct dentry *d_events; + struct eventfs_inode *e_events; struct dentry *entry; - int error = 0; + int nr_entries; + static struct eventfs_entry events_entries[] = { + { + .name = "enable", + .callback = events_callback, + }, + { + .name = "header_page", + .callback = events_callback, + }, + { + .name = "header_event", + .callback = events_callback, + }, + };

entry = trace_create_file("set_event", TRACE_MODE_WRITE, parent, tr, &ftrace_set_event_fops); if (!entry) return -ENOMEM;

- d_events = eventfs_create_events_dir("events", parent); - if (IS_ERR(d_events)) { + nr_entries = ARRAY_SIZE(events_entries); + + e_events = eventfs_create_events_dir("events", parent, events_entries, + nr_entries, tr); + if (IS_ERR(e_events)) { pr_warn("Could not create tracefs 'events' directory\n"); return -ENOMEM; }

- error = eventfs_add_events_file("enable", TRACE_MODE_WRITE, d_events, - tr, &ftrace_tr_enable_fops); - if (error) - return -ENOMEM; - /* There are not as crucial, just warn if they are not created */

trace_create_file("set_event_pid", TRACE_MODE_WRITE, parent, @@ -3677,16 +3809,7 @@ create_event_toplevel_files(struct dentry *parent, struct trace_array *tr) TRACE_MODE_WRITE, parent, tr, &ftrace_set_event_notrace_pid_fops);

- /* ring buffer internal formats */ - eventfs_add_events_file("header_page", TRACE_MODE_READ, d_events, - ring_buffer_print_page_header, - &ftrace_show_header_fops); - - eventfs_add_events_file("header_event", TRACE_MODE_READ, d_events, - ring_buffer_print_entry_header, - &ftrace_show_header_fops); - - tr->event_dir = d_events; + tr->event_dir = e_events;

return 0; } @@ -3770,7 +3893,7 @@ int event_trace_del_tracer(struct trace_array *tr)

down_write(&trace_event_sem); __trace_remove_event_dirs(tr); - eventfs_remove_events_dir(tr->event_dir); + eventfs_remove_dir(tr->event_dir); up_write(&trace_event_sem);

tr->event_dir = NULL;

From: Nathan Chancellor nathan@kernel.org

When building with clang and CONFIG_RANDSTRUCT_FULL=y, there is an error due to a cast in eventfs_create_events_dir():

fs/tracefs/event_inode.c:734:10: error: casting from randomized structure pointer type 'struct dentry *' to 'struct eventfs_inode *' 734 | return (struct eventfs_inode *)dentry; | ^ 1 error generated.

Use the ERR_CAST() function to resolve the error, as it was designed for this exact situation (casting an error pointer to another type).

Link: https://lore.kernel.org/linux-trace-kernel/20231018-ftrace-fix-clang-randstr...

Closes: https://github.com/ClangBuiltLinux/linux/issues/1947 Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Nathan Chancellor nathan@kernel.org Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit b8a555dc31e5aa18d976de0bc228006e398a2e7d) --- fs/tracefs/event_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 1ccd100bc565..9f19b6608954 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -731,7 +731,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry return NULL;

if (IS_ERR(dentry)) - return (struct eventfs_inode *)dentry; + return ERR_CAST(dentry);

ei = kzalloc(sizeof(*ei), GFP_KERNEL); if (!ei)

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The failure path of allocating ei goes to a path that dereferences ei. Add another label that skips over the ei dereferences to do the rest of the clean up.

Link: https://lore.kernel.org/all/70e7bace-561c-95f-1117-706c2c220bc@inria.fr/ Link: https://lore.kernel.org/linux-trace-kernel/20231019204132.6662fef0@gandalf.l...

Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Mark Rutland mark.rutland@arm.com Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: Julia Lawall julia.lawall@inria.fr Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 7e8ad67c9b5c11e990c320ed7e7563f2301672a7) --- fs/tracefs/event_inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 9f19b6608954..1885f1f1f339 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -735,7 +735,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry

ei = kzalloc(sizeof(*ei), GFP_KERNEL); if (!ei) - goto fail; + goto fail_ei;

inode = tracefs_get_inode(dentry->d_sb); if (unlikely(!inode)) @@ -781,6 +781,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry fail: kfree(ei->d_children); kfree(ei); + fail_ei: tracefs_failed_creating(dentry); return ERR_PTR(-ENOMEM); }

From: "Steven Rostedt (Google)" rostedt@goodmis.org

As the comment right above a WARN_ON() in create_file_dentry() states:

* Note, with the mutex held, the e_dentry cannot have content * and the ei->is_freed be true at the same time.

But the WARN_ON() only has:

WARN_ON_ONCE(ei->is_free);

Where to match the comment (and what it should actually do) is:

dentry = *e_dentry; WARN_ON_ONCE(dentry && ei->is_free)

Also in that case, set dentry to NULL (although it should never happen).

Link: https://lore.kernel.org/linux-trace-kernel/20231024123628.62b88755@gandalf.l...

Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Mark Rutland mark.rutland@arm.com Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit a9de4eb15ad430fe45747c211e367da745a90093) --- fs/tracefs/event_inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 09ab93357957..4d2da7480e5f 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -264,8 +264,9 @@ create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry, * Note, with the mutex held, the e_dentry cannot have content * and the ei->is_freed be true at the same time. */ - WARN_ON_ONCE(ei->is_freed); dentry = *e_dentry; + if (WARN_ON_ONCE(dentry && ei->is_freed)) + dentry = NULL; /* The lookup does not need to up the dentry refcount */ if (dentry && !lookup) dget(dentry);

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The creation of the top events directory does a dget() at the end of the creation in eventfs_create_events_dir() with a comment saying the final dput() will happen when it is removed. The problem is that a dget() is already done on the dentry when it was created with tracefs_start_creating()! The dget() now just causes a memory leak of that dentry.

Remove the extra dget() as the final dput() in the deletion of the events directory actually matches the one in tracefs_start_creating().

Link: https://lore.kernel.org/linux-trace-kernel/20231031124229.4f2e3fa1@gandalf.l...

Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Mark Rutland mark.rutland@arm.com Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 77bc4d4921bd3497678ba8e7f4e480de35692f05) --- fs/tracefs/event_inode.c | 3 --- 1 file changed, 3 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 4d2da7480e5f..5536860eb2ff 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -774,9 +774,6 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry fsnotify_mkdir(dentry->d_parent->d_inode, dentry); tracefs_end_creating(dentry);

- /* Will call dput when the directory is removed */ - dget(dentry); - return ei;

fail:

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The eventfs_inode->is_freed was a union with the rcu_head with the assumption that when it was on the srcu list the head would contain a pointer which would make "is_freed" true. But that was a wrong assumption as the rcu head is a single link list where the last element is NULL.

Instead, split the nr_entries integer so that "is_freed" is one bit and the nr_entries is the next 31 bits. As there shouldn't be more than 10 (currently there's at most 5 to 7 depending on the config), this should not be a problem.

Link: https://lkml.kernel.org/r/20231101172649.049758712@goodmis.org

Cc: stable@vger.kernel.org Cc: Mark Rutland mark.rutland@arm.com Cc: Andrew Morton akpm@linux-foundation.org Cc: Ajay Kaher akaher@vmware.com Fixes: 63940449555e7 ("eventfs: Implement eventfs lookup, read, open functions") Reviewed-by: Masami Hiramatsu (Google) mhiramat@kernel.org Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit f2f496370afcbc5227d7002da28c74b91fed12ff) --- fs/tracefs/event_inode.c | 2 ++ fs/tracefs/internal.h | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 9f612a8f009d..1ce73acf3df0 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -824,6 +824,8 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, eventfs_remove_rec(ei_child, head, level + 1); }

+ ei->is_freed = 1; + list_del_rcu(&ei->list); list_add_tail(&ei->del_list, head); } diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 64fde9490f52..c7d88aaa949f 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -23,6 +23,7 @@ struct tracefs_inode { * @d_parent: pointer to the parent's dentry * @d_children: The array of dentries to represent the files when created * @data: The private data to pass to the callbacks + * @is_freed: Flag set if the eventfs is on its way to be freed * @nr_entries: The number of items in @entries */ struct eventfs_inode { @@ -38,14 +39,13 @@ struct eventfs_inode { * Union - used for deletion * @del_list: list of eventfs_inode to delete * @rcu: eventfs_inode to delete in RCU - * @is_freed: node is freed if one of the above is set */ union { struct list_head del_list; struct rcu_head rcu; - unsigned long is_freed; }; - int nr_entries; + unsigned int is_freed:1; + unsigned int nr_entries:31; };

static inline struct tracefs_inode *get_tracefs(const struct inode *inode)

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The eventfs_inode (ei) is protected by SRCU, but the ei->dentry is not. It is protected by the eventfs_mutex. Anytime the eventfs_mutex is released, and access to the ei->dentry needs to be done, it should first check if ei->is_freed is set under the eventfs_mutex. If it is, then the ei->dentry is invalid and must not be used. The ei->dentry must only be accessed under the eventfs_mutex and after checking if ei->is_freed is set.

When the ei is being freed, it will (under the eventfs_mutex) set is_freed and at the same time move the dentry to a free list to be cleared after the eventfs_mutex is released. This means that any access to the ei->dentry must check first if ei->is_freed is set, because if it is, then the dentry is on its way to be freed.

Also add comments to describe this better.

Link: https://lore.kernel.org/all/CA+G9fYt6pY+tMZEOg=SoEywQOe19fGP3uR15SGowkdK+_X8... Link: https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunSJf=MTe=... Link: https://lkml.kernel.org/r/20231101172649.477608228@goodmis.org

Cc: Ajay Kaher akaher@vmware.com Cc: Mark Rutland mark.rutland@arm.com Cc: Andrew Morton akpm@linux-foundation.org Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: Linux Kernel Functional Testing lkft@linaro.org Reported-by: Naresh Kamboju naresh.kamboju@linaro.org Reported-by: Beau Belgrave beaub@linux.microsoft.com Reviewed-by: Masami Hiramatsu (Google) mhiramat@kernel.org Tested-by: Linux Kernel Functional Testing lkft@linaro.org Tested-by: Naresh Kamboju naresh.kamboju@linaro.org Tested-by: Beau Belgrave beaub@linux.microsoft.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 77a06c33a22d13f3a6e31f06f6ee6bca666e6898) --- fs/tracefs/event_inode.c | 45 ++++++++++++++++++++++++++++++++++------ fs/tracefs/internal.h | 3 ++- 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index dd5971855732..e9625732c52d 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -24,7 +24,20 @@ #include <linux/delay.h> #include "internal.h"

+/* + * eventfs_mutex protects the eventfs_inode (ei) dentry. Any access + * to the ei->dentry must be done under this mutex and after checking + * if ei->is_freed is not set. The ei->dentry is released under the + * mutex at the same time ei->is_freed is set. If ei->is_freed is set + * then the ei->dentry is invalid. + */ static DEFINE_MUTEX(eventfs_mutex); + +/* + * 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). + * After the SRCU grace period is over, the ei may be freed. + */ DEFINE_STATIC_SRCU(eventfs_srcu);

static struct dentry *eventfs_root_lookup(struct inode *dir, @@ -239,6 +252,10 @@ create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry, bool invalidate = false;

mutex_lock(&eventfs_mutex); + if (ei->is_freed) { + mutex_unlock(&eventfs_mutex); + return NULL; + } /* If the e_dentry already has a dentry, use it */ if (*e_dentry) { /* lookup does not need to up the ref count */ @@ -312,6 +329,8 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei) struct eventfs_inode *ei_child; struct tracefs_inode *ti;

+ lockdep_assert_held(&eventfs_mutex); + /* srcu lock already held */ /* fill parent-child relation */ list_for_each_entry_srcu(ei_child, &ei->children, list, @@ -325,6 +344,7 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)

/** * create_dir_dentry - Create a directory dentry for the eventfs_inode + * @pei: The eventfs_inode parent of ei. * @ei: The eventfs_inode to create the directory for * @parent: The dentry of the parent of this directory * @lookup: True if this is called by the lookup code @@ -332,12 +352,17 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei) * This creates and attaches a directory dentry to the eventfs_inode @ei. */ static struct dentry * -create_dir_dentry(struct eventfs_inode *ei, struct dentry *parent, bool lookup) +create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, + struct dentry *parent, bool lookup) { bool invalidate = false; struct dentry *dentry = NULL;

mutex_lock(&eventfs_mutex); + if (pei->is_freed || ei->is_freed) { + mutex_unlock(&eventfs_mutex); + return NULL; + } if (ei->dentry) { /* If the dentry already has a dentry, use it */ dentry = ei->dentry; @@ -440,7 +465,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir, */ mutex_lock(&eventfs_mutex); ei = READ_ONCE(ti->private); - if (ei) + if (ei && !ei->is_freed) ei_dentry = READ_ONCE(ei->dentry); mutex_unlock(&eventfs_mutex);

@@ -454,7 +479,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir, if (strcmp(ei_child->name, name) != 0) continue; ret = simple_lookup(dir, dentry, flags); - create_dir_dentry(ei_child, ei_dentry, true); + create_dir_dentry(ei, ei_child, ei_dentry, true); created = true; break; } @@ -588,7 +613,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)

list_for_each_entry_srcu(ei_child, &ei->children, list, srcu_read_lock_held(&eventfs_srcu)) { - d = create_dir_dentry(ei_child, parent, false); + d = create_dir_dentry(ei, ei_child, parent, false); if (d) { ret = add_dentries(&dentries, d, cnt); if (ret < 0) @@ -705,12 +730,20 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode ei->nr_entries = size; ei->data = data; INIT_LIST_HEAD(&ei->children); + INIT_LIST_HEAD(&ei->list);

mutex_lock(&eventfs_mutex); - list_add_tail(&ei->list, &parent->children); - ei->d_parent = parent->dentry; + if (!parent->is_freed) { + list_add_tail(&ei->list, &parent->children); + ei->d_parent = parent->dentry; + } mutex_unlock(&eventfs_mutex);

+ /* Was the parent freed? */ + if (list_empty(&ei->list)) { + free_ei(ei); + ei = NULL; + } return ei; }

diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index c7d88aaa949f..5a98e87dd3d1 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -24,6 +24,7 @@ struct tracefs_inode { * @d_children: The array of dentries to represent the files when created * @data: The private data to pass to the callbacks * @is_freed: Flag set if the eventfs is on its way to be freed + * Note if is_freed is set, then dentry is corrupted. * @nr_entries: The number of items in @entries */ struct eventfs_inode { @@ -31,7 +32,7 @@ struct eventfs_inode { const struct eventfs_entry *entries; const char *name; struct list_head children; - struct dentry *dentry; + struct dentry *dentry; /* Check is_freed to access */ struct dentry *d_parent; struct dentry **d_children; void *data;

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The callback function that is used to create inodes and dentries is not protected by anything and the data that is passed to it could become stale. After eventfs_remove_dir() is called by the tracing system, it is free to remove the events that are associated to that directory. Unfortunately, that means the callbacks must not be called after that.

CPU0 CPU1 ---- ---- eventfs_root_lookup() { eventfs_remove_dir() { mutex_lock(&event_mutex); ei->is_freed = set; mutex_unlock(&event_mutex); } kfree(event_call);

for (...) { entry = &ei->entries[i]; r = entry->callback() { call = data; // call == event_call above if (call->flags ...)

[ USE AFTER FREE BUG ]

The safest way to protect this is to wrap the callback with:

mutex_lock(&eventfs_mutex); if (!ei->is_freed) r = entry->callback(); else r = -1; mutex_unlock(&eventfs_mutex);

This will make sure that the callback will not be called after it is freed. But now it needs to be known that the callback is called while holding internal eventfs locks, and that it must not call back into the eventfs / tracefs system. There's no reason it should anyway, but document that as well.

Link: https://lore.kernel.org/all/CA+G9fYu9GOEbD=rR5eMR-=HJ8H6rMsbzDC2ZY5=Y50WpWAE... Link: https://lkml.kernel.org/r/20231101172649.906696613@goodmis.org

Cc: Ajay Kaher akaher@vmware.com Cc: Mark Rutland mark.rutland@arm.com Cc: Andrew Morton akpm@linux-foundation.org Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: Linux Kernel Functional Testing lkft@linaro.org Reported-by: Naresh Kamboju naresh.kamboju@linaro.org Tested-by: Linux Kernel Functional Testing lkft@linaro.org Tested-by: Naresh Kamboju naresh.kamboju@linaro.org Reviewed-by: Masami Hiramatsu (Google) mhiramat@kernel.org Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 44365329f8219fc379097c2c9a75ff53f123764f) --- fs/tracefs/event_inode.c | 22 ++++++++++++++++++-- include/linux/tracefs.h | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 93d08e552483..8ac9abf7a3d5 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -615,7 +615,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir, entry = &ei->entries[i]; if (strcmp(name, entry->name) == 0) { void *cdata = data; - r = entry->callback(name, &mode, &cdata, &fops); + 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); @@ -749,7 +755,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file) void *cdata = data; entry = &ei->entries[i]; name = entry->name; - r = entry->callback(name, &mode, &cdata, &fops); + 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; d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false); @@ -819,6 +831,10 @@ static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx) * data = A pointer to @data, and the callback may replace it, which will * cause the file created to pass the new data to the open() call. * fops = the fops to use for the created file. + * + * NB. @callback is called while holding internal locks of the eventfs + * system. The callback must not call any code that might also call into + * the tracefs or eventfs system or it will risk creating a deadlock. */ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent, const struct eventfs_entry *entries, @@ -878,6 +894,8 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode * @data: The default data to pass to the files (an entry may override it). * * This function creates the top of the trace event directory. + * + * See eventfs_create_dir() for use of @entries. */ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent, const struct eventfs_entry *entries, diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h index 13359b1a35d1..7a5fe17b6bf9 100644 --- a/include/linux/tracefs.h +++ b/include/linux/tracefs.h @@ -23,9 +23,52 @@ struct file_operations;

struct eventfs_file;

+/** + * eventfs_callback - A callback function to create dynamic files in eventfs + * @name: The name of the file that is to be created + * @mode: return the file mode for the file (RW access, etc) + * @data: data to pass to the created file ops + * @fops: the file operations of the created file + * + * The evetnfs files are dynamically created. The struct eventfs_entry array + * is passed to eventfs_create_dir() or eventfs_create_events_dir() that will + * be used to create the files within those directories. When a lookup + * or access to a file within the directory is made, the struct eventfs_entry + * array is used to find a callback() with the matching name that is being + * referenced (for lookups, the entire array is iterated and each callback + * will be called). + * + * The callback will be called with @name for the name of the file to create. + * The callback can return less than 1 to indicate that no file should be + * created. + * + * If a file is to be created, then @mode should be populated with the file + * mode (permissions) for which the file is created for. This would be + * used to set the created inode i_mode field. + * + * The @data should be set to the data passed to the other file operations + * (read, write, etc). Note, @data will also point to the data passed in + * to eventfs_create_dir() or eventfs_create_events_dir(), but the callback + * can replace the data if it chooses to. Otherwise, the original data + * will be used for the file operation functions. + * + * The @fops should be set to the file operations that will be used to create + * the inode. + * + * NB. This callback is called while holding internal locks of the eventfs + * system. The callback must not call any code that might also call into + * the tracefs or eventfs system or it will risk creating a deadlock. + */ typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data, const struct file_operations **fops);

+/** + * struct eventfs_entry - dynamically created eventfs file call back handler + * @name: Then name of the dynamic file in an eventfs directory + * @callback: The callback to get the fops of the file when it is created + * + * See evenfs_callback() typedef for how to set up @callback. + */ struct eventfs_entry { const char *name; eventfs_callback callback;

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The top level events directory is no longer special with regards to how it should be delete. Remove the extra processing for it in eventfs_set_ei_status_free().

Link: https://lkml.kernel.org/r/20231101172650.340876747@goodmis.org

Cc: Ajay Kaher akaher@vmware.com Cc: Mark Rutland mark.rutland@arm.com Cc: Andrew Morton akpm@linux-foundation.org Reviewed-by: Masami Hiramatsu (Google) mhiramat@kernel.org Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 62d65cac119d08d39f751b4e3e2063ed996edc05) --- fs/tracefs/event_inode.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 0a04ae0ca8c8..0087a3f455f1 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -274,28 +274,11 @@ static void free_ei(struct eventfs_inode *ei) */ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) { - struct tracefs_inode *ti_parent; struct eventfs_inode *ei; int i;

- /* The top level events directory may be freed by this */ - if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) { - mutex_lock(&eventfs_mutex); - ei = ti->private; - /* Nothing should access this, but just in case! */ - ti->private = NULL; - mutex_unlock(&eventfs_mutex); - - free_ei(ei); - return; - } - mutex_lock(&eventfs_mutex);

- ti_parent = get_tracefs(dentry->d_parent->d_inode); - if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE)) - goto out; - ei = dentry->d_fsdata; if (!ei) goto out; @@ -920,6 +903,8 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry inode->i_op = &eventfs_root_dir_inode_operations; inode->i_fop = &eventfs_file_operations;

+ dentry->d_fsdata = ei; + /* directory inodes start off with i_nlink == 2 (for "." entry) */ inc_nlink(inode); d_instantiate(dentry, inode);

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The logic to free the eventfs_inode (ei) use to set is_freed and clear the "dentry" field under the eventfs_mutex. But that changed when a race was found where the ei->dentry needed to be cleared when the last dput() was called on it. But there was still logic that checked if ei->dentry was not NULL and is_freed is set, and would warn if it was.

But since that situation was changed and the ei->dentry isn't cleared until the last dput() is called on it while the ei->is_freed is set, do not test for that condition anymore, and change the comments to reflect that.

Link: https://lkml.kernel.org/r/20231120235154.265826243@goodmis.org

Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Fixes: 020010fbfa20 ("eventfs: Delete eventfs_inode when the last dentry is freed") Reported-by: Mark Rutland mark.rutland@arm.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 88903daecacf03b1e5636e1b5f18bda5b07030fc) --- fs/tracefs/event_inode.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index f8a594a50ae6..f239b2b507a4 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -27,16 +27,16 @@ /* * eventfs_mutex protects the eventfs_inode (ei) dentry. Any access * to the ei->dentry must be done under this mutex and after checking - * if ei->is_freed is not set. The ei->dentry is released under the - * mutex at the same time ei->is_freed is set. If ei->is_freed is set - * then the ei->dentry is invalid. + * if ei->is_freed is not set. When ei->is_freed is set, the dentry + * is on its way to being freed after the last dput() is made on it. */ static DEFINE_MUTEX(eventfs_mutex);

/* * 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). - * After the SRCU grace period is over, the ei may be freed. + * After the SRCU grace period is over and the last dput() is called + * the ei is freed. */ DEFINE_STATIC_SRCU(eventfs_srcu);

@@ -365,12 +365,14 @@ create_file_dentry(struct eventfs_inode *ei, int idx, * created the dentry for this e_dentry. In which case * use that one. * - * Note, with the mutex held, the e_dentry cannot have content - * and the ei->is_freed be true at the same time. + * If ei->is_freed is set, the e_dentry is currently on its + * way to being freed, don't return it. If e_dentry is NULL + * it means it was already freed. */ - dentry = *e_dentry; - if (WARN_ON_ONCE(dentry && ei->is_freed)) + if (ei->is_freed) dentry = NULL; + else + dentry = *e_dentry; /* The lookup does not need to up the dentry refcount */ if (dentry && !lookup) dget(dentry); @@ -473,8 +475,8 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, * created the dentry for this e_dentry. In which case * use that one. * - * Note, with the mutex held, the e_dentry cannot have content - * and the ei->is_freed be true at the same time. + * If ei->is_freed is set, the e_dentry is currently on its + * way to being freed. */ dentry = ei->dentry; if (dentry && !lookup)

From: "Steven Rostedt (Google)" rostedt@goodmis.org

If memory reclaim happens, it can reclaim file system pages. The file system pages from eventfs may take the eventfs_mutex on reclaim. This means that allocation while holding the eventfs_mutex must not call into filesystem reclaim. A lockdep splat uncovered this.

Link: https://lkml.kernel.org/r/20231121231112.373501894@goodmis.org

Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Andrew Morton akpm@linux-foundation.org Fixes: 28e12c09f5aa0 ("eventfs: Save ownership and mode") Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: Mark Rutland mark.rutland@arm.com Reviewed-by: Josef Bacik josef@toxicpanda.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 4763d635c907baed212664dc579dde1663bb2676) --- fs/tracefs/event_inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 3eb6c622a74d..56d192f0ead8 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -95,7 +95,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry, if (!(dentry->d_inode->i_mode & S_IFDIR)) { if (!ei->entry_attrs) { ei->entry_attrs = kzalloc(sizeof(*ei->entry_attrs) * ei->nr_entries, - GFP_KERNEL); + GFP_NOFS); if (!ei->entry_attrs) { ret = -ENOMEM; goto out; @@ -627,7 +627,7 @@ static int add_dentries(struct dentry ***dentries, struct dentry *d, int cnt) { struct dentry **tmp;

- tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_KERNEL); + tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_NOFS); if (!tmp) return -1; tmp[cnt] = d;

From: "Steven Rostedt (Google)" rostedt@goodmis.org

The eventfs directory is dynamically created via the meta data supplied by the existing trace events. All files and directories in eventfs has a parent. Do not allow NULL to be passed into eventfs_start_creating() as the parent because that should never happen. Warn if it does.

Link: https://lkml.kernel.org/r/20231121231112.693841807@goodmis.org

Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Mark Rutland mark.rutland@arm.com Cc: Andrew Morton akpm@linux-foundation.org Reviewed-by: Josef Bacik josef@toxicpanda.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit fc4561226feaad5fcdcb55646c348d77b8ee69c5) --- fs/tracefs/inode.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index 34ffb2f8114e..b9ed8db4f6b9 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -509,20 +509,15 @@ struct dentry *eventfs_start_creating(const char *name, struct dentry *parent) struct dentry *dentry; int error;

+ /* Must always have a parent. */ + if (WARN_ON_ONCE(!parent)) + return ERR_PTR(-EINVAL); + error = simple_pin_fs(&trace_fs_type, &tracefs_mount, &tracefs_mount_count); if (error) return ERR_PTR(error);

- /* - * If the parent is not specified, we create it in the root. - * We need the root dentry to do this, which is in the super - * block. A pointer to that is in the struct vfsmount that we - * have around. - */ - if (!parent) - parent = tracefs_mount->mnt_root; - if (unlikely(IS_DEADDIR(parent->d_inode))) dentry = ERR_PTR(-ENOENT); else

From: Beau Belgrave beaub@linux.microsoft.com

Eventfs uses simple_lookup(), however, it will fail if the name of the entry is beyond NAME_MAX length. When this error is encountered, eventfs still tries to create dentries instead of skipping the dentry creation. When the dentry is attempted to be created in this state d_wait_lookup() will loop forever, waiting for the lookup to be removed.

Fix eventfs to return the error in simple_lookup() back to the caller instead of continuing to try to create the dentry.

Link: https://lore.kernel.org/linux-trace-kernel/20231210213534.497-1-beaub@linux....

Fixes: 63940449555e ("eventfs: Implement eventfs lookup, read, open functions") Link: https://lore.kernel.org/linux-trace-kernel/20231208183601.GA46-beaub@linux.m... Signed-off-by: Beau Belgrave beaub@linux.microsoft.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 5eaf7f0589c0d88178f0fbeebe0e0b7108258707) --- fs/tracefs/event_inode.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 0b90869fd805..43e237864a42 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -546,6 +546,8 @@ static struct dentry *eventfs_root_lookup(struct inode *dir, if (strcmp(ei_child->name, name) != 0) continue; ret = simple_lookup(dir, dentry, flags); + if (IS_ERR(ret)) + goto out; create_dir_dentry(ei, ei_child, ei_dentry, true); created = true; break; @@ -568,6 +570,8 @@ static struct dentry *eventfs_root_lookup(struct inode *dir, if (r <= 0) continue; ret = simple_lookup(dir, dentry, flags); + if (IS_ERR(ret)) + goto out; create_file_dentry(ei, i, ei_dentry, name, mode, cdata, fops, true); break;

From: "Steven Rostedt (Google)" rostedt@goodmis.org

It was reported that when mounting the tracefs file system with a gid other than root, the ownership did not carry down to the eventfs directory due to the dynamic nature of it.

A fix was done to solve this, but it had two issues.

(a) if the attr passed into update_inode_attr() was NULL, it didn't do anything. This is true for files that have not had a chown or chgrp done to itself or any of its sibling files, as the attr is allocated for all children when any one needs it.

# umount /sys/kernel/tracing # mount -o rw,seclabel,relatime,gid=1000 -t tracefs nodev /mnt

# ls -ld /mnt/events/sched drwxr-xr-x 28 root rostedt 0 Dec 21 13:12 /mnt/events/sched/

# ls -ld /mnt/events/sched/sched_switch drwxr-xr-x 2 root rostedt 0 Dec 21 13:12 /mnt/events/sched/sched_switch/

But when checking the files:

# ls -l /mnt/events/sched/sched_switch total 0 -rw-r----- 1 root root 0 Dec 21 13:12 enable -rw-r----- 1 root root 0 Dec 21 13:12 filter -r--r----- 1 root root 0 Dec 21 13:12 format -r--r----- 1 root root 0 Dec 21 13:12 hist -r--r----- 1 root root 0 Dec 21 13:12 id -rw-r----- 1 root root 0 Dec 21 13:12 trigger

(b) When the attr does not denote the UID or GID, it defaulted to using the parent uid or gid. This is incorrect as changing the parent uid or gid will automatically change all its children.

# chgrp tracing /mnt/events/timer

# ls -ld /mnt/events/timer drwxr-xr-x 2 root tracing 0 Dec 21 14:34 /mnt/events/timer

# ls -l /mnt/events/timer total 0 -rw-r----- 1 root root 0 Dec 21 14:35 enable -rw-r----- 1 root root 0 Dec 21 14:35 filter drwxr-xr-x 2 root tracing 0 Dec 21 14:35 hrtimer_cancel drwxr-xr-x 2 root tracing 0 Dec 21 14:35 hrtimer_expire_entry drwxr-xr-x 2 root tracing 0 Dec 21 14:35 hrtimer_expire_exit drwxr-xr-x 2 root tracing 0 Dec 21 14:35 hrtimer_init drwxr-xr-x 2 root tracing 0 Dec 21 14:35 hrtimer_start drwxr-xr-x 2 root tracing 0 Dec 21 14:35 itimer_expire drwxr-xr-x 2 root tracing 0 Dec 21 14:35 itimer_state drwxr-xr-x 2 root tracing 0 Dec 21 14:35 tick_stop drwxr-xr-x 2 root tracing 0 Dec 21 14:35 timer_cancel drwxr-xr-x 2 root tracing 0 Dec 21 14:35 timer_expire_entry drwxr-xr-x 2 root tracing 0 Dec 21 14:35 timer_expire_exit drwxr-xr-x 2 root tracing 0 Dec 21 14:35 timer_init drwxr-xr-x 2 root tracing 0 Dec 21 14:35 timer_start

At first it was thought that this could be easily fixed by just making the default ownership of the superblock when it was mounted. But this does not handle the case of:

# chgrp tracing instances # mkdir instances/foo

If the superblock was used, then the group ownership would be that of what it was when it was mounted, when it should instead be "tracing".

Instead, set a flag for the top level eventfs directory ("events") to flag which eventfs_inode belongs to it.

Since the "events" directory's dentry and inode are never freed, it does not need to use its attr field to restore its mode and ownership. Use the this eventfs_inode's attr as the default ownership for all the files and directories underneath it.

When the events eventfs_inode is created, it sets its ownership to its parent uid and gid. As the events directory is created at boot up before it gets mounted, this will always be uid=0 and gid=0. If it's created via an instance, then it will take the ownership of the instance directory.

When the file system is mounted, it will update all the gids if one is specified. This will have a callback to update the events evenfs_inode's default entries.

When a file or directory is created under the events directory, it will walk the ei->dentry parents until it finds the evenfs_inode that belongs to the events directory to retrieve the default uid and gid values.

Link: https://lore.kernel.org/all/CAHk-=wiwQtUHvzwyZucDq8=Gtw+AnwScyLhpFswrQ84Pjho... Link: https://lore.kernel.org/linux-trace-kernel/20231221190757.7eddbca9@gandalf.l...

Cc: stable@vger.kernel.org Cc: Mark Rutland mark.rutland@arm.com Cc: Mathieu Desnoyers mathieu.desnoyers@efficios.com Cc: Dongliang Cui cuidongliang390@gmail.com Cc: Hongyu Jin hongyu.jin@unisoc.com Fixes: 0dfc852b6fe3 ("eventfs: Have event files and directories default to parent uid and gid") Reviewed-by: Masami Hiramatsu (Google) mhiramat@kernel.org Tested-by: Masami Hiramatsu (Google) mhiramat@kernel.org Reported-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit 7e8358edf503e87236c8d07f69ef0ed846dd5112) --- fs/tracefs/event_inode.c | 105 +++++++++++++++++++++++++++++++++++---- fs/tracefs/inode.c | 6 +++ fs/tracefs/internal.h | 2 + 3 files changed, 103 insertions(+), 10 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 2ccc849a5bda..f0677ea0ec24 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -113,7 +113,14 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry, * determined by the parent directory. */ if (dentry->d_inode->i_mode & S_IFDIR) { - update_attr(&ei->attr, iattr); + /* + * The events directory dentry is never freed, unless its + * part of an instance that is deleted. It's attr is the + * default for its child files and directories. + * Do not update it. It's not used for its own mode or ownership + */ + if (!ei->is_events) + update_attr(&ei->attr, iattr);

} else { name = dentry->d_name.name; @@ -148,28 +155,93 @@ static const struct file_operations eventfs_file_operations = { .release = eventfs_release, };

+/* Return the evenfs_inode of the "events" directory */ +static struct eventfs_inode *eventfs_find_events(struct dentry *dentry) +{ + struct eventfs_inode *ei; + + mutex_lock(&eventfs_mutex); + do { + /* The parent always has an ei, except for events itself */ + ei = dentry->d_parent->d_fsdata; + + /* + * If the ei is being freed, the ownership of the children + * doesn't matter. + */ + if (ei->is_freed) { + ei = NULL; + break; + } + + dentry = ei->dentry; + } while (!ei->is_events); + mutex_unlock(&eventfs_mutex); + + return ei; +} + static void update_inode_attr(struct dentry *dentry, struct inode *inode, struct eventfs_attr *attr, umode_t mode) { - if (!attr) { - inode->i_mode = mode; + struct eventfs_inode *events_ei = eventfs_find_events(dentry); + + if (!events_ei) + return; + + inode->i_mode = mode; + inode->i_uid = events_ei->attr.uid; + inode->i_gid = events_ei->attr.gid; + + if (!attr) return; - }

if (attr->mode & EVENTFS_SAVE_MODE) inode->i_mode = attr->mode & EVENTFS_MODE_MASK; - else - inode->i_mode = mode;

if (attr->mode & EVENTFS_SAVE_UID) inode->i_uid = attr->uid; - else - inode->i_uid = d_inode(dentry->d_parent)->i_uid;

if (attr->mode & EVENTFS_SAVE_GID) inode->i_gid = attr->gid; - else - inode->i_gid = d_inode(dentry->d_parent)->i_gid; +} + +static void update_gid(struct eventfs_inode *ei, kgid_t gid, int level) +{ + struct eventfs_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 (int i = 0; i < ei->nr_entries; i++) { + ei->entry_attrs[i].gid = gid; + } + } + + /* + * Only eventfs_inode with dentries are updated, make sure + * all eventfs_inodes are updated. If one of the children + * do not have a dentry, this function must traverse it. + */ + 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); + } +} + +void eventfs_update_gid(struct dentry *dentry, kgid_t gid) +{ + struct eventfs_inode *ei = dentry->d_fsdata; + int idx; + + idx = srcu_read_lock(&eventfs_srcu); + update_gid(ei, gid, 0); + srcu_read_unlock(&eventfs_srcu, idx); }

/** @@ -860,6 +932,8 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry struct eventfs_inode *ei; struct tracefs_inode *ti; struct inode *inode; + kuid_t uid; + kgid_t gid;

if (security_locked_down(LOCKDOWN_TRACEFS)) return NULL; @@ -884,11 +958,20 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry ei->dentry = 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) goto fail;

+ /* Save the ownership of this directory */ + uid = d_inode(dentry->d_parent)->i_uid; + gid = d_inode(dentry->d_parent)->i_gid; + + /* This is used as the default ownership of the files and directories */ + ei->attr.uid = uid; + ei->attr.gid = gid; + INIT_LIST_HEAD(&ei->children); INIT_LIST_HEAD(&ei->list);

@@ -897,6 +980,8 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry ti->private = ei;

inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO; + inode->i_uid = uid; + inode->i_gid = gid; inode->i_op = &eventfs_root_dir_inode_operations; inode->i_fop = &eventfs_file_operations;

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index b9ed8db4f6b9..7b0ba379d6d7 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -210,6 +210,7 @@ static void set_gid(struct dentry *parent, kgid_t gid) next = this_parent->d_subdirs.next; resume: while (next != &this_parent->d_subdirs) { + struct tracefs_inode *ti; struct list_head *tmp = next; struct dentry *dentry = list_entry(tmp, struct dentry, d_child); next = tmp->next; @@ -218,6 +219,11 @@ static void set_gid(struct dentry *parent, kgid_t gid)

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_); diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index ccee18ca66c7..899e447778ac 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -62,6 +62,7 @@ struct eventfs_inode { struct rcu_head rcu; }; unsigned int is_freed:1; + unsigned int is_events:1; unsigned int nr_entries:31; };

@@ -77,6 +78,7 @@ struct inode *tracefs_get_inode(struct super_block *sb); struct dentry *eventfs_start_creating(const char *name, struct dentry *parent); struct dentry *eventfs_failed_creating(struct dentry *dentry); struct dentry *eventfs_end_creating(struct dentry *dentry); +void eventfs_update_gid(struct dentry *dentry, kgid_t gid); void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry);

#endif /* _TRACEFS_INTERNAL_H */

From: "Steven Rostedt (Google)" rostedt@goodmis.org

A flag was needed to denote which eventfs_inode was the "events" directory, so a bit was taken from the "nr_entries" field, as there's not that many entries, and 2^30 is plenty. But the bit number for nr_entries was not updated to reflect the bit taken from it, which would add an unnecessary integer to the structure.

Link: https://lore.kernel.org/linux-trace-kernel/20240102151832.7ca87275@gandalf.l...

Cc: stable@vger.kernel.org Cc: Masami Hiramatsu mhiramat@kernel.org Cc: Mathieu Desnoyers mathieu.desnoyers@efficios.com Fixes: 7e8358edf503e ("eventfs: Fix file and directory uid and gid ownership") Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org (cherry picked from commit fd56cd5f6d76e93356d9520cf9dabffe1e3d1aa0) --- fs/tracefs/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 899e447778ac..42bdeb471a07 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -63,7 +63,7 @@ struct eventfs_inode { }; unsigned int is_freed:1; unsigned int is_events:1; - unsigned int nr_entries:31; + unsigned int nr_entries:30; };

static inline struct tracefs_inode *get_tracefs(const struct inode *inode)

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....

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 akaher@vmware.com 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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index c360300fb866..41af56f44f0a 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -52,9 +52,7 @@ enum { static struct dentry *eventfs_root_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags); -static int dcache_dir_open_wrapper(struct inode *inode, struct file *file); -static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx); -static int eventfs_release(struct inode *inode, struct file *file); +static int eventfs_iterate(struct file *file, struct dir_context *ctx);

static void update_attr(struct eventfs_attr *attr, struct iattr *iattr) { @@ -148,11 +146,9 @@ static const struct inode_operations eventfs_file_inode_operations = { };

static const struct file_operations eventfs_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, return ret; }

-struct dentry_list { - void *cursor; - struct dentry **dentries; -}; - -/** - * eventfs_release - called to release eventfs file/dir - * @inode: inode to be released - * @file: file to be released (not used) - */ -static int eventfs_release(struct inode *inode, struct file *file) -{ - struct tracefs_inode *ti; - struct dentry_list *dlist = file->private_data; - void *cursor; - int i; - - 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; - return dcache_dir_close(inode, file); -} - -static int add_dentries(struct dentry ***dentries, struct dentry *d, int cnt) -{ - struct dentry **tmp; - - tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_NOFS); - if (!tmp) - return -1; - tmp[cnt] = d; - tmp[cnt + 1] = NULL; - *dentries = tmp; - return 0; -} - -/** - * dcache_dir_open_wrapper - eventfs open wrapper - * @inode: not used - * @file: dir to be opened (to create it's children) - * - * Used to dynamic create file/dir with-in @file, all the - * file/dir will be created. If already created then references - * will be increased +/* + * Walk the children of a eventfs_inode to fill in getdents(). */ -static int dcache_dir_open_wrapper(struct inode *inode, struct file *file) +static int eventfs_iterate(struct file *file, struct dir_context *ctx) { const struct file_operations *fops; + struct inode *f_inode = file_inode(file); const struct eventfs_entry *entry; struct eventfs_inode *ei_child; struct tracefs_inode *ti; struct eventfs_inode *ei; - struct dentry_list *dlist; - struct dentry **dentries = NULL; - struct dentry *parent = file_dentry(file); - struct dentry *d; - struct inode *f_inode = file_inode(file); - const char *name = parent->d_name.name; + struct dentry *ei_dentry = NULL; + struct dentry *dentry; + const char *name; umode_t mode; - void *data; - int cnt = 0; int idx; - int ret; - int i; - int r; + int ret = -EINVAL; + int ino; + int i, r, c; + + if (!dir_emit_dots(file, ctx)) + return 0;

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) + goto out;

- dlist = kmalloc(sizeof(*dlist), GFP_KERNEL); - if (!dlist) { - srcu_read_unlock(&eventfs_srcu, idx); - return -ENOMEM; - } + ret = 0;

- inode_lock(parent->d_inode); + /* + * Need to create the dentries and inodes to have a consistent + * inode number. + */ 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) + goto out; + ino = dentry->d_inode->i_ino; + dput(dentry); + + if (!dir_emit(ctx, name, strlen(name), ino, DT_DIR)) + goto out; + 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) @@ -774,42 +729,21 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file) mutex_unlock(&eventfs_mutex); if (r <= 0) continue; - d = create_file_dentry(ei, i, parent, name, mode, cdata, fops); - if (d) { - ret = add_dentries(&dentries, d, cnt); - dput(d); - if (ret < 0) - break; - cnt++; - } - } - inode_unlock(parent->d_inode); - srcu_read_unlock(&eventfs_srcu, idx); - ret = dcache_dir_open(inode, file);

- /* - * dcache_dir_open() sets file->private_data to a dentry cursor. - * Need to save that but also save all the dentries that were - * opened by this function. - */ - dlist->cursor = file->private_data; - dlist->dentries = dentries; - file->private_data = dlist; - return ret; -} + dentry = create_file_dentry(ei, i, ei_dentry, name, mode, cdata, fops); + if (!dentry) + goto out; + ino = dentry->d_inode->i_ino; + dput(dentry);

-/* - * This just sets the file->private_data back to the cursor and back. - */ -static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx) -{ - struct dentry_list *dlist = file->private_data; - int ret; + if (!dir_emit(ctx, name, strlen(name), ino, DT_REG)) + goto out; + ctx->pos++; + } + ret = 1; + out: + srcu_read_unlock(&eventfs_srcu, idx);

- file->private_data = dlist->cursor; - ret = dcache_readdir(file, ctx); - dlist->cursor = file->private_data; - file->private_data = dlist; return ret; }

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....

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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 72912b5f9a90..0aca6910efb3 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -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); + goto out; + } + r = entry->callback(name, &mode, &cdata, &fops); mutex_unlock(&eventfs_mutex); if (r <= 0) continue;

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+D... Link: https://lore.kernel.org/linux-trace-kernel/20240104220048.333115095@goodmis....

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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index c73fb1f7ddbc..a1934e0eea3b 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -752,8 +752,8 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx) * Need to create the dentries and inodes to have a consistent * inode number. */ - list_for_each_entry_srcu(ei_child, &ei->children, list, - srcu_read_lock_held(&eventfs_srcu)) { + for (i = 0; i < ei->nr_entries; i++) { + void *cdata = ei->data;

if (c > 0) { c--; @@ -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); + goto out_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) goto out_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)) goto out_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); - goto out_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) goto out_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)) goto out_dec; } ret = 1;

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....

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(+)

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;

From: Erick Archer erick.archer@gmx.com

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-arit...

Link: https://lore.kernel.org/linux-trace-kernel/20240115181658.4562-1-erick.arche...

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 erick.archer@gmx.com 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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 10580d6b5012..6795fda2af19 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -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; @@ -874,7 +874,7 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode }

if (size) { - ei->d_children = kzalloc(sizeof(*ei->d_children) * size, GFP_KERNEL); + ei->d_children = kcalloc(size, sizeof(*ei->d_children), GFP_KERNEL); if (!ei->d_children) { kfree_const(ei->name); kfree(ei); @@ -941,7 +941,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry goto fail;

if (size) { - ei->d_children = kzalloc(sizeof(*ei->d_children) * size, GFP_KERNEL); + ei->d_children = kcalloc(size, sizeof(*ei->d_children), GFP_KERNEL); if (!ei->d_children) goto fail; }

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-Nojs9fjYMvPs1NuoM3yK7aTDtJf...

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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 6b211522a13e..1c3dd0ad4660 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -281,44 +281,6 @@ static void update_inode_attr(struct dentry *dentry, struct inode *inode, inode->i_gid = attr->gid; }

-static void update_gid(struct eventfs_inode *ei, kgid_t gid, int level) -{ - struct eventfs_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 (int i = 0; i < ei->nr_entries; i++) { - ei->entry_attrs[i].gid = gid; - } - } - - /* - * Only eventfs_inode with dentries are updated, make sure - * all eventfs_inodes are updated. If one of the children - * do not have a dentry, this function must traverse it. - */ - 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); - } -} - -void eventfs_update_gid(struct dentry *dentry, kgid_t gid) -{ - struct eventfs_inode *ei = dentry->d_fsdata; - int idx; - - idx = srcu_read_lock(&eventfs_srcu); - update_gid(ei, gid, 0); - srcu_read_unlock(&eventfs_srcu, idx); -} - /** * create_file - create a file in the tracefs filesystem * @name: the name of the file to create. diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 45397df9bb65..91c2bf0b91d9 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -82,7 +82,6 @@ struct inode *tracefs_get_inode(struct super_block *sb); struct dentry *eventfs_start_creating(const char *name, struct dentry *parent); struct dentry *eventfs_failed_creating(struct dentry *dentry); struct dentry *eventfs_end_creating(struct dentry *dentry); -void eventfs_update_gid(struct dentry *dentry, kgid_t gid); void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry);

#endif /* _TRACEFS_INTERNAL_H */

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....

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 oliver.sang@intel.com 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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 1c3dd0ad4660..824b1811e342 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -370,6 +370,8 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent

ti = 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, static void eventfs_post_create_dir(struct eventfs_inode *ei) { struct eventfs_inode *ei_child; - struct tracefs_inode *ti;

lockdep_assert_held(&eventfs_mutex);

@@ -525,9 +526,6 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei) srcu_read_lock_held(&eventfs_srcu)) { ei_child->d_parent = ei->dentry; } - - ti = get_tracefs(ei->dentry->d_inode); - ti->private = ei; }

/**

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... Link: https://lore.kernel.org/linux-trace-kernel/20240131233227.73db55e1@gandalf.l...

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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index e9819d719d2a..04c2ab90f93e 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -230,7 +230,6 @@ static struct eventfs_inode *eventfs_find_events(struct dentry *dentry) { struct eventfs_inode *ei;

- mutex_lock(&eventfs_mutex); do { // The parent is stable because we do not do renames dentry = 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);

@@ -280,11 +278,10 @@ static void update_inode_attr(struct dentry *dentry, struct inode *inode, }

/** - * create_file - create a file in the tracefs filesystem - * @name: the name of the file to create. + * lookup_file - look up a file in the tracefs filesystem + * @dentry: the dentry to look up * @mode: the permission that the file should have. * @attr: saved attributes changed by user - * @parent: parent dentry for this file. * @data: something that the caller will want to get to later on. * @fop: struct file_operations that should be used for this file. * @@ -292,13 +289,13 @@ static void update_inode_attr(struct dentry *dentry, struct inode *inode, * directory. The inode.i_private pointer will point to @data in the open() * call. */ -static struct dentry *create_file(const char *name, umode_t mode, +static struct dentry *lookup_file(struct dentry *dentry, + umode_t mode, struct eventfs_attr *attr, - struct dentry *parent, void *data, + void *data, const struct file_operations *fop) { struct tracefs_inode *ti; - struct dentry *dentry; struct inode *inode;

if (!(mode & S_IFMT)) @@ -307,15 +304,9 @@ static struct dentry *create_file(const char *name, umode_t mode, if (WARN_ON_ONCE(!S_ISREG(mode))) return NULL;

- WARN_ON_ONCE(!parent); - dentry = eventfs_start_creating(name, parent); - - if (IS_ERR(dentry)) - return dentry; - inode = tracefs_get_inode(dentry->d_sb); if (unlikely(!inode)) - return eventfs_failed_creating(dentry); + return ERR_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); - return eventfs_end_creating(dentry); + return dentry; };

/** - * create_dir - create a dir in the tracefs filesystem + * lookup_dir_entry - look up a dir in the tracefs filesystem + * @dentry: the directory to look up * @ei: the eventfs_inode that represents the directory to create - * @parent: parent dentry for this file. * - * This function will create a dentry for a directory represented by + * This function will look up a dentry for a directory represented by * a eventfs_inode. */ -static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent) +static struct dentry *lookup_dir_entry(struct dentry *dentry, + struct eventfs_inode *pei, struct eventfs_inode *ei) { struct tracefs_inode *ti; - struct dentry *dentry; struct inode *inode;

- dentry = eventfs_start_creating(ei->name, parent); - if (IS_ERR(dentry)) - return dentry; - inode = tracefs_get_inode(dentry->d_sb); if (unlikely(!inode)) - return eventfs_failed_creating(dentry); + return ERR_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); - return eventfs_end_creating(dentry); + return dentry; }

static void free_ei(struct eventfs_inode *ei) @@ -425,7 +416,7 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) }

/** - * create_file_dentry - create a dentry for a file of an eventfs_inode + * lookup_file_dentry - create a dentry for a file of an eventfs_inode * @ei: the eventfs_inode that the file will be created under * @idx: the index into the d_children[] of the @ei * @parent: The parent dentry of the created file. @@ -438,157 +429,21 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) * address located at @e_dentry. */ static struct dentry * -create_file_dentry(struct eventfs_inode *ei, int idx, - struct dentry *parent, const char *name, umode_t mode, void *data, +lookup_file_dentry(struct dentry *dentry, + struct eventfs_inode *ei, int idx, + umode_t mode, void *data, const struct file_operations *fops) { struct eventfs_attr *attr = NULL; struct dentry **e_dentry = &ei->d_children[idx]; - struct dentry *dentry; - - WARN_ON_ONCE(!inode_is_locked(parent->d_inode));

- mutex_lock(&eventfs_mutex); - if (ei->is_freed) { - mutex_unlock(&eventfs_mutex); - return NULL; - } - /* 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)) { - /* - * When the mutex was released, something else could have - * created the dentry for this e_dentry. In which case - * use that one. - * - * If ei->is_freed is set, the e_dentry is currently on its - * way to being freed, don't return it. If e_dentry is NULL - * it means it was already freed. - */ - if (ei->is_freed) { - dentry = NULL; - } else { - dentry = *e_dentry; - dget(dentry); - } - mutex_unlock(&eventfs_mutex); - return dentry; - } - - if (!*e_dentry && !ei->is_freed) { - *e_dentry = dentry; - dentry->d_fsdata = ei; - } else { - /* - * Should never happen unless we get here due to being freed. - * Otherwise it means two dentries exist with the same name. - */ - WARN_ON_ONCE(!ei->is_freed); - dentry = NULL; - } - mutex_unlock(&eventfs_mutex); - - return dentry; -} - -/** - * eventfs_post_create_dir - post create dir routine - * @ei: eventfs_inode of recently created dir - * - * Map the meta-data of files within an eventfs dir to their parent dentry - */ -static void eventfs_post_create_dir(struct eventfs_inode *ei) -{ - struct eventfs_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 - Create a directory dentry for the eventfs_inode - * @pei: The eventfs_inode parent of ei. - * @ei: The eventfs_inode to create the directory for - * @parent: The dentry of the parent of this directory - * - * This creates and attaches a directory dentry to the eventfs_inode @ei. - */ -static struct dentry * -create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, - struct dentry *parent) -{ - struct dentry *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); - return NULL; - } - if (ei->dentry) { - /* If the eventfs_inode already has a dentry, use it */ - dentry = ei->dentry; - dget(dentry); - mutex_unlock(&eventfs_mutex); - return dentry; - } - mutex_unlock(&eventfs_mutex); - - dentry = create_dir(ei, parent); - - mutex_lock(&eventfs_mutex); - - if (IS_ERR_OR_NULL(dentry) && !ei->is_freed) { - /* - * When the mutex was released, something else could have - * created the dentry for this e_dentry. In which case - * use that one. - * - * If ei->is_freed is set, the e_dentry is currently on its - * way to being freed. - */ - dentry = ei->dentry; - if (dentry) - dget(dentry); - mutex_unlock(&eventfs_mutex); - return dentry; - } - - if (!ei->dentry && !ei->is_freed) { - ei->dentry = dentry; - eventfs_post_create_dir(ei); - dentry->d_fsdata = ei; - } else { - /* - * Should never happen unless we get here due to being freed. - * Otherwise it means two dentries exist with the same name. - */ - WARN_ON_ONCE(!ei->is_freed); - dentry = NULL; - } - mutex_unlock(&eventfs_mutex); + *e_dentry = dentry; // Remove me

return dentry; } @@ -607,79 +462,49 @@ static struct dentry *eventfs_root_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) { - const struct file_operations *fops; - const struct eventfs_entry *entry; struct eventfs_inode *ei_child; struct tracefs_inode *ti; struct eventfs_inode *ei; - struct dentry *ei_dentry = NULL; - struct dentry *ret = NULL; - struct dentry *d; const char *name = dentry->d_name.name; - umode_t mode; - void *data; - int idx; - int i; - int r;

ti = get_tracefs(dir); if (!(ti->flags & TRACEFS_EVENT_INODE)) - return NULL; - - /* Grab srcu to prevent the ei from going away */ - idx = srcu_read_lock(&eventfs_srcu); + return ERR_PTR(-EIO);

- /* - * Grab the eventfs_mutex to consistent value from ti->private. - * This s - */ 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) goto out;

- 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) goto out; - d = create_dir_dentry(ei, ei_child, ei_dentry); - dput(d); + lookup_dir_entry(dentry, ei, ei_child); goto out; }

- 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)) - goto out; - d = create_file_dentry(ei, i, ei_dentry, name, mode, cdata, fops); - dput(d); - break; - } + for (int i = 0; i < ei->nr_entries; i++) { + void *data; + umode_t mode; + const struct file_operations *fops; + const struct eventfs_entry *entry = &ei->entries[i]; + + if (strcmp(name, entry->name) != 0) + continue; + + data = ei->data; + if (entry->callback(name, &mode, &data, &fops) <= 0) + goto out; + + lookup_file_dentry(dentry, ei, i, mode, data, fops); + goto out; } out: - srcu_read_unlock(&eventfs_srcu, idx); - return ret; + mutex_unlock(&eventfs_mutex); + return NULL; }

/* diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index ad4c137b7be9..a24dfa7fcf23 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -495,75 +495,6 @@ struct dentry *tracefs_end_creating(struct dentry *dentry) return dentry; }

-/** - * eventfs_start_creating - start the process of creating a dentry - * @name: Name of the file created for the dentry - * @parent: The parent dentry where this dentry will be created - * - * This is a simple helper function for the dynamically created eventfs - * files. When the directory of the eventfs files are accessed, their - * dentries are created on the fly. This function is used to start that - * process. - */ -struct dentry *eventfs_start_creating(const char *name, struct dentry *parent) -{ - struct dentry *dentry; - int error; - - /* Must always have a parent. */ - if (WARN_ON_ONCE(!parent)) - return ERR_PTR(-EINVAL); - - error = simple_pin_fs(&trace_fs_type, &tracefs_mount, - &tracefs_mount_count); - if (error) - return ERR_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); - - return dentry; -} - -/** - * eventfs_failed_creating - clean up a failed eventfs dentry creation - * @dentry: The dentry to clean up - * - * If after calling eventfs_start_creating(), a failure is detected, the - * resources created by eventfs_start_creating() needs to be cleaned up. In - * that case, this function should be called to perform that clean up. - */ -struct dentry *eventfs_failed_creating(struct dentry *dentry) -{ - dput(dentry); - simple_release_fs(&tracefs_mount, &tracefs_mount_count); - return NULL; -} - -/** - * eventfs_end_creating - Finish the process of creating a eventfs dentry - * @dentry: The dentry that has successfully been created. - * - * This function is currently just a place holder to match - * eventfs_start_creating(). In case any synchronization needs to be added, - * this function will be used to implement that without having to modify - * the callers of eventfs_start_creating(). - */ -struct dentry *eventfs_end_creating(struct dentry *dentry) -{ - return dentry; -} - /* Find the inode that this will use for default */ static struct inode *instance_inode(struct dentry *parent, struct inode *inode) { diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 7d84349ade87..09037e2c173d 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -80,9 +80,6 @@ struct dentry *tracefs_start_creating(const char *name, struct dentry *parent); struct dentry *tracefs_end_creating(struct dentry *dentry); struct dentry *tracefs_failed_creating(struct dentry *dentry); struct inode *tracefs_get_inode(struct super_block *sb); -struct dentry *eventfs_start_creating(const char *name, struct dentry *parent); -struct dentry *eventfs_failed_creating(struct dentry *dentry); -struct dentry *eventfs_end_creating(struct dentry *dentry); void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry);

#endif /* _TRACEFS_INTERNAL_H */

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... Link: https://lore.kernel.org/linux-trace-kernel/20240131185513.124644253@goodmis....

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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 16ca8d9759b1..b2285d5f3fed 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -378,13 +378,12 @@ static void free_ei(struct eventfs_inode *ei) }

/** - * eventfs_set_ei_status_free - remove the dentry reference from an eventfs_inode - * @ti: the tracefs_inode of the dentry + * eventfs_d_release - dentry is going away * @dentry: dentry which has the reference to remove. * * Remove the association between a dentry from an eventfs_inode. */ -void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) +void eventfs_d_release(struct dentry *dentry) { struct eventfs_inode *ei; int i; diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c index a24dfa7fcf23..99caf1c3d0c4 100644 --- a/fs/tracefs/inode.c +++ b/fs/tracefs/inode.c @@ -377,21 +377,30 @@ static const struct super_operations tracefs_super_operations = { .show_options = tracefs_show_options, };

-static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode) +/* + * It would be cleaner if eventfs had its own dentry ops. + * + * Note that d_revalidate is called potentially under RCU, + * so it can't take the eventfs mutex etc. It's fine - if + * we open a file just as it's marked dead, things will + * still work just fine, and just see the old stale case. + */ +static void tracefs_d_release(struct dentry *dentry) { - struct tracefs_inode *ti; + if (dentry->d_fsdata) + eventfs_d_release(dentry); +}

- if (!dentry || !inode) - return; +static int tracefs_d_revalidate(struct dentry *dentry, unsigned int flags) +{ + struct eventfs_inode *ei = dentry->d_fsdata;

- ti = get_tracefs(inode); - if (ti && ti->flags & TRACEFS_EVENT_INODE) - eventfs_set_ei_status_free(ti, dentry); - iput(inode); + return !(ei && ei->is_freed); }

static const struct dentry_operations tracefs_dentry_operations = { - .d_iput = tracefs_dentry_iput, + .d_revalidate = tracefs_d_revalidate, + .d_release = tracefs_d_release, };

static int trace_fill_super(struct super_block *sb, void *data, int silent) diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 932733a2696a..4b50a0668055 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -78,6 +78,7 @@ struct dentry *tracefs_start_creating(const char *name, struct dentry *parent); struct dentry *tracefs_end_creating(struct dentry *dentry); struct dentry *tracefs_failed_creating(struct dentry *dentry); struct inode *tracefs_get_inode(struct super_block *sb); -void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry); + +void eventfs_d_release(struct dentry *dentry);

#endif /* _TRACEFS_INTERNAL_H */

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....

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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 515fdace1eea..ca7daee7c811 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -73,6 +73,9 @@ enum { static void release_ei(struct kref *ref) { struct eventfs_inode *ei = container_of(ref, struct eventfs_inode, kref); + + WARN_ON_ONCE(!ei->is_freed); + kfree(ei->entry_attrs); kfree_const(ei->name); kfree_rcu(ei, rcu); @@ -84,6 +87,14 @@ static inline void put_ei(struct eventfs_inode *ei) kref_put(&ei->kref, release_ei); }

+static inline void free_ei(struct eventfs_inode *ei) +{ + if (ei) { + ei->is_freed = 1; + put_ei(ei); + } +} + static inline struct eventfs_inode *get_ei(struct eventfs_inode *ei) { if (ei) @@ -679,7 +690,7 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode

/* Was the parent freed? */ if (list_empty(&ei->list)) { - put_ei(ei); + free_ei(ei); ei = NULL; } return ei; @@ -770,7 +781,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry return ei;

fail: - put_ei(ei); + free_ei(ei); tracefs_failed_creating(dentry); return ERR_PTR(-ENOMEM); } @@ -801,9 +812,8 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, int level) list_for_each_entry(ei_child, &ei->children, list) eventfs_remove_rec(ei_child, level + 1);

- ei->is_freed = 1; list_del(&ei->list); - put_ei(ei); + free_ei(ei); }

/**

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....

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(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index ca7daee7c811..9e031e5a2713 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -366,7 +366,6 @@ static struct dentry *lookup_file(struct eventfs_inode *parent_ei, dentry->d_fsdata = get_ei(parent_ei);

d_add(dentry, inode); - fsnotify_create(dentry->d_parent->d_inode, dentry); return NULL; };

@@ -408,7 +407,6 @@ static struct dentry *lookup_dir_entry(struct dentry *dentry, inc_nlink(inode); d_add(dentry, inode); inc_nlink(dentry->d_parent->d_inode); - fsnotify_mkdir(dentry->d_parent->d_inode, dentry); return NULL; }