diff mbox series

[2/6] eventfsfs: initialize the tracefs inode properly

Message ID 20240130190355.11486-2-torvalds@linux-foundation.org (mailing list archive)
State Superseded
Headers show
Series [1/6] tracefs: avoid using the ei->dentry pointer unnecessarily | expand

Commit Message

Linus Torvalds Jan. 30, 2024, 7:03 p.m. UTC 
The tracefs-specific fields in the inode were not initialized before the
inode was exposed to others through the dentry with 'd_instantiate()'.

And the ->flags file was initialized incorrectly with a '|=', when the
old value was stale.  It should have just been a straight assignment.

Move the field initializations up to before the d_instantiate, and fix
the use of uninitialized data.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/tracefs/event_inode.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Steven Rostedt Jan. 30, 2024, 7:48 p.m. UTC | #1 
On Tue, 30 Jan 2024 11:03:51 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> The tracefs-specific fields in the inode were not initialized before the
> inode was exposed to others through the dentry with 'd_instantiate()'.
> 
> And the ->flags file was initialized incorrectly with a '|=', when the
> old value was stale.  It should have just been a straight assignment.

The ti is allocated from fs/tracefs/inode.c that has:

static struct inode *tracefs_alloc_inode(struct super_block *sb)
{
	struct tracefs_inode *ti;

	ti = kmem_cache_alloc(tracefs_inode_cachep, GFP_KERNEL);
	if (!ti)
		return NULL;

	ti->flags = 0;

	return &ti->vfs_inode;
}

Shouldn't that make it valid?

Granted, the eventfs inodes don't have any of the tracefs inode flags set.
But I purposely made he ti->flags initialized to zero.

Is this update really necessary?

Or do I need to make sure that the iput() clears it?

The flags are used by tracefs, so I want to know if there's not a bug there.

-- Steve


> 
> Move the field initializations up to before the d_instantiate, and fix
> the use of uninitialized data.
> 
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>  fs/tracefs/event_inode.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 2d128bedd654..c0d977e6c0f2 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -328,7 +328,9 @@ static struct dentry *create_file(const char *name, umode_t mode,
>  	inode->i_ino = EVENTFS_FILE_INODE_INO;
>  
>  	ti = get_tracefs(inode);
> -	ti->flags |= TRACEFS_EVENT_INODE;
> +	ti->flags = TRACEFS_EVENT_INODE;
> +	ti->private = NULL;			// Directories have 'ei', files not
> +
>  	d_instantiate(dentry, inode);
>  	fsnotify_create(dentry->d_parent->d_inode, dentry);
>  	return eventfs_end_creating(dentry);
> @@ -367,7 +369,8 @@ static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent
>  	inode->i_ino = eventfs_dir_ino(ei);
>  
>  	ti = get_tracefs(inode);
> -	ti->flags |= TRACEFS_EVENT_INODE;
> +	ti->flags = TRACEFS_EVENT_INODE;
> +	ti->private = ei;
>  
>  	inc_nlink(inode);
>  	d_instantiate(dentry, inode);
> @@ -513,7 +516,6 @@ create_file_dentry(struct eventfs_inode *ei, int idx,
>  static void eventfs_post_create_dir(struct eventfs_inode *ei)
>  {
>  	struct eventfs_inode *ei_child;
> -	struct tracefs_inode *ti;
>  
>  	lockdep_assert_held(&eventfs_mutex);
>  
> @@ -523,9 +525,6 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)
>  				 srcu_read_lock_held(&eventfs_srcu)) {
>  		ei_child->d_parent = ei->dentry;
>  	}
> -
> -	ti = get_tracefs(ei->dentry->d_inode);
> -	ti->private = ei;
>  }
>  
>  /**
> @@ -943,7 +942,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
>  	INIT_LIST_HEAD(&ei->list);
>  
>  	ti = get_tracefs(inode);
> -	ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
> +	ti->flags = TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
>  	ti->private = ei;
>  
>  	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
Steven Rostedt Jan. 30, 2024, 7:49 p.m. UTC | #2 
On Tue, 30 Jan 2024 11:03:51 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> @@ -328,7 +328,9 @@ static struct dentry *create_file(const char *name, umode_t mode,
>  	inode->i_ino = EVENTFS_FILE_INODE_INO;
>  
>  	ti = get_tracefs(inode);
> -	ti->flags |= TRACEFS_EVENT_INODE;
> +	ti->flags = TRACEFS_EVENT_INODE;
> +	ti->private = NULL;			// Directories have 'ei', files not

Although ti->private does need to be initialized here.

-- Steve

> +
>  	d_instantiate(dentry, inode);
>  	fsnotify_create(dentry->d_parent->d_inode, dentry);
>  	return eventfs_end_creating(dentry);
Steven Rostedt Jan. 30, 2024, 7:56 p.m. UTC | #3 
On Tue, 30 Jan 2024 14:48:02 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
 
> The ti is allocated from fs/tracefs/inode.c that has:
> 
> static struct inode *tracefs_alloc_inode(struct super_block *sb)
> {
> 	struct tracefs_inode *ti;
> 
> 	ti = kmem_cache_alloc(tracefs_inode_cachep, GFP_KERNEL);

I could also just add __GFP_ZERO so that all of it is initialized to zero,
and then we don't need to assign NULL to any part of it.

-- Steve


> 	if (!ti)
> 		return NULL;
> 
> 	ti->flags = 0;
> 
> 	return &ti->vfs_inode;
> }
> 
>
diff mbox series

Patch

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 2d128bedd654..c0d977e6c0f2 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -328,7 +328,9 @@  static struct dentry *create_file(const char *name, umode_t mode,
 	inode->i_ino = EVENTFS_FILE_INODE_INO;
 
 	ti = get_tracefs(inode);
-	ti->flags |= TRACEFS_EVENT_INODE;
+	ti->flags = TRACEFS_EVENT_INODE;
+	ti->private = NULL;			// Directories have 'ei', files not
+
 	d_instantiate(dentry, inode);
 	fsnotify_create(dentry->d_parent->d_inode, dentry);
 	return eventfs_end_creating(dentry);
@@ -367,7 +369,8 @@  static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent
 	inode->i_ino = eventfs_dir_ino(ei);
 
 	ti = get_tracefs(inode);
-	ti->flags |= TRACEFS_EVENT_INODE;
+	ti->flags = TRACEFS_EVENT_INODE;
+	ti->private = ei;
 
 	inc_nlink(inode);
 	d_instantiate(dentry, inode);
@@ -513,7 +516,6 @@  create_file_dentry(struct eventfs_inode *ei, int idx,
 static void eventfs_post_create_dir(struct eventfs_inode *ei)
 {
 	struct eventfs_inode *ei_child;
-	struct tracefs_inode *ti;
 
 	lockdep_assert_held(&eventfs_mutex);
 
@@ -523,9 +525,6 @@  static void eventfs_post_create_dir(struct eventfs_inode *ei)
 				 srcu_read_lock_held(&eventfs_srcu)) {
 		ei_child->d_parent = ei->dentry;
 	}
-
-	ti = get_tracefs(ei->dentry->d_inode);
-	ti->private = ei;
 }
 
 /**
@@ -943,7 +942,7 @@  struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
 	INIT_LIST_HEAD(&ei->list);
 
 	ti = get_tracefs(inode);
-	ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
+	ti->flags = TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
 	ti->private = ei;
 
 	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;