| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230714123537.3c397d83@gandalf.local.home>
Date: Fri, 14 Jul 2023 12:35:37 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Ajay Kaher <akaher@...are.com>
Cc: shuah@...nel.org, mhiramat@...nel.org, chinglinyu@...gle.com,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
linux-trace-kernel@...r.kernel.org, lkp@...el.com,
namit@...are.com, oe-lkp@...ts.linux.dev, amakhalov@...are.com,
er.ajay.kaher@...il.com, srivatsa@...il.mit.edu, tkundu@...are.com,
vsirnapalli@...are.com
Subject: Re: [PATCH v4 05/10] eventfs: Implement eventfs file, directory
remove function
Some more nits:
Subject:
eventfs: Implement removal of meta data from eventfs
On Thu, 13 Jul 2023 17:03:19 +0530
Ajay Kaher <akaher@...are.com> wrote:
> Adding eventfs_remove(), this function will recursively remove
> dir or file info from eventfs.
When events are removed from tracefs, the eventfs must be aware of this.
The eventfs_remove() removes the meta data from eventfs so that it will no
longer create the files associated with that event.
When an instance is removed from tracefs, eventfs_remove_events_dir() will
remove and clean up the entire "events" directory.
>
> added a recursion check to eventfs_remove_rec() as it is really
> dangerous to have unchecked recursion in the kernel (we do have
> a fixed size stack).
The above doesn't need to be in the change log. It's an update from the
previous version. The eventfs_remove_rec() is added here, so the recursion
check was not.
>
> have the free use srcu callbacks. After the srcu grace periods
> are done, it adds the eventfs_file onto a llist (lockless link
> list) and wakes up a work queue. Then the work queue does the
> freeing (this needs to be done in task/workqueue context, as
> srcu callbacks are done in softirq context).
If you want to document the above, we can say:
The helper function eventfs_remove_rec() is used to clean up and free the
associated data from eventfs for both of the added functions. SRCU is used
to protect the lists of meta data stored in the eventfs. The eventfs_mutex
is used to protect the content of the items in the list.
As lookups may be happening as deletions of events are made, the freeing
of of dentry/inodes and relative information is done after the SRCU grace
period has passed. As the callback of SRCU is in a softirq context, a work
queue is added to perform the cleanups in a task context.
The struct evenfs_file is given a union of an rcu_head and a llist_node.
The SRCU callback uses the rcu_head from this structure to insert it into
the SRCU queue. When the SRCU grace periods are complete, the callback
will then insert the eventfs_file struct onto a lockless llist using the
llist_node of the structure. A union is used as this process is just a
hand off from SRCU to workqueue, and only one field is necessary for this
to work.
You can also add the above as a comment in the code (and keep it in the
change log as well).
-- Steve
>
> Signed-off-by: Ajay Kaher <akaher@...are.com>
> Co-developed-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> Tested-by: Ching-lin Yu <chinglinyu@...gle.com>
> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202305030611.Kas747Ev-lkp@intel.com/
> ---
> fs/tracefs/event_inode.c | 110 +++++++++++++++++++++++++++++++++++++++
> include/linux/tracefs.h | 4 ++
> 2 files changed, 114 insertions(+)
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 75dc8953d813..322a77be5a56 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -45,6 +45,7 @@ struct eventfs_file {
> };
>
> static DEFINE_MUTEX(eventfs_mutex);
> +DEFINE_STATIC_SRCU(eventfs_srcu);
>
> static const struct file_operations eventfs_file_operations = {
> };
> @@ -299,3 +300,112 @@ int eventfs_add_file(const char *name, umode_t mode,
> mutex_unlock(&eventfs_mutex);
> return 0;
> }
> +
> +static LLIST_HEAD(free_list);
> +
> +static void eventfs_workfn(struct work_struct *work)
> +{
> + struct eventfs_file *ef, *tmp;
> + struct llist_node *llnode;
> +
> + llnode = llist_del_all(&free_list);
> + llist_for_each_entry_safe(ef, tmp, llnode, llist) {
> + if (ef->created && ef->dentry)
> + dput(ef->dentry);
> + kfree(ef->name);
> + kfree(ef->ei);
> + kfree(ef);
> + }
> +}
> +
> +DECLARE_WORK(eventfs_work, eventfs_workfn);
> +
> +static void free_ef(struct rcu_head *head)
> +{
> + struct eventfs_file *ef = container_of(head, struct eventfs_file, rcu);
> +
> + if (!llist_add(&ef->llist, &free_list))
> + return;
> +
> + queue_work(system_unbound_wq, &eventfs_work);
> +}
> +
> +
> +
> +/**
> + * eventfs_remove_rec - remove eventfs dir or file from list
> + * @ef: eventfs_file to be removed.
> + *
> + * This function recursively remove eventfs_file which
> + * contains info of file or dir.
> + */
> +static void eventfs_remove_rec(struct eventfs_file *ef, int level)
> +{
> + struct eventfs_file *ef_child;
> +
> + if (!ef)
> + return;
> + /*
> + * Check recursion depth. It should never be greater than 3:
> + * 0 - events/
> + * 1 - events/group/
> + * 2 - events/group/event/
> + * 3 - events/group/event/file
> + */
> + 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, level + 1);
> + }
> + }
> +
> + if (ef->created && ef->dentry)
> + d_invalidate(ef->dentry);
> +
> + list_del_rcu(&ef->list);
> + call_srcu(&eventfs_srcu, &ef->rcu, free_ef);
> +}
> +
> +/**
> + * eventfs_remove - remove eventfs dir or file from list
> + * @ef: eventfs_file to be removed.
> + *
> + * This function acquire the eventfs_mutex lock and call eventfs_remove_rec()
> + */
> +void eventfs_remove(struct eventfs_file *ef)
> +{
> + if (!ef)
> + return;
> +
> + mutex_lock(&eventfs_mutex);
> + eventfs_remove_rec(ef, 0);
> + mutex_unlock(&eventfs_mutex);
> +}
> +
> +/**
> + * eventfs_remove_events_dir - remove eventfs dir or file from list
> + * @dentry: events's dentry to be removed.
> + *
> + * This function remove events main directory
> + */
> +void eventfs_remove_events_dir(struct dentry *dentry)
> +{
> + struct tracefs_inode *ti;
> + struct eventfs_inode *ei;
> +
> + if (!dentry || !dentry->d_inode)
> + return;
> +
> + ti = get_tracefs(dentry->d_inode);
> + if (!ti || !(ti->flags & TRACEFS_EVENT_INODE))
> + return;
> +
> + ei = ti->private;
> + d_invalidate(dentry);
> + dput(dentry);
> + kfree(ei);
> +}
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index a51312ff803c..2c08edd4a739 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -40,6 +40,10 @@ int eventfs_add_top_file(const char *name, umode_t mode,
> struct dentry *parent, void *data,
> const struct file_operations *fops);
>
> +void eventfs_remove(struct eventfs_file *ef);
> +
> +void eventfs_remove_events_dir(struct dentry *dentry);
> +
> struct dentry *tracefs_create_file(const char *name, umode_t mode,
> struct dentry *parent, void *data,
> const struct file_operations *fops);
Powered by blists - more mailing lists