Re: [PATCH] eventfs: Remember what dentries were created on dir open

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 20 Sep 2023 22:15:37 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Using the following code with libtracefs:
> 
> 	int dfd;
> 
> 	// create the directory events/kprobes/kp1
> 	tracefs_kprobe_raw(NULL, "kp1", "schedule_timeout", "time=$arg1");
> 
> 	// Open the kprobes directory
> 	dfd = tracefs_instance_file_open(NULL, "events/kprobes", O_RDONLY);
> 
> 	// Do a lookup of the kprobes/kp1 directory (by looking at enable)
> 	tracefs_file_exists(NULL, "events/kprobes/kp1/enable");
> 
> 	// Now create a new entry in the kprobes directory
> 	tracefs_kprobe_raw(NULL, "kp2", "schedule_hrtimeout", "expires=$arg1");
> 
> 	// Do another lookup to create the dentries
> 	tracefs_file_exists(NULL, "events/kprobes/kp2/enable"))
> 
> 	// Close the directory
> 	close(dfd);
> 
> What happened above, the first open (dfd) will call
> dcache_dir_open_wrapper() that will create the dentries and up their ref
> counts.
> 
> Now the creation of "kp2" will add another dentry within the kprobes
> directory.
> 
> Upon the close of dfd, eventfs_release() will now do a dput for all the
> entries in kprobes. But this is where the problem lies. The open only
> upped the dentry of kp1 and not kp2. Now the close is decrementing both
> kp1 and kp2, which causes kp2 to get a negative count.
> 
> Doing a "trace-cmd reset" which deletes all the kprobes cause the kernel
> to crash! (due to the messed up accounting of the ref counts).
> 
> To solve this, save all the dentries that are opened in the
> dcache_dir_open_wrapper() into an array, and use this array to know what
> dentries to do a dput on in eventfs_release().
> 
> Since the dcache_dir_open_wrapper() calls dcache_dir_open() which uses the
> file->private_data, we need to also add a wrapper around dcache_readdir()
> that uses the cursor assigned to the file->private_data. This is because
> the dentries need to also be saved in the file->private_data. To do this
> create the structure:
> 
>   struct dentry_list {
> 	void		*cursor;
> 	struct dentry	**dentries;
>   };
> 
> Which will hold both the cursor and the dentries. Some shuffling around is
> needed to make sure that dcache_dir_open() and dcache_readdir() only see
> the cursor.
> 
> Link: https://lore.kernel.org/linux-trace-kernel/20230919211804.230edf1e@gandalf.local.home/

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you!

> 
> Fixes: 63940449555e7 ("eventfs: Implement eventfs lookup, read, open functions")
> Reported-by: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  fs/tracefs/event_inode.c | 87 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 70 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index b23bb0957bb4..1c5c6a1ff4cc 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -30,6 +30,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>  					  struct dentry *dentry,
>  					  unsigned int flags);
>  static int dcache_dir_open_wrapper(struct inode *inode, struct file *file);
> +static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx);
>  static int eventfs_release(struct inode *inode, struct file *file);
>  
>  static const struct inode_operations eventfs_root_dir_inode_operations = {
> @@ -39,7 +40,7 @@ static const struct inode_operations eventfs_root_dir_inode_operations = {
>  static const struct file_operations eventfs_file_operations = {
>  	.open		= dcache_dir_open_wrapper,
>  	.read		= generic_read_dir,
> -	.iterate_shared	= dcache_readdir,
> +	.iterate_shared	= dcache_readdir_wrapper,
>  	.llseek		= generic_file_llseek,
>  	.release	= eventfs_release,
>  };
> @@ -356,6 +357,11 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>  	return ret;
>  }
>  
> +struct dentry_list {
> +	void			*cursor;
> +	struct dentry		**dentries;
> +};
> +
>  /**
>   * eventfs_release - called to release eventfs file/dir
>   * @inode: inode to be released
> @@ -364,26 +370,25 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>  static int eventfs_release(struct inode *inode, struct file *file)
>  {
>  	struct tracefs_inode *ti;
> -	struct eventfs_inode *ei;
> -	struct eventfs_file *ef;
> -	struct dentry *dentry;
> -	int idx;
> +	struct dentry_list *dlist = file->private_data;
> +	void *cursor;
> +	int i;
>  
>  	ti = get_tracefs(inode);
>  	if (!(ti->flags & TRACEFS_EVENT_INODE))
>  		return -EINVAL;
>  
> -	ei = ti->private;
> -	idx = srcu_read_lock(&eventfs_srcu);
> -	list_for_each_entry_srcu(ef, &ei->e_top_files, list,
> -				 srcu_read_lock_held(&eventfs_srcu)) {
> -		mutex_lock(&eventfs_mutex);
> -		dentry = ef->dentry;
> -		mutex_unlock(&eventfs_mutex);
> -		if (dentry)
> -			dput(dentry);
> +	if (WARN_ON_ONCE(!dlist))
> +		return -EINVAL;
> +
> +	for (i = 0; dlist->dentries[i]; i++) {
> +		dput(dlist->dentries[i]);
>  	}
> -	srcu_read_unlock(&eventfs_srcu, idx);
> +
> +	cursor = dlist->cursor;
> +	kfree(dlist->dentries);
> +	kfree(dlist);
> +	file->private_data = cursor;
>  	return dcache_dir_close(inode, file);
>  }
>  
> @@ -402,22 +407,70 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
>  	struct tracefs_inode *ti;
>  	struct eventfs_inode *ei;
>  	struct eventfs_file *ef;
> +	struct dentry_list *dlist;
> +	struct dentry **dentries = NULL;
>  	struct dentry *dentry = file_dentry(file);
> +	struct dentry *d;
>  	struct inode *f_inode = file_inode(file);
> +	int cnt = 0;
>  	int idx;
> +	int ret;
>  
>  	ti = get_tracefs(f_inode);
>  	if (!(ti->flags & TRACEFS_EVENT_INODE))
>  		return -EINVAL;
>  
> +	if (WARN_ON_ONCE(file->private_data))
> +		return -EINVAL;
> +
> +	dlist = kmalloc(sizeof(*dlist), GFP_KERNEL);
> +	if (!dlist)
> +		return -ENOMEM;
> +
>  	ei = ti->private;
>  	idx = srcu_read_lock(&eventfs_srcu);
>  	list_for_each_entry_srcu(ef, &ei->e_top_files, list,
>  				 srcu_read_lock_held(&eventfs_srcu)) {
> -		create_dentry(ef, dentry, false);
> +		d = create_dentry(ef, dentry, false);
> +		if (d) {
> +			struct dentry **tmp;
> +
> +			tmp = krealloc(dentries, sizeof(d) * (cnt + 2), GFP_KERNEL);
> +			if (!tmp)
> +				break;
> +			tmp[cnt] = d;
> +			tmp[cnt + 1] = NULL;
> +			cnt++;
> +			dentries = tmp;
> +		}
>  	}
>  	srcu_read_unlock(&eventfs_srcu, idx);
> -	return dcache_dir_open(inode, file);
> +	ret = dcache_dir_open(inode, file);
> +
> +	/*
> +	 * dcache_dir_open() sets file->private_data to a dentry cursor.
> +	 * Need to save that but also save all the dentries that were
> +	 * opened by this function.
> +	 */
> +	dlist->cursor = file->private_data;
> +	dlist->dentries = dentries;
> +	file->private_data = dlist;
> +	return ret;
> +}
> +
> +/*
> + * This just sets the file->private_data back to the cursor and back.
> + */
> +static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx)
> +{
> +	struct dentry_list *dlist = file->private_data;
> +	int ret;
> +
> +	file->private_data = dlist->cursor;
> +	ret = dcache_readdir_wrapper;
> +	dlist->cursor = file->private_data;
> +	file->private_data = dlist;
> +	return ret;
>  }
>  
>  /**
> -- 
> 2.40.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>