Skip to contentSkip to conversation

eventfs: Some more minor fixes

PATCH In review Steven Rostedt 2 participants 7 messages Nov 21, 2023 – Nov 22, 2023

Subsystems (2)
VFS Tracing
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover eventfs: Some more minor fixes

Mark Rutland reported some crashes from the latest eventfs updates. This fixes most of them.

He still has one splat that he can trigger but I can not. Still looking into that.

Steven Rostedt (Google) (4):
eventfs: Use GFP_NOFS for allocation when eventfs_mutex is held
eventfs: Move taking of inode_lock into dcache_dir_open_wrapper()
eventfs: Do not allow NULL parent to eventfs_start_creating()
eventfs: Make sure that parent->d_inode is locked in creating files/dirs

----

 fs/tracefs/event_inode.c | 24 ++++++++----------------
 fs/tracefs/inode.c       | 13 ++++---------
 2 files changed, 12 insertions(+), 25 deletions(-)
author maintainer
#1 lore raw

PATCH 1/4 eventfs: Use GFP_NOFS for allocation when eventfs_mutex is held

From: "Steven Rostedt (Google)" <[email protected]>

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

Fixes 28e12c09f5aa0 ("eventfs: Save ownership and mode")
Fixes 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Reported-by Mark Rutland <[email protected]>
Signed-off-by Steven Rostedt (Google) <[email protected]>
---
 fs/tracefs/event_inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/tracefs/event_inode.c +2 -2
index 3eb6c622a74d..56d192f0ead8 100644
@@ -95,7 +95,7 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
 	if (!(dentry->d_inode->i_mode & S_IFDIR)) {
 		if (!ei->entry_attrs) {
 			ei->entry_attrs = kzalloc(sizeof(*ei->entry_attrs) * ei->nr_entries,
-						  GFP_KERNEL);
+						  GFP_NOFS);
 			if (!ei->entry_attrs) {
 				ret = -ENOMEM;
 				goto out;

@@ -627,7 +627,7 @@ static int add_dentries(struct dentry ***dentries, struct dentry *d, int cnt)
 {
 	struct dentry **tmp;
 
-	tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_KERNEL);
+	tmp = krealloc(*dentries, sizeof(d) * (cnt + 2), GFP_NOFS);
 	if (!tmp)
 		return -1;
 	tmp[cnt] = d;
Signature · 1 line
2.42.0
author maintainer
#2 lore raw

PATCH 2/4 eventfs: Move taking of inode_lock into dcache_dir_open_wrapper()

In reply to Steven Rostedt

From: "Steven Rostedt (Google)" <[email protected]>

The both create_file_dentry() and create_dir_dentry() takes a boolean parameter "lookup", as on lookup the inode_lock should already be taken, but for dcache_dir_open_wrapper() it is not taken.

There's no reason that the dcache_dir_open_wrapper() can't take the inode_lock before calling these functions. In fact, it's better if it does, as the lock can be held throughout both directory and file creations.

This also simplifies the code, and possibly prevents unexpected race conditions when the lock is released.

Fixes 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Signed-off-by Steven Rostedt (Google) <[email protected]>
---
 fs/tracefs/event_inode.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)
fs/tracefs/event_inode.c +2 -14
index 56d192f0ead8..590e8176449b 100644
@@ -347,15 +347,8 @@ create_file_dentry(struct eventfs_inode *ei, int 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, attr, parent, data, fops);
 
-	if (!lookup)
-		inode_unlock(parent->d_inode);
-
 	mutex_lock(&eventfs_mutex);
 
 	if (IS_ERR_OR_NULL(dentry)) {

@@ -453,15 +446,8 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
 	}
 	mutex_unlock(&eventfs_mutex);
 
-	/* The lookup already has the parent->d_inode locked */
-	if (!lookup)
-		inode_lock(parent->d_inode);
-
 	dentry = create_dir(ei, parent);
 
-	if (!lookup)
-		inode_unlock(parent->d_inode);
-
 	mutex_lock(&eventfs_mutex);
 
 	if (IS_ERR_OR_NULL(dentry) && !ei->is_freed) {

@@ -693,6 +679,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	}
 
+	inode_lock(parent->d_inode);
 	list_for_each_entry_srcu(ei_child, &ei->children, list,
 				 srcu_read_lock_held(&eventfs_srcu)) {
 		d = create_dir_dentry(ei, ei_child, parent, false);

@@ -725,6 +712,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
 			cnt++;
 		}
 	}
+	inode_unlock(parent->d_inode);
 	srcu_read_unlock(&eventfs_srcu, idx);
 	ret = dcache_dir_open(inode, file);
Signature · 1 line
2.42.0
author maintainer
#3 lore raw

PATCH 3/4 eventfs: Do not allow NULL parent to eventfs_start_creating()

In reply to Steven Rostedt

From: "Steven Rostedt (Google)" <[email protected]>

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

Signed-off-by Steven Rostedt (Google) <[email protected]>
---
 fs/tracefs/inode.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)
fs/tracefs/inode.c +4 -9
index 5b54948514fe..ae648deed019 100644
@@ -509,20 +509,15 @@ struct dentry *eventfs_start_creating(const char *name, struct dentry *parent)
 	struct dentry *dentry;
 	int error;
 
+	/* Must always have a parent. */
+	if (WARN_ON_ONCE(!parent))
+		return ERR_PTR(-EINVAL);
+
 	error = simple_pin_fs(&trace_fs_type, &tracefs_mount,
 			      &tracefs_mount_count);
 	if (error)
 		return ERR_PTR(error);
 
-	/*
-	 * If the parent is not specified, we create it in the root.
-	 * We need the root dentry to do this, which is in the super
-	 * block. A pointer to that is in the struct vfsmount that we
-	 * have around.
-	 */
-	if (!parent)
-		parent = tracefs_mount->mnt_root;
-
 	if (unlikely(IS_DEADDIR(parent->d_inode)))
 		dentry = ERR_PTR(-ENOENT);
 	else
Signature · 1 line
2.42.0
author maintainer
#4 lore raw

PATCH 4/4 eventfs: Make sure that parent->d_inode is locked in creating files/dirs

In reply to Steven Rostedt

From: "Steven Rostedt (Google)" <[email protected]>

Since the locking of the parent->d_inode has been moved outside the creation of the files and directories (as it use to be locked via a conditional), add a WARN_ON_ONCE() to the case that it's not locked.

Signed-off-by Steven Rostedt (Google) <[email protected]>
---
 fs/tracefs/event_inode.c | 4 ++++
 1 file changed, 4 insertions(+)
fs/tracefs/event_inode.c +4 -0
index 590e8176449b..0b90869fd805 100644
@@ -327,6 +327,8 @@ create_file_dentry(struct eventfs_inode *ei, int idx,
 	struct dentry **e_dentry = &ei->d_children[idx];
 	struct dentry *dentry;
 
+	WARN_ON_ONCE(!inode_is_locked(parent->d_inode));
+
 	mutex_lock(&eventfs_mutex);
 	if (ei->is_freed) {
 		mutex_unlock(&eventfs_mutex);

@@ -430,6 +432,8 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
 {
 	struct dentry *dentry = NULL;
 
+	WARN_ON_ONCE(!inode_is_locked(parent->d_inode));
+
 	mutex_lock(&eventfs_mutex);
 	if (pei->is_freed || ei->is_freed) {
 		mutex_unlock(&eventfs_mutex);
Signature · 1 line
2.42.0
author maintainer
#6 lore raw
Quoted 8 lines · On Wed, 22 Nov 2023 09:19:25 -0500 Josef Bacik <[email protected]> wrote:
Quoted 5 lines · Steven Rostedt

Mark Rutland reported some crashes from the latest eventfs updates. This fixes most of them.

He still has one splat that he can trigger but I can not. Still looking into that.

Reviewed-by Josef Bacik <[email protected]>

Thanks Josef!

-- Steve