From 5790b1fb3d672d9a1fe3881a7181dfdbe741568f Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 4 Oct 2023 16:50:07 -0400 Subject: eventfs: Remove eventfs_file and just use eventfs_inode 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: : [ * = ] 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: : [ * = ] 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.local.home Link: https://lore.kernel.org/linux-trace-kernel/20231004165007.43d79161@gandalf.local.home Cc: Masami Hiramatsu Cc: Mark Rutland Cc: Andrew Morton Cc: Ajay Kaher Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 847 ++++++++++++++++++++++++----------------------- fs/tracefs/inode.c | 2 +- fs/tracefs/internal.h | 37 ++- 3 files changed, 469 insertions(+), 417 deletions(-) (limited to 'fs/tracefs') 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) + * Copyright (C) 2020-23 VMware Inc, author: Steven Rostedt * Copyright (C) 2020-23 VMware Inc, author: Ajay Kaher + * Copyright (C) 2023 Google, author: Steven Rostedt * * eventfs is used to dynamically create inodes and dentries based on the * meta data provided by the tracing system. @@ -23,46 +24,6 @@ #include #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 */ -- cgit From 2819f23ac12ce93ff79ca7a54597df9a4a1f6331 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Thu, 5 Oct 2023 09:13:48 -0400 Subject: eventfs: Use eventfs_remove_events_dir() The update to removing the eventfs_file changed the way the events top level directory was handled. Instead of returning a dentry, it now returns the eventfs_inode. In this changed, the removing of the events top level directory is not much different than removing any of the other directories. Because of this, the removal just called eventfs_remove_dir() instead of eventfs_remove_events_dir(). Although eventfs_remove_dir() does the clean up, it misses out on the dget() of the ei->dentry done in eventfs_create_events_dir(). It makes more sense to match eventfs_create_events_dir() with a specific function eventfs_remove_events_dir() and this specific function can then perform the dput() to the dentry that had the dget() when it was created. Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202310051743.y9EobbUr-lkp@intel.com/ Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index eab18b157ef5..1ccd100bc565 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -901,22 +901,17 @@ void eventfs_remove_dir(struct eventfs_inode *ei) } /** - * eventfs_remove_events_dir - remove eventfs dir or file from list - * @dentry: events's dentry to be removed. + * eventfs_remove_events_dir - remove the top level eventfs directory + * @ei: the event_inode returned by eventfs_create_events_dir(). * - * This function remove events main directory + * This function removes the events main directory */ -void eventfs_remove_events_dir(struct dentry *dentry) +void eventfs_remove_events_dir(struct eventfs_inode *ei) { - struct tracefs_inode *ti; - - if (!dentry || !dentry->d_inode) - return; + struct dentry *dentry = ei->dentry; - ti = get_tracefs(dentry->d_inode); - if (!ti || !(ti->flags & TRACEFS_EVENT_INODE)) - return; + eventfs_remove_dir(ei); - d_invalidate(dentry); + /* Matches the dget() from eventfs_create_events_dir() */ dput(dentry); } -- cgit From b8a555dc31e5aa18d976de0bc228006e398a2e7d Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Wed, 18 Oct 2023 11:10:31 -0700 Subject: eventfs: Use ERR_CAST() in eventfs_create_events_dir() 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-randstruct-v1-1-338cb214abfb@kernel.org Closes: https://github.com/ClangBuiltLinux/linux/issues/1947 Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reviewed-by: Kees Cook Signed-off-by: Nathan Chancellor Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/tracefs') 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) -- cgit From 7e8ad67c9b5c11e990c320ed7e7563f2301672a7 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Thu, 19 Oct 2023 20:41:32 -0400 Subject: eventfs: Fix failure path in eventfs_create_events_dir() 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.local.home Cc: Masami Hiramatsu Cc: Mark Rutland Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: Julia Lawall Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs/tracefs') 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); } -- cgit From 64bf2f685c795e75dd855761c75a193ee5998731 Mon Sep 17 00:00:00 2001 From: Jiapeng Chong Date: Thu, 19 Oct 2023 11:13:53 +0800 Subject: tracefs/eventfs: Modify mismatched function name No functional modification involved. fs/tracefs/event_inode.c:864: warning: expecting prototype for eventfs_remove(). Prototype was for eventfs_remove_dir() instead. Link: https://lore.kernel.org/linux-trace-kernel/20231019031353.73846-1-jiapeng.chong@linux.alibaba.com Reported-by: Abaci Robot Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=6939 Signed-off-by: Jiapeng Chong Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 1885f1f1f339..09ab93357957 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -856,7 +856,7 @@ static void unhook_dentry(struct dentry **dentry, struct dentry **list) } } /** - * eventfs_remove - remove eventfs dir or file from list + * eventfs_remove_dir - remove eventfs dir or file from list * @ei: eventfs_inode to be removed. * * This function acquire the eventfs_mutex lock and call eventfs_remove_rec() -- cgit From a9de4eb15ad430fe45747c211e367da745a90093 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Tue, 24 Oct 2023 12:36:28 -0400 Subject: eventfs: Fix WARN_ON() in create_file_dentry() 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.local.home Cc: Masami Hiramatsu Cc: Mark Rutland Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs/tracefs') 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); -- cgit From 29e06c10702e81a7d0b75020ca514d2f2962704a Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Tue, 24 Oct 2023 13:10:24 -0400 Subject: eventfs: Fix typo in eventfs_inode union comment It's eventfs_inode not eventfs_indoe. There's no deer involved! Link: https://lore.kernel.org/linux-trace-kernel/20231024131024.5634c743@gandalf.local.home Cc: Masami Hiramatsu Cc: Mark Rutland Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 298d3ecaf621..64fde9490f52 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -37,7 +37,7 @@ struct eventfs_inode { /* * Union - used for deletion * @del_list: list of eventfs_inode to delete - * @rcu: eventfs_indoe to delete in RCU + * @rcu: eventfs_inode to delete in RCU * @is_freed: node is freed if one of the above is set */ union { -- cgit From 77bc4d4921bd3497678ba8e7f4e480de35692f05 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Tue, 31 Oct 2023 12:42:29 -0400 Subject: eventfs: Remove extra dget() in eventfs_create_events_dir() 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.local.home Cc: Masami Hiramatsu Cc: Mark Rutland Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'fs/tracefs') 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: -- cgit From 9037caa09ed345b35325200f0e4acf5a94ae0a65 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Mon, 30 Oct 2023 12:15:23 -0400 Subject: eventfs: Fix kerneldoc of eventfs_remove_rec() The eventfs_remove_rec() had some missing parameters in the kerneldoc comment above it. Also, rephrase the description a bit more to have a bit more correct grammar. Link: https://lore.kernel.org/linux-trace-kernel/20231030121523.0b2225a7@gandalf.local.home Cc: Masami Hiramatsu Cc: Mark Rutland Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode"); Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202310052216.4SgqasWo-lkp@intel.com/ Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 5536860eb2ff..9f612a8f009d 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -796,9 +796,11 @@ static void free_ei(struct rcu_head *head) /** * eventfs_remove_rec - remove eventfs dir or file from list * @ei: eventfs_inode to be removed. + * @head: the list head to place the deleted @ei and children + * @level: prevent recursion from going more than 3 levels deep. * - * This function recursively remove eventfs_inode which - * contains info of file or dir. + * This function recursively removes eventfs_inodes which + * contains info of files and/or directories. */ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, int level) { -- cgit From f2f496370afcbc5227d7002da28c74b91fed12ff Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:42 -0400 Subject: eventfs: Remove "is_freed" union with rcu head 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 Cc: Andrew Morton Cc: Ajay Kaher Fixes: 63940449555e7 ("eventfs: Implement eventfs lookup, read, open functions") Reviewed-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 2 ++ fs/tracefs/internal.h | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'fs/tracefs') 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) -- cgit From db3a397209b00d2e4e0a068608e5c546fc064b82 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:43 -0400 Subject: eventfs: Have a free_ei() that just frees the eventfs_inode As the eventfs_inode is freed in two different locations, make a helper function free_ei() to make sure all the allocated fields of the eventfs_inode is freed. This requires renaming the existing free_ei() which is called by the srcu handler to free_rcu_ei() and have free_ei() just do the freeing, where free_rcu_ei() will call it. Link: https://lkml.kernel.org/r/20231101172649.265214087@goodmis.org Cc: Ajay Kaher Cc: Mark Rutland Cc: Andrew Morton Reviewed-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 1ce73acf3df0..dd5971855732 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -129,6 +129,13 @@ static struct dentry *create_dir(const char *name, struct dentry *parent) return eventfs_end_creating(dentry); } +static void free_ei(struct eventfs_inode *ei) +{ + kfree_const(ei->name); + kfree(ei->d_children); + kfree(ei); +} + /** * eventfs_set_ei_status_free - remove the dentry reference from an eventfs_inode * @ti: the tracefs_inode of the dentry @@ -168,9 +175,7 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) eventfs_remove_dir(ei_child); } - kfree_const(ei->name); - kfree(ei->d_children); - kfree(ei); + free_ei(ei); return; } @@ -784,13 +789,11 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry return ERR_PTR(-ENOMEM); } -static void free_ei(struct rcu_head *head) +static void free_rcu_ei(struct rcu_head *head) { struct eventfs_inode *ei = container_of(head, struct eventfs_inode, rcu); - kfree_const(ei->name); - kfree(ei->d_children); - kfree(ei); + free_ei(ei); } /** @@ -881,7 +884,7 @@ void eventfs_remove_dir(struct eventfs_inode *ei) 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); + call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei); } mutex_unlock(&eventfs_mutex); -- cgit From 77a06c33a22d13f3a6e31f06f6ee6bca666e6898 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:44 -0400 Subject: eventfs: Test for ei->is_freed when accessing ei->dentry 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+_X85Cg@mail.gmail.com/ Link: https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunSJf=MTe=6aA@mail.gmail.com/ Link: https://lkml.kernel.org/r/20231101172649.477608228@goodmis.org Cc: Ajay Kaher Cc: Mark Rutland Cc: Andrew Morton Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: Linux Kernel Functional Testing Reported-by: Naresh Kamboju Reported-by: Beau Belgrave Reviewed-by: Masami Hiramatsu (Google) Tested-by: Linux Kernel Functional Testing Tested-by: Naresh Kamboju Tested-by: Beau Belgrave Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 45 +++++++++++++++++++++++++++++++++++++++------ fs/tracefs/internal.h | 3 ++- 2 files changed, 41 insertions(+), 7 deletions(-) (limited to 'fs/tracefs') 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 #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; -- cgit From 28e12c09f5aa081b2d13d1340e3610070b6c624d Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:45 -0400 Subject: eventfs: Save ownership and mode Now that inodes and dentries are created on the fly, they are also reclaimed on memory pressure. Since the ownership and file mode are saved in the inode, if they are freed, any changes to the ownership and mode will be lost. To counter this, if the user changes the permissions or ownership, save them, and when creating the inodes again, restore those changes. Link: https://lkml.kernel.org/r/20231101172649.691841445@goodmis.org Cc: stable@vger.kernel.org Cc: Ajay Kaher Cc: Mark Rutland Cc: Andrew Morton Fixes: 63940449555e7 ("eventfs: Implement eventfs lookup, read, open functions") Reviewed-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 148 ++++++++++++++++++++++++++++++++++++++++++----- fs/tracefs/internal.h | 16 +++++ 2 files changed, 151 insertions(+), 13 deletions(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index e9625732c52d..93d08e552483 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -40,6 +40,15 @@ 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); @@ -47,8 +56,89 @@ 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_attr *attr, struct iattr *iattr) +{ + unsigned int ia_valid = iattr->ia_valid; + + if (ia_valid & ATTR_MODE) { + attr->mode = (attr->mode & ~EVENTFS_MODE_MASK) | + (iattr->ia_mode & EVENTFS_MODE_MASK) | + EVENTFS_SAVE_MODE; + } + if (ia_valid & ATTR_UID) { + attr->mode |= EVENTFS_SAVE_UID; + attr->uid = iattr->ia_uid; + } + if (ia_valid & ATTR_GID) { + attr->mode |= EVENTFS_SAVE_GID; + attr->gid = iattr->ia_gid; + } +} + +static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry, + struct iattr *iattr) +{ + const struct eventfs_entry *entry; + struct eventfs_inode *ei; + const char *name; + int ret; + + mutex_lock(&eventfs_mutex); + ei = dentry->d_fsdata; + /* The LSB is set when the eventfs_inode is being freed */ + if (((unsigned long)ei & 1UL) || ei->is_freed) { + /* Do not allow changes if the event is about to be removed. */ + mutex_unlock(&eventfs_mutex); + return -ENODEV; + } + + /* 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, + GFP_KERNEL); + if (!ei->entry_attrs) { + ret = -ENOMEM; + goto out; + } + } + } + + ret = simple_setattr(idmap, dentry, iattr); + if (ret < 0) + goto out; + + /* + * If this is a dir, then update the ei cache, only the file + * mode is saved in the ei->m_children, and the ownership is + * determined by the parent directory. + */ + if (dentry->d_inode->i_mode & S_IFDIR) { + update_attr(&ei->attr, iattr); + + } else { + name = dentry->d_name.name; + + for (int i = 0; i < ei->nr_entries; i++) { + entry = &ei->entries[i]; + if (strcmp(name, entry->name) == 0) { + update_attr(&ei->entry_attrs[i], iattr); + break; + } + } + } + out: + 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 = { @@ -59,10 +149,30 @@ static const struct file_operations eventfs_file_operations = { .release = eventfs_release, }; +static void update_inode_attr(struct inode *inode, struct eventfs_attr *attr, umode_t mode) +{ + if (!attr) { + inode->i_mode = mode; + 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; + + if (attr->mode & EVENTFS_SAVE_GID) + inode->i_gid = attr->gid; +} + /** * create_file - create a file in the tracefs filesystem * @name: the name of the file to create. * @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. @@ -72,6 +182,7 @@ static const struct file_operations eventfs_file_operations = { * call. */ static struct dentry *create_file(const char *name, umode_t mode, + struct eventfs_attr *attr, struct dentry *parent, void *data, const struct file_operations *fop) { @@ -95,7 +206,10 @@ static struct dentry *create_file(const char *name, umode_t mode, if (unlikely(!inode)) return eventfs_failed_creating(dentry); - inode->i_mode = mode; + /* If the user updated the directory's attributes, use them */ + update_inode_attr(inode, attr, mode); + + inode->i_op = &eventfs_file_inode_operations; inode->i_fop = fop; inode->i_private = data; @@ -108,19 +222,19 @@ 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. + * @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 * a eventfs_inode. */ -static struct dentry *create_dir(const char *name, struct dentry *parent) +static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent) { struct tracefs_inode *ti; struct dentry *dentry; struct inode *inode; - dentry = eventfs_start_creating(name, parent); + dentry = eventfs_start_creating(ei->name, parent); if (IS_ERR(dentry)) return dentry; @@ -128,7 +242,9 @@ static struct dentry *create_dir(const char *name, struct dentry *parent) if (unlikely(!inode)) return eventfs_failed_creating(dentry); - inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO; + /* If the user updated the directory's attributes, use them */ + update_inode_attr(inode, &ei->attr, S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO); + inode->i_op = &eventfs_root_dir_inode_operations; inode->i_fop = &eventfs_file_operations; @@ -146,6 +262,7 @@ static void free_ei(struct eventfs_inode *ei) { kfree_const(ei->name); kfree(ei->d_children); + kfree(ei->entry_attrs); kfree(ei); } @@ -231,7 +348,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 * @ei: the eventfs_inode that the file will be created under - * @e_dentry: a pointer to the d_children[] of the @ei + * @idx: the index into 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. @@ -244,10 +361,12 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) * 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, +create_file_dentry(struct eventfs_inode *ei, int idx, struct dentry *parent, const char *name, umode_t mode, void *data, const struct file_operations *fops, bool lookup) { + struct eventfs_attr *attr = NULL; + struct dentry **e_dentry = &ei->d_children[idx]; struct dentry *dentry; bool invalidate = false; @@ -264,13 +383,18 @@ create_file_dentry(struct eventfs_inode *ei, struct dentry **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); /* The lookup already has the parent->d_inode locked */ if (!lookup) inode_lock(parent->d_inode); - dentry = create_file(name, mode, parent, data, fops); + dentry = create_file(name, mode, attr, parent, data, fops); if (!lookup) inode_unlock(parent->d_inode); @@ -378,7 +502,7 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, if (!lookup) inode_lock(parent->d_inode); - dentry = create_dir(ei->name, parent); + dentry = create_dir(ei, parent); if (!lookup) inode_unlock(parent->d_inode); @@ -495,8 +619,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir, if (r <= 0) continue; ret = simple_lookup(dir, dentry, flags); - create_file_dentry(ei, &ei->d_children[i], - ei_dentry, name, mode, cdata, + create_file_dentry(ei, i, ei_dentry, name, mode, cdata, fops, true); break; } @@ -629,8 +752,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file) 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); + d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false); if (d) { ret = add_dentries(&dentries, d, cnt); if (ret < 0) diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 5a98e87dd3d1..5f60bcd69289 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -13,6 +13,18 @@ struct tracefs_inode { struct inode vfs_inode; }; +/* + * struct eventfs_attr - cache the mode and ownership of a eventfs entry + * @mode: saved mode plus flags of what is saved + * @uid: saved uid if changed + * @gid: saved gid if changed + */ +struct eventfs_attr { + int mode; + kuid_t uid; + kgid_t gid; +}; + /* * struct eventfs_inode - hold the properties of the eventfs directories. * @list: link list into the parent directory @@ -22,6 +34,8 @@ struct tracefs_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 + * @entry_attrs: Saved mode and ownership of the @d_children + * @attr: Saved mode and ownership of eventfs_inode itself * @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. @@ -35,6 +49,8 @@ struct eventfs_inode { struct dentry *dentry; /* Check is_freed to access */ struct dentry *d_parent; struct dentry **d_children; + struct eventfs_attr *entry_attrs; + struct eventfs_attr attr; void *data; /* * Union - used for deletion -- cgit From 44365329f8219fc379097c2c9a75ff53f123764f Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:46 -0400 Subject: eventfs: Hold eventfs_mutex when calling callback functions 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=Y50WpWAE7_Q@mail.gmail.com/ Link: https://lkml.kernel.org/r/20231101172649.906696613@goodmis.org Cc: Ajay Kaher Cc: Mark Rutland Cc: Andrew Morton Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode") Reported-by: Linux Kernel Functional Testing Reported-by: Naresh Kamboju Tested-by: Linux Kernel Functional Testing Tested-by: Naresh Kamboju Reviewed-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) (limited to 'fs/tracefs') 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, -- cgit From 020010fbfa202aa528a52743eba4ab0da3400a4e Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:47 -0400 Subject: eventfs: Delete eventfs_inode when the last dentry is freed There exists a race between holding a reference of an eventfs_inode dentry and the freeing of the eventfs_inode. If user space has a dentry held long enough, it may still be able to access the dentry's eventfs_inode after it has been freed. To prevent this, have he eventfs_inode freed via the last dput() (or via RCU if the eventfs_inode does not have a dentry). This means reintroducing the eventfs_inode del_list field at a temporary place to put the eventfs_inode. It needs to mark it as freed (via the list) but also must invalidate the dentry immediately as the return from eventfs_remove_dir() expects that they are. But the dentry invalidation must not be called under the eventfs_mutex, so it must be done after the eventfs_inode is marked as free (put on a deletion list). Link: https://lkml.kernel.org/r/20231101172650.123479767@goodmis.org Cc: stable@vger.kernel.org Cc: Masami Hiramatsu Cc: Mark Rutland Cc: Andrew Morton Cc: Ajay Kaher Fixes: 5bdcd5f5331a2 ("eventfs: Implement removal of meta data from eventfs") Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 146 ++++++++++++++++++++++------------------------- fs/tracefs/internal.h | 2 + 2 files changed, 69 insertions(+), 79 deletions(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 8ac9abf7a3d5..0a04ae0ca8c8 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -85,8 +85,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry, mutex_lock(&eventfs_mutex); ei = dentry->d_fsdata; - /* The LSB is set when the eventfs_inode is being freed */ - if (((unsigned long)ei & 1UL) || ei->is_freed) { + if (ei->is_freed) { /* Do not allow changes if the event is about to be removed. */ mutex_unlock(&eventfs_mutex); return -ENODEV; @@ -276,35 +275,17 @@ 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_child, *tmp; struct eventfs_inode *ei; int i; /* The top level events directory may be freed by this */ if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) { - LIST_HEAD(ef_del_list); - mutex_lock(&eventfs_mutex); - ei = ti->private; - - /* Record all the top level files */ - list_for_each_entry_srcu(ei_child, &ei->children, list, - lockdep_is_held(&eventfs_mutex)) { - list_add_tail(&ei_child->del_list, &ef_del_list); - } - /* Nothing should access this, but just in case! */ ti->private = NULL; - mutex_unlock(&eventfs_mutex); - /* Now safely free the top level files and their children */ - list_for_each_entry_safe(ei_child, tmp, &ef_del_list, del_list) { - list_del(&ei_child->del_list); - eventfs_remove_dir(ei_child); - } - free_ei(ei); return; } @@ -319,14 +300,6 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) if (!ei) goto out; - /* - * 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)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++) { @@ -336,6 +309,8 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry) if (WARN_ON_ONCE(i == ei->nr_entries)) goto out; ei->d_children[i] = NULL; + } else if (ei->is_freed) { + free_ei(ei); } else { ei->dentry = NULL; } @@ -962,13 +937,65 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry return ERR_PTR(-ENOMEM); } +static LLIST_HEAD(free_list); + +static void eventfs_workfn(struct work_struct *work) +{ + struct eventfs_inode *ei, *tmp; + struct llist_node *llnode; + + llnode = llist_del_all(&free_list); + llist_for_each_entry_safe(ei, tmp, llnode, llist) { + /* This dput() matches the dget() from unhook_dentry() */ + for (int i = 0; i < ei->nr_entries; i++) { + if (ei->d_children[i]) + dput(ei->d_children[i]); + } + /* This should only get here if it had a dentry */ + if (!WARN_ON_ONCE(!ei->dentry)) + dput(ei->dentry); + } +} + +static DECLARE_WORK(eventfs_work, eventfs_workfn); + static void free_rcu_ei(struct rcu_head *head) { struct eventfs_inode *ei = container_of(head, struct eventfs_inode, rcu); + if (ei->dentry) { + /* Do not free the ei until all references of dentry are gone */ + if (llist_add(&ei->llist, &free_list)) + queue_work(system_unbound_wq, &eventfs_work); + return; + } + + /* If the ei doesn't have a dentry, neither should its children */ + for (int i = 0; i < ei->nr_entries; i++) { + WARN_ON_ONCE(ei->d_children[i]); + } + free_ei(ei); } +static void unhook_dentry(struct dentry *dentry) +{ + if (!dentry) + return; + + /* 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 * @ei: eventfs_inode to be removed. @@ -1006,33 +1033,6 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, 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_dir - remove eventfs dir or file from list * @ei: eventfs_inode to be removed. @@ -1043,40 +1043,28 @@ void eventfs_remove_dir(struct eventfs_inode *ei) { struct eventfs_inode *tmp; LIST_HEAD(ei_del_list); - struct dentry *dentry_list = NULL; - struct dentry *dentry; - int i; if (!ei) 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); eventfs_remove_rec(ei, &ei_del_list, 0); + mutex_unlock(&eventfs_mutex); 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); + for (int i = 0; i < ei->nr_entries; i++) + unhook_dentry(ei->d_children[i]); + unhook_dentry(ei->dentry); + list_del(&ei->del_list); call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei); } - mutex_unlock(&eventfs_mutex); - - while (dentry_list) { - unsigned long ptr; - - dentry = dentry_list; - ptr = (unsigned long)dentry->d_fsdata & ~1UL; - dentry_list = (struct dentry *)ptr; - 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); - dput(dentry); - } } /** diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 5f60bcd69289..06a1f220b901 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -54,10 +54,12 @@ struct eventfs_inode { void *data; /* * Union - used for deletion + * @llist: for calling dput() if needed after RCU * @del_list: list of eventfs_inode to delete * @rcu: eventfs_inode to delete in RCU */ union { + struct llist_node llist; struct list_head del_list; struct rcu_head rcu; }; -- cgit From 62d65cac119d08d39f751b4e3e2063ed996edc05 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:48 -0400 Subject: eventfs: Remove special processing of dput() of events directory 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 Cc: Mark Rutland Cc: Andrew Morton Reviewed-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) (limited to 'fs/tracefs') 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); -- cgit From 407c6726ca71b33330d2d6345d9ea7ebc02575e9 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (Google)" Date: Wed, 1 Nov 2023 13:25:49 -0400 Subject: eventfs: Use simple_recursive_removal() to clean up dentries Looking at how dentry is removed via the tracefs system, I found that eventfs does not do everything that it did under tracefs. The tracefs removal of a dentry calls simple_recursive_removal() that does a lot more than a simple d_invalidate(). As it should be a requirement that any eventfs_inode that has a dentry, so does its parent. When removing a eventfs_inode, if it has a dentry, a call to simple_recursive_removal() on that dentry should clean up all the dentries underneath it. Add WARN_ON_ONCE() to check for the parent having a dentry if any children do. Link: https://lore.kernel.org/all/20231101022553.GE1957730@ZenIV/ Link: https://lkml.kernel.org/r/20231101172650.552471568@goodmis.org Cc: stable@vger.kernel.org Cc: Masami Hiramatsu Cc: Mark Rutland Cc: Andrew Morton Cc: Al Viro Fixes: 5bdcd5f5331a2 ("eventfs: Implement removal of meta data from eventfs") Signed-off-by: Steven Rostedt (Google) --- fs/tracefs/event_inode.c | 77 +++++++++++++++++++++++++++--------------------- fs/tracefs/internal.h | 2 -- 2 files changed, 44 insertions(+), 35 deletions(-) (limited to 'fs/tracefs') diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 0087a3f455f1..f8a594a50ae6 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -967,30 +967,29 @@ static void unhook_dentry(struct dentry *dentry) { if (!dentry) return; - - /* Keep the dentry from being freed yet (see eventfs_workfn()) */ + /* + * Need to add a reference to the dentry that is expected by + * simple_recursive_removal(), which will include a dput(). + */ 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); + /* + * 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. + */ + dget(dentry); } /** * eventfs_remove_rec - remove eventfs dir or file from list * @ei: eventfs_inode to be removed. - * @head: the list head to place the deleted @ei and children * @level: prevent recursion from going more than 3 levels deep. * * This function recursively removes eventfs_inodes which * contains info of files and/or directories. */ -static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, int level) +static void eventfs_remove_rec(struct eventfs_inode *ei, int level) { struct eventfs_inode *ei_child; @@ -1009,13 +1008,26 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, /* 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); + /* Children only have dentry if parent does */ + WARN_ON_ONCE(ei_child->dentry && !ei->dentry); + eventfs_remove_rec(ei_child, level + 1); } + ei->is_freed = 1; + for (int i = 0; i < ei->nr_entries; i++) { + if (ei->d_children[i]) { + /* Children only have dentry if parent does */ + WARN_ON_ONCE(!ei->dentry); + unhook_dentry(ei->d_children[i]); + } + } + + unhook_dentry(ei->dentry); + list_del_rcu(&ei->list); - list_add_tail(&ei->del_list, head); + call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei); } /** @@ -1026,30 +1038,22 @@ static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head *head, */ void eventfs_remove_dir(struct eventfs_inode *ei) { - struct eventfs_inode *tmp; - LIST_HEAD(ei_del_list); + struct dentry *dentry; if (!ei) 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); - eventfs_remove_rec(ei, &ei_del_list, 0); + dentry = ei->dentry; + eventfs_remove_rec(ei, 0); mutex_unlock(&eventfs_mutex); - list_for_each_entry_safe(ei, tmp, &ei_del_list, del_list) { - for (int i = 0; i < ei->nr_entries; i++) - unhook_dentry(ei->d_children[i]); - unhook_dentry(ei->dentry); - list_del(&ei->del_list); - call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei); - } + /* + * If any of the ei children has a dentry, then the ei itself + * must have a dentry. + */ + if (dentry) + simple_recursive_removal(dentry, NULL); } /** @@ -1060,10 +1064,17 @@ void eventfs_remove_dir(struct eventfs_inode *ei) */ void eventfs_remove_events_dir(struct eventfs_inode *ei) { - struct dentry *dentry = ei->dentry; + struct dentry *dentry; + dentry = ei->dentry; eventfs_remove_dir(ei); - /* Matches the dget() from eventfs_create_events_dir() */ + /* + * Matches the dget() done by tracefs_start_creating() + * in eventfs_create_events_dir() when it the dentry was + * created. In other words, it's a normal dentry that + * sticks around while the other ei->dentry are created + * and destroyed dynamically. + */ dput(dentry); } diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h index 06a1f220b901..ccee18ca66c7 100644 --- a/fs/tracefs/internal.h +++ b/fs/tracefs/internal.h @@ -55,12 +55,10 @@ struct eventfs_inode { /* * Union - used for deletion * @llist: for calling dput() if needed after RCU - * @del_list: list of eventfs_inode to delete * @rcu: eventfs_inode to delete in RCU */ union { struct llist_node llist; - struct list_head del_list; struct rcu_head rcu; }; unsigned int is_freed:1; -- cgit