Re: [PATCH v5 4/7] eventfs: Save ownership and mode

Masami Hiramatsu
2023-11-01T23:43:44Z

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

> From: "Steven Rostedt (Google)" <rostedt@goodmis.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@kernel.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@goodmis.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@kernel.org>