Re: [PATCH v6 5/8] eventfs: Hold eventfs_mutex when calling callback functions

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

On Wed, 01 Nov 2023 13:25:46 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The callback function that is used to create inodes and dentries is not
> protected by anything and the data that is passed to it could become
> stale. After eventfs_remove_dir() is called by the tracing system, it is
> free to remove the events that are associated to that directory.
> Unfortunately, that means the callbacks must not be called after that.
> 
>      CPU0				CPU1
>      ----				----
>  eventfs_root_lookup() {
> 				 eventfs_remove_dir() {
> 				      mutex_lock(&event_mutex);
> 				      ei->is_freed = set;
> 				      mutex_unlock(&event_mutex);
> 				 }
> 				 kfree(event_call);
> 
>     for (...) {
>       entry = &ei->entries[i];
>       r = entry->callback() {
>           call = data;		// call == event_call above
>           if (call->flags ...)
> 
>  [ USE AFTER FREE BUG ]
> 
> The safest way to protect this is to wrap the callback with:
> 
>  mutex_lock(&eventfs_mutex);
>  if (!ei->is_freed)
>      r = entry->callback();
>  else
>      r = -1;
>  mutex_unlock(&eventfs_mutex);
> 
> This will make sure that the callback will not be called after it is
> freed. But now it needs to be known that the callback is called while
> holding internal eventfs locks, and that it must not call back into the
> eventfs / tracefs system. There's no reason it should anyway, but document
> that as well.
> 
> Link: https://lore.kernel.org/all/CA+G9fYu9GOEbD=rR5eMR-=HJ8H6rMsbzDC2ZY5=Y50WpWAE7_Q@mail.gmail.com/
> 

Looks good to me.

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

Thanks!

> Cc: Ajay Kaher <akaher@vmware.com>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v5: https://lkml.kernel.org/r/20231031223420.778161254@goodmis.org
> 
> - Resynced to this patch series
> 
>  fs/tracefs/event_inode.c | 22 ++++++++++++++++++--
>  include/linux/tracefs.h  | 43 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 93d08e552483..8ac9abf7a3d5 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -615,7 +615,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>  		entry = &ei->entries[i];
>  		if (strcmp(name, entry->name) == 0) {
>  			void *cdata = data;
> -			r = entry->callback(name, &mode, &cdata, &fops);
> +			mutex_lock(&eventfs_mutex);
> +			/* If ei->is_freed, then the event itself may be too */
> +			if (!ei->is_freed)
> +				r = entry->callback(name, &mode, &cdata, &fops);
> +			else
> +				r = -1;
> +			mutex_unlock(&eventfs_mutex);
>  			if (r <= 0)
>  				continue;
>  			ret = simple_lookup(dir, dentry, flags);
> @@ -749,7 +755,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
>  		void *cdata = data;
>  		entry = &ei->entries[i];
>  		name = entry->name;
> -		r = entry->callback(name, &mode, &cdata, &fops);
> +		mutex_lock(&eventfs_mutex);
> +		/* If ei->is_freed, then the event itself may be too */
> +		if (!ei->is_freed)
> +			r = entry->callback(name, &mode, &cdata, &fops);
> +		else
> +			r = -1;
> +		mutex_unlock(&eventfs_mutex);
>  		if (r <= 0)
>  			continue;
>  		d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);
> @@ -819,6 +831,10 @@ static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx)
>   *   data = A pointer to @data, and the callback may replace it, which will
>   *         cause the file created to pass the new data to the open() call.
>   *   fops = the fops to use for the created file.
> + *
> + * NB. @callback is called while holding internal locks of the eventfs
> + *     system. The callback must not call any code that might also call into
> + *     the tracefs or eventfs system or it will risk creating a deadlock.
>   */
>  struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent,
>  					 const struct eventfs_entry *entries,
> @@ -878,6 +894,8 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
>   * @data: The default data to pass to the files (an entry may override it).
>   *
>   * This function creates the top of the trace event directory.
> + *
> + * See eventfs_create_dir() for use of @entries.
>   */
>  struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent,
>  						const struct eventfs_entry *entries,
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index 13359b1a35d1..7a5fe17b6bf9 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -23,9 +23,52 @@ struct file_operations;
>  
>  struct eventfs_file;
>  
> +/**
> + * eventfs_callback - A callback function to create dynamic files in eventfs
> + * @name: The name of the file that is to be created
> + * @mode: return the file mode for the file (RW access, etc)
> + * @data: data to pass to the created file ops
> + * @fops: the file operations of the created file
> + *
> + * The evetnfs files are dynamically created. The struct eventfs_entry array
> + * is passed to eventfs_create_dir() or eventfs_create_events_dir() that will
> + * be used to create the files within those directories. When a lookup
> + * or access to a file within the directory is made, the struct eventfs_entry
> + * array is used to find a callback() with the matching name that is being
> + * referenced (for lookups, the entire array is iterated and each callback
> + * will be called).
> + *
> + * The callback will be called with @name for the name of the file to create.
> + * The callback can return less than 1 to indicate  that no file should be
> + * created.
> + *
> + * If a file is to be created, then @mode should be populated with the file
> + * mode (permissions) for which the file is created for. This would be
> + * used to set the created inode i_mode field.
> + *
> + * The @data should be set to the data passed to the other file operations
> + * (read, write, etc). Note, @data will also point to the data passed in
> + * to eventfs_create_dir() or eventfs_create_events_dir(), but the callback
> + * can replace the data if it chooses to. Otherwise, the original data
> + * will be used for the file operation functions.
> + *
> + * The @fops should be set to the file operations that will be used to create
> + * the inode.
> + *
> + * NB. This callback is called while holding internal locks of the eventfs
> + *     system. The callback must not call any code that might also call into
> + *     the tracefs or eventfs system or it will risk creating a deadlock.
> + */
>  typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
>  				const struct file_operations **fops);
>  
> +/**
> + * struct eventfs_entry - dynamically created eventfs file call back handler
> + * @name:	Then name of the dynamic file in an eventfs directory
> + * @callback:	The callback to get the fops of the file when it is created
> + *
> + * See evenfs_callback() typedef for how to set up @callback.
> + */
>  struct eventfs_entry {
>  	const char			*name;
>  	eventfs_callback		callback;
> -- 
> 2.42.0


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