| 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: <20250605100458.dc8c85fec33d8f8ca54dbc44@kernel.org>
Date: Thu, 5 Jun 2025 10:04:58 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux Trace Kernel
<linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu
<mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH] eventfs: Simplify code using guard()s
On Wed, 4 Jun 2025 15:16:25 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
>
> Use guard(mutex), scoped_guard(mutex) and guard(src) to simplify the code
> and remove a lot of the jumps to "out:" labels.
>
Looks good to me.
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
Thanks!
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
> fs/tracefs/event_inode.c | 96 +++++++++++++++-------------------------
> 1 file changed, 36 insertions(+), 60 deletions(-)
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 8705c77a9e75..117110fbabbf 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -180,29 +180,25 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
> const char *name;
> int ret;
>
> - mutex_lock(&eventfs_mutex);
> + guard(mutex)(&eventfs_mutex);
> ei = dentry->d_fsdata;
> - if (ei->is_freed) {
> - /* Do not allow changes if the event is about to be removed. */
> - mutex_unlock(&eventfs_mutex);
> + /* Do not allow changes if the event is about to be removed. */
> + if (ei->is_freed)
> return -ENODEV;
> - }
>
> /* Preallocate the children mode array if necessary */
> if (!(dentry->d_inode->i_mode & S_IFDIR)) {
> if (!ei->entry_attrs) {
> ei->entry_attrs = kcalloc(ei->nr_entries, sizeof(*ei->entry_attrs),
> GFP_NOFS);
> - if (!ei->entry_attrs) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + if (!ei->entry_attrs)
> + return -ENOMEM;
> }
> }
>
> ret = simple_setattr(idmap, dentry, iattr);
> if (ret < 0)
> - goto out;
> + return ret;
>
> /*
> * If this is a dir, then update the ei cache, only the file
> @@ -225,8 +221,6 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
> }
> }
> }
> - out:
> - mutex_unlock(&eventfs_mutex);
> return ret;
> }
>
> @@ -528,26 +522,24 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
> struct tracefs_inode *ti;
> struct eventfs_inode *ei;
> const char *name = dentry->d_name.name;
> - struct dentry *result = NULL;
>
> ti = get_tracefs(dir);
> if (WARN_ON_ONCE(!(ti->flags & TRACEFS_EVENT_INODE)))
> return ERR_PTR(-EIO);
>
> - mutex_lock(&eventfs_mutex);
> + guard(mutex)(&eventfs_mutex);
>
> ei = ti->private;
> if (!ei || ei->is_freed)
> - goto out;
> + return NULL;
>
> list_for_each_entry(ei_child, &ei->children, list) {
> if (strcmp(ei_child->name, name) != 0)
> continue;
> /* A child is freed and removed from the list at the same time */
> if (WARN_ON_ONCE(ei_child->is_freed))
> - goto out;
> - result = lookup_dir_entry(dentry, ei, ei_child);
> - goto out;
> + return NULL;
> + return lookup_dir_entry(dentry, ei, ei_child);
> }
>
> for (int i = 0; i < ei->nr_entries; i++) {
> @@ -561,14 +553,12 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>
> data = ei->data;
> if (entry->callback(name, &mode, &data, &fops) <= 0)
> - goto out;
> + return NULL;
> +
> + return lookup_file_dentry(dentry, ei, i, mode, data, fops);
>
> - result = lookup_file_dentry(dentry, ei, i, mode, data, fops);
> - goto out;
> }
> - out:
> - mutex_unlock(&eventfs_mutex);
> - return result;
> + return NULL;
> }
>
> /*
> @@ -584,7 +574,6 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
> struct eventfs_inode *ei;
> const char *name;
> umode_t mode;
> - int idx;
> int ret = -EINVAL;
> int ino;
> int i, r, c;
> @@ -598,16 +587,13 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
>
> c = ctx->pos - 2;
>
> - idx = srcu_read_lock(&eventfs_srcu);
> + guard(srcu)(&eventfs_srcu);
>
> - mutex_lock(&eventfs_mutex);
> - ei = READ_ONCE(ti->private);
> - if (ei && ei->is_freed)
> - ei = NULL;
> - mutex_unlock(&eventfs_mutex);
> -
> - if (!ei)
> - goto out;
> + scoped_guard(mutex, &eventfs_mutex) {
> + ei = READ_ONCE(ti->private);
> + if (!ei || ei->is_freed)
> + return -EINVAL;
> + }
>
> /*
> * Need to create the dentries and inodes to have a consistent
> @@ -622,21 +608,19 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
> entry = &ei->entries[i];
> name = entry->name;
>
> - mutex_lock(&eventfs_mutex);
> /* If ei->is_freed then just bail here, nothing more to do */
> - if (ei->is_freed) {
> - mutex_unlock(&eventfs_mutex);
> - goto out;
> + scoped_guard(mutex, &eventfs_mutex) {
> + if (ei->is_freed)
> + return -EINVAL;
> + r = entry->callback(name, &mode, &cdata, &fops);
> }
> - r = entry->callback(name, &mode, &cdata, &fops);
> - mutex_unlock(&eventfs_mutex);
> if (r <= 0)
> continue;
>
> ino = EVENTFS_FILE_INODE_INO;
>
> if (!dir_emit(ctx, name, strlen(name), ino, DT_REG))
> - goto out;
> + return -EINVAL;
> }
>
> /* Subtract the skipped entries above */
> @@ -659,19 +643,13 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
>
> ino = eventfs_dir_ino(ei_child);
>
> - if (!dir_emit(ctx, name, strlen(name), ino, DT_DIR))
> - goto out_dec;
> + if (!dir_emit(ctx, name, strlen(name), ino, DT_DIR)) {
> + /* Incremented ctx->pos without adding something, reset it */
> + ctx->pos--;
> + return -EINVAL;
> + }
> }
> - ret = 1;
> - out:
> - srcu_read_unlock(&eventfs_srcu, idx);
> -
> - return ret;
> -
> - out_dec:
> - /* Incremented ctx->pos without adding something, reset it */
> - ctx->pos--;
> - goto out;
> + return 1;
> }
>
> /**
> @@ -728,11 +706,10 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
> INIT_LIST_HEAD(&ei->children);
> INIT_LIST_HEAD(&ei->list);
>
> - mutex_lock(&eventfs_mutex);
> - if (!parent->is_freed)
> - list_add_tail(&ei->list, &parent->children);
> - mutex_unlock(&eventfs_mutex);
> -
> + scoped_guard(mutex, &eventfs_mutex) {
> + if (!parent->is_freed)
> + list_add_tail(&ei->list, &parent->children);
> + }
> /* Was the parent freed? */
> if (list_empty(&ei->list)) {
> cleanup_ei(ei);
> @@ -877,9 +854,8 @@ void eventfs_remove_dir(struct eventfs_inode *ei)
> if (!ei)
> return;
>
> - mutex_lock(&eventfs_mutex);
> + guard(mutex)(&eventfs_mutex);
> eventfs_remove_rec(ei, 0);
> - mutex_unlock(&eventfs_mutex);
> }
>
> /**
> --
> 2.47.2
>
--
Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists