On Tue, 30 Jan 2024 11:03:51 -0800
Linus Torvalds <[email protected]> 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 <[email protected]>
> ---
> 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;
