Message-Id: <20231102084332.754ff8867b7616c36bdf65ad@kernel.org>
Date:   Thu, 2 Nov 2023 08:43:32 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Ajay Kaher <akaher@...are.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v5 4/7] eventfs: Save ownership and mode

On Tue, 31 Oct 2023 18:33:30 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
> 
> Now that inodes and dentries are created on the fly, they are also
> reclaimed on memory pressure. Since the ownership and file mode are saved
> in the inode, if they are freed, any changes to the ownership and mode
> will be lost.

Do we (need to) allow to change the ownership and mode of the eventfs files?
I thought it was fixed on the files in tracefs...

Otherwise, the code itself looks good to me.

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

Thanks,

> 
> To counter this, if the user changes the permissions or ownership, save
> them, and when creating the inodes again, restore those changes.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
> Changes since v4: https://lkml.kernel.org/r/20231031193428.558586557@goodmis.org
> 
> - Rebased to this series
> 
>  fs/tracefs/event_inode.c | 148 +++++++++++++++++++++++++++++++++++----
>  fs/tracefs/internal.h    |  16 +++++
>  2 files changed, 151 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index b28f240bbb6d..d1683bf6d316 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -40,6 +40,15 @@ static DEFINE_MUTEX(eventfs_mutex);
>   */
>  DEFINE_STATIC_SRCU(eventfs_srcu);
>  
> +/* Mode is unsigned short, use the upper bits for flags */
> +enum {
> +	EVENTFS_SAVE_MODE	= BIT(16),
> +	EVENTFS_SAVE_UID	= BIT(17),
> +	EVENTFS_SAVE_GID	= BIT(18),
> +};
> +
> +#define EVENTFS_MODE_MASK	(EVENTFS_SAVE_MODE - 1)
> +
>  static struct dentry *eventfs_root_lookup(struct inode *dir,
>  					  struct dentry *dentry,
>  					  unsigned int flags);
> @@ -47,8 +56,89 @@ 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 void update_attr(struct eventfs_attr *attr, struct iattr *iattr)
> +{
> +	unsigned int ia_valid = iattr->ia_valid;
> +
> +	if (ia_valid & ATTR_MODE) {
> +		attr->mode = (attr->mode & ~EVENTFS_MODE_MASK) |
> +			(iattr->ia_mode & EVENTFS_MODE_MASK) |
> +			EVENTFS_SAVE_MODE;
> +	}
> +	if (ia_valid & ATTR_UID) {
> +		attr->mode |= EVENTFS_SAVE_UID;
> +		attr->uid = iattr->ia_uid;
> +	}
> +	if (ia_valid & ATTR_GID) {
> +		attr->mode |= EVENTFS_SAVE_GID;
> +		attr->gid = iattr->ia_gid;
> +	}
> +}
> +
> +static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
> +			    struct iattr *iattr)
> +{
> +	const struct eventfs_entry *entry;
> +	struct eventfs_inode *ei;
> +	const char *name;
> +	int ret;
> +
> +	mutex_lock(&eventfs_mutex);
> +	ei = dentry->d_fsdata;
> +	/* The LSB is set when the eventfs_inode is being freed */
> +	if (((unsigned long)ei & 1UL) || ei->is_freed) {
> +		/* Do not allow changes if the event is about to be removed. */
> +		mutex_unlock(&eventfs_mutex);
> +		return -ENODEV;
> +	}
> +
> +	/* Preallocate the children mode array if necessary */
> +	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);
> +			if (!ei->entry_attrs) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +		}
> +	}
> +
> +	ret = simple_setattr(idmap, dentry, iattr);
> +	if (ret < 0)
> +		goto out;
> +
> +	/*
> +	 * If this is a dir, then update the ei cache, only the file
> +	 * mode is saved in the ei->m_children, and the ownership is
> +	 * determined by the parent directory.
> +	 */
> +	if (dentry->d_inode->i_mode & S_IFDIR) {
> +		update_attr(&ei->attr, iattr);
> +
> +	} else {
> +		name = dentry->d_name.name;
> +
> +		for (int i = 0; i < ei->nr_entries; i++) {
> +			entry = &ei->entries[i];
> +			if (strcmp(name, entry->name) == 0) {
> +				update_attr(&ei->entry_attrs[i], iattr);
> +				break;
> +			}
> +		}
> +	}
> + out:
> +	mutex_unlock(&eventfs_mutex);
> +	return ret;
> +}
> +
>  static const struct inode_operations eventfs_root_dir_inode_operations = {
>  	.lookup		= eventfs_root_lookup,
> +	.setattr	= eventfs_set_attr,
> +};
> +
> +static const struct inode_operations eventfs_file_inode_operations = {
> +	.setattr	= eventfs_set_attr,
>  };
>  
>  static const struct file_operations eventfs_file_operations = {
> @@ -59,10 +149,30 @@ static const struct file_operations eventfs_file_operations = {
>  	.release	= eventfs_release,
>  };
>  
> +static void update_inode_attr(struct inode *inode, struct eventfs_attr *attr, umode_t mode)
> +{
> +	if (!attr) {
> +		inode->i_mode = mode;
> +		return;
> +	}
> +
> +	if (attr->mode & EVENTFS_SAVE_MODE)
> +		inode->i_mode = attr->mode & EVENTFS_MODE_MASK;
> +	else
> +		inode->i_mode = mode;
> +
> +	if (attr->mode & EVENTFS_SAVE_UID)
> +		inode->i_uid = attr->uid;
> +
> +	if (attr->mode & EVENTFS_SAVE_GID)
> +		inode->i_gid = attr->gid;
> +}
> +
>  /**
>   * create_file - create a file in the tracefs filesystem
>   * @name: the name of the file to create.
>   * @mode: the permission that the file should have.
> + * @attr: saved attributes changed by user
>   * @parent: parent dentry for this file.
>   * @data: something that the caller will want to get to later on.
>   * @fop: struct file_operations that should be used for this file.
> @@ -72,6 +182,7 @@ static const struct file_operations eventfs_file_operations = {
>   * call.
>   */
>  static struct dentry *create_file(const char *name, umode_t mode,
> +				  struct eventfs_attr *attr,
>  				  struct dentry *parent, void *data,
>  				  const struct file_operations *fop)
>  {
> @@ -95,7 +206,10 @@ static struct dentry *create_file(const char *name, umode_t mode,
>  	if (unlikely(!inode))
>  		return eventfs_failed_creating(dentry);
>  
> -	inode->i_mode = mode;
> +	/* If the user updated the directory's attributes, use them */
> +	update_inode_attr(inode, attr, mode);
> +
> +	inode->i_op = &eventfs_file_inode_operations;
>  	inode->i_fop = fop;
>  	inode->i_private = data;
>  
> @@ -108,19 +222,19 @@ static struct dentry *create_file(const char *name, umode_t mode,
>  
>  /**
>   * create_dir - create a dir in the tracefs filesystem
> - * @name: the name of the file to create.
> + * @ei: the eventfs_inode that represents the directory to create
>   * @parent: parent dentry for this file.
>   *
>   * This function will create a dentry for a directory represented by
>   * a eventfs_inode.
>   */
> -static struct dentry *create_dir(const char *name, struct dentry *parent)
> +static struct dentry *create_dir(struct eventfs_inode *ei, struct dentry *parent)
>  {
>  	struct tracefs_inode *ti;
>  	struct dentry *dentry;
>  	struct inode *inode;
>  
> -	dentry = eventfs_start_creating(name, parent);
> +	dentry = eventfs_start_creating(ei->name, parent);
>  	if (IS_ERR(dentry))
>  		return dentry;
>  
> @@ -128,7 +242,9 @@ static struct dentry *create_dir(const char *name, struct dentry *parent)
>  	if (unlikely(!inode))
>  		return eventfs_failed_creating(dentry);
>  
> -	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
> +	/* If the user updated the directory's attributes, use them */
> +	update_inode_attr(inode, &ei->attr, S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO);
> +
>  	inode->i_op = &eventfs_root_dir_inode_operations;
>  	inode->i_fop = &eventfs_file_operations;
>  
> @@ -146,6 +262,7 @@ static void free_ei(struct eventfs_inode *ei)
>  {
>  	kfree_const(ei->name);
>  	kfree(ei->d_children);
> +	kfree(ei->entry_attrs);
>  	kfree(ei);
>  }
>  
> @@ -231,7 +348,7 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry)
>  /**
>   * create_file_dentry - create a dentry for a file of an eventfs_inode
>   * @ei: the eventfs_inode that the file will be created under
> - * @e_dentry: a pointer to the d_children[] of the @ei
> + * @idx: the index into the d_children[] of the @ei
>   * @parent: The parent dentry of the created file.
>   * @name: The name of the file to create
>   * @mode: The mode of the file.
> @@ -244,10 +361,12 @@ void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry)
>   * just do a dget() on it and return. Otherwise create the dentry and attach it.
>   */
>  static struct dentry *
> -create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry,
> +create_file_dentry(struct eventfs_inode *ei, int idx,
>  		   struct dentry *parent, const char *name, umode_t mode, void *data,
>  		   const struct file_operations *fops, bool lookup)
>  {
> +	struct eventfs_attr *attr = NULL;
> +	struct dentry **e_dentry = &ei->d_children[idx];
>  	struct dentry *dentry;
>  	bool invalidate = false;
>  
> @@ -264,13 +383,18 @@ create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry,
>  		mutex_unlock(&eventfs_mutex);
>  		return *e_dentry;
>  	}
> +
> +	/* ei->entry_attrs are protected by SRCU */
> +	if (ei->entry_attrs)
> +		attr = &ei->entry_attrs[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, parent, data, fops);
> +	dentry = create_file(name, mode, attr, parent, data, fops);
>  
>  	if (!lookup)
>  		inode_unlock(parent->d_inode);
> @@ -378,7 +502,7 @@ create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
>  	if (!lookup)
>  		inode_lock(parent->d_inode);
>  
> -	dentry = create_dir(ei->name, parent);
> +	dentry = create_dir(ei, parent);
>  
>  	if (!lookup)
>  		inode_unlock(parent->d_inode);
> @@ -495,8 +619,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>  			if (r <= 0)
>  				continue;
>  			ret = simple_lookup(dir, dentry, flags);
> -			create_file_dentry(ei, &ei->d_children[i],
> -					   ei_dentry, name, mode, cdata,
> +			create_file_dentry(ei, i, ei_dentry, name, mode, cdata,
>  					   fops, true);
>  			break;
>  		}
> @@ -629,8 +752,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
>  		r = entry->callback(name, &mode, &cdata, &fops);
>  		if (r <= 0)
>  			continue;
> -		d = create_file_dentry(ei, &ei->d_children[i],
> -				       parent, name, mode, cdata, fops, false);
> +		d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);
>  		if (d) {
>  			ret = add_dentries(&dentries, d, cnt);
>  			if (ret < 0)
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index 5a98e87dd3d1..5f60bcd69289 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -13,6 +13,18 @@ struct tracefs_inode {
>  	struct inode            vfs_inode;
>  };
>  
> +/*
> + * struct eventfs_attr - cache the mode and ownership of a eventfs entry
> + * @mode:	saved mode plus flags of what is saved
> + * @uid:	saved uid if changed
> + * @gid:	saved gid if changed
> + */
> +struct eventfs_attr {
> +	int				mode;
> +	kuid_t				uid;
> +	kgid_t				gid;
> +};
> +
>  /*
>   * struct eventfs_inode - hold the properties of the eventfs directories.
>   * @list:	link list into the parent directory
> @@ -22,6 +34,8 @@ struct tracefs_inode {
>   * @dentry:     the dentry of the directory
>   * @d_parent:   pointer to the parent's dentry
>   * @d_children: The array of dentries to represent the files when created
> + * @entry_attrs: Saved mode and ownership of the @d_children
> + * @attr:	Saved mode and ownership of eventfs_inode itself
>   * @data:	The private data to pass to the callbacks
>   * @is_freed:	Flag set if the eventfs is on its way to be freed
>   *                Note if is_freed is set, then dentry is corrupted.
> @@ -35,6 +49,8 @@ struct eventfs_inode {
>  	struct dentry			*dentry; /* Check is_freed to access */
>  	struct dentry			*d_parent;
>  	struct dentry			**d_children;
> +	struct eventfs_attr		*entry_attrs;
> +	struct eventfs_attr		attr;
>  	void				*data;
>  	/*
>  	 * Union - used for deletion
> -- 
> 2.42.0


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

Powered by blists - more mailing lists