* [PATCH v4 2/3] eventfs: Test for ei->is_freed when accessing ei->dentry
2023-10-31 19:31 [PATCH v4 0/3] eventfs: Fixing dynamic creation Steven Rostedt
2023-10-31 19:31 ` [PATCH v4 1/3] eventfs: Have a free_ei() that just frees the eventfs_inode Steven Rostedt
@ 2023-10-31 19:31 ` Steven Rostedt
2023-10-31 19:31 ` [PATCH v4 3/3] eventfs: Save ownership and mode Steven Rostedt
2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2023-10-31 19:31 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, Arnd Bergmann,
Ajay Kaher, Linux Kernel Functional Testing, Naresh Kamboju,
Beau Belgrave
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
The eventfs_inode (ei) is protected by SRCU, but the ei->dentry is not. It
is protected by the eventfs_mutex. Anytime the eventfs_mutex is released,
and access to the ei->dentry needs to be done, it should first check if
ei->is_freed is set under the eventfs_mutex. If it is, then the ei->dentry
is invalid and must not be used. The ei->dentry must only be accessed
under the eventfs_mutex and after checking if ei->is_freed is set.
When the ei is being freed, it will (under the eventfs_mutex) set is_freed
and at the same time move the dentry to a free list to be cleared after
the eventfs_mutex is released. This means that any access to the
ei->dentry must check first if ei->is_freed is set, because if it is, then
the dentry is on its way to be freed.
Also add comments to describe this better.
Link: https://lore.kernel.org/all/CA+G9fYt6pY+tMZEOg=SoEywQOe19fGP3uR15SGowkdK+_X85Cg@mail.gmail.com/
Link: https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunSJf=MTe=6aA@mail.gmail.com/
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Arnd Bergmann" <arnd@arndb.de>
Cc: "Ajay Kaher" <akaher@vmware.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reported-by: Beau Belgrave <beaub@linux.microsoft.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Tested-by: Beau Belgrave <beaub@linux.microsoft.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v2: https://lore.kernel.org/linux-trace-kernel/20231028164650.4f5ea18a@rorschach.local.home/
- Separated out free_ei() into its own patch
fs/tracefs/event_inode.c | 50 +++++++++++++++++++++++++++++++++++-----
fs/tracefs/internal.h | 3 ++-
2 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 76323f48d5f0..362a2dba82c2 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -24,7 +24,20 @@
#include <linux/delay.h>
#include "internal.h"
+/*
+ * eventfs_mutex protects the eventfs_inode (ei) dentry. Any access
+ * to the ei->dentry must be done under this mutex and after checking
+ * if ei->is_freed is not set. The ei->dentry is released under the
+ * mutex at the same time ei->is_freed is set. If ei->is_freed is set
+ * then the ei->dentry is invalid.
+ */
static DEFINE_MUTEX(eventfs_mutex);
+
+/*
+ * The eventfs_inode (ei) itself is protected by SRCU. It is released from
+ * its parent's list and will have is_freed set (under eventfs_mutex).
+ * After the SRCU grace period is over, the ei may be freed.
+ */
DEFINE_STATIC_SRCU(eventfs_srcu);
static struct dentry *eventfs_root_lookup(struct inode *dir,
@@ -239,6 +252,10 @@ create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry,
bool invalidate = false;
mutex_lock(&eventfs_mutex);
+ if (ei->is_freed) {
+ mutex_unlock(&eventfs_mutex);
+ return NULL;
+ }
/* If the e_dentry already has a dentry, use it */
if (*e_dentry) {
/* lookup does not need to up the ref count */
@@ -312,6 +329,8 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)
struct eventfs_inode *ei_child;
struct tracefs_inode *ti;
+ lockdep_assert_held(&eventfs_mutex);
+
/* srcu lock already held */
/* fill parent-child relation */
list_for_each_entry_srcu(ei_child, &ei->children, list,
@@ -325,6 +344,7 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)
/**
* create_dir_dentry - Create a directory dentry for the eventfs_inode
+ * @pei: The eventfs_inode parent of ei.
* @ei: The eventfs_inode to create the directory for
* @parent: The dentry of the parent of this directory
* @lookup: True if this is called by the lookup code
@@ -332,12 +352,17 @@ static void eventfs_post_create_dir(struct eventfs_inode *ei)
* This creates and attaches a directory dentry to the eventfs_inode @ei.
*/
static struct dentry *
-create_dir_dentry(struct eventfs_inode *ei, struct dentry *parent, bool lookup)
+create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei,
+ struct dentry *parent, bool lookup)
{
bool invalidate = false;
struct dentry *dentry = NULL;
mutex_lock(&eventfs_mutex);
+ if (pei->is_freed || ei->is_freed) {
+ mutex_unlock(&eventfs_mutex);
+ return NULL;
+ }
if (ei->dentry) {
/* If the dentry already has a dentry, use it */
dentry = ei->dentry;
@@ -440,7 +465,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
*/
mutex_lock(&eventfs_mutex);
ei = READ_ONCE(ti->private);
- if (ei)
+ if (ei && !ei->is_freed)
ei_dentry = READ_ONCE(ei->dentry);
mutex_unlock(&eventfs_mutex);
@@ -454,7 +479,7 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
if (strcmp(ei_child->name, name) != 0)
continue;
ret = simple_lookup(dir, dentry, flags);
- create_dir_dentry(ei_child, ei_dentry, true);
+ create_dir_dentry(ei, ei_child, ei_dentry, true);
created = true;
break;
}
@@ -588,7 +613,7 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
list_for_each_entry_srcu(ei_child, &ei->children, list,
srcu_read_lock_held(&eventfs_srcu)) {
- d = create_dir_dentry(ei_child, parent, false);
+ d = create_dir_dentry(ei, ei_child, parent, false);
if (d) {
ret = add_dentries(&dentries, d, cnt);
if (ret < 0)
@@ -705,12 +730,20 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
ei->nr_entries = size;
ei->data = data;
INIT_LIST_HEAD(&ei->children);
+ INIT_LIST_HEAD(&ei->list);
mutex_lock(&eventfs_mutex);
- list_add_tail(&ei->list, &parent->children);
- ei->d_parent = parent->dentry;
+ if (!parent->is_freed) {
+ list_add_tail(&ei->list, &parent->children);
+ ei->d_parent = parent->dentry;
+ }
mutex_unlock(&eventfs_mutex);
+ /* Was the parent freed? */
+ if (list_empty(&ei->list)) {
+ free_ei(ei);
+ ei = NULL;
+ }
return ei;
}
@@ -884,6 +917,11 @@ void eventfs_remove_dir(struct eventfs_inode *ei)
for (i = 0; i < ei->nr_entries; i++)
unhook_dentry(&ei->d_children[i], &dentry_list);
unhook_dentry(&ei->dentry, &dentry_list);
+ /*
+ * Note, ei->is_freed is a union along with ei->rcu
+ * and ei->del_list. When the ei is added to either
+ * of those lists, it automatically sets ei->is_freed.
+ */
call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei);
}
mutex_unlock(&eventfs_mutex);
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 64fde9490f52..21a1fa682b74 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -30,7 +30,7 @@ struct eventfs_inode {
const struct eventfs_entry *entries;
const char *name;
struct list_head children;
- struct dentry *dentry;
+ struct dentry *dentry; /* Check is_freed to access */
struct dentry *d_parent;
struct dentry **d_children;
void *data;
@@ -39,6 +39,7 @@ struct eventfs_inode {
* @del_list: list of eventfs_inode to delete
* @rcu: eventfs_inode to delete in RCU
* @is_freed: node is freed if one of the above is set
+ * Note if is_freed is set, then dentry is corrupted.
*/
union {
struct list_head del_list;
--
2.42.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v4 3/3] eventfs: Save ownership and mode
2023-10-31 19:31 [PATCH v4 0/3] eventfs: Fixing dynamic creation Steven Rostedt
2023-10-31 19:31 ` [PATCH v4 1/3] eventfs: Have a free_ei() that just frees the eventfs_inode Steven Rostedt
2023-10-31 19:31 ` [PATCH v4 2/3] eventfs: Test for ei->is_freed when accessing ei->dentry Steven Rostedt
@ 2023-10-31 19:31 ` Steven Rostedt
2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2023-10-31 19:31 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Andrew Morton, Ajay Kaher
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.
To counter this, if the user changes the permissions or ownership, save
them, and when creating the inodes again, restore those changes.
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ajay Kaher <akaher@vmware.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v3: https://lore.kernel.org/linux-trace-kernel/20231031113627.2edfa83f@gandalf.local.home
- Just rebased as part of this patch 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 362a2dba82c2..81e7ccef2783 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 21a1fa682b74..1ec0d167542a 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,7 +34,9 @@ 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
* @data: The private data to pass to the callbacks
+ * @attr: Saved mode and ownership of eventfs_inode itself
* @nr_entries: The number of items in @entries
*/
struct eventfs_inode {
@@ -33,6 +47,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
^ permalink raw reply related [flat|nested] 4+ messages in thread