On Sun, 29 Oct 2023 at 02:16, Steven Rostedt rostedt@goodmis.org wrote:

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+_X8... Link: https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunSJf=MTe=...

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 Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org

Following build errors have been noticed.

fs/tracefs/event_inode.c:348:1: error: return type defaults to 'int' [-Werror=implicit-int] 348 | create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, | ^~~~~~~~~~~~~~~~~ In file included from include/uapi/linux/posix_types.h:5, from include/uapi/linux/types.h:14, from include/linux/types.h:6, from include/linux/kasan-checks.h:5, from include/asm-generic/rwonce.h:26, from ./arch/x86/include/generated/asm/rwonce.h:1, from include/linux/compiler.h:251, from include/linux/build_bug.h:5, from include/linux/bits.h:21, from include/linux/bitops.h:6, from include/linux/radix-tree.h:11, from include/linux/idr.h:15, from include/linux/fsnotify_backend.h:13, from include/linux/fsnotify.h:15, from fs/tracefs/event_inode.c:17: fs/tracefs/event_inode.c: In function 'create_dir_dentry': include/linux/stddef.h:8:14: error: returning 'void *' from a function with return type 'int' makes integer from pointer without a cast [-Werror=int-conversion] 8 | #define NULL ((void *)0) | ^ fs/tracefs/event_inode.c:357:24: note: in expansion of macro 'NULL' 357 | return NULL; | ^~~~ fs/tracefs/event_inode.c:366:24: error: returning 'struct dentry *' from a function with return type 'int' makes integer from pointer without a cast [-Werror=int-conversion] 366 | return dentry; | ^~~~~~ fs/tracefs/event_inode.c:394:24: error: returning 'struct dentry *' from a function with return type 'int' makes integer from pointer without a cast [-Werror=int-conversion] 394 | return dentry; | ^~~~~~ fs/tracefs/event_inode.c:416:34: error: returning 'struct dentry *' from a function with return type 'int' makes integer from pointer without a cast [-Werror=int-conversion] 416 | return invalidate ? NULL : dentry; | ~~~~~~~~~~~~~~~~~~^~~~~~~~ fs/tracefs/event_inode.c: In function 'dcache_dir_open_wrapper': fs/tracefs/event_inode.c:609:49: error: passing argument 2 of 'create_dir_dentry' from incompatible pointer type [-Werror=incompatible-pointer-types] 609 | d = create_dir_dentry(ei_child, parent, false); | ^~~~~~ | | | struct dentry * fs/tracefs/event_inode.c:348:68: note: expected 'struct eventfs_inode *' but argument is of type 'struct dentry *' 348 | create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, | ~~~~~~~~~~~~~~~~~~~~~~^~ fs/tracefs/event_inode.c:609:21: error: too few arguments to function 'create_dir_dentry' 609 | d = create_dir_dentry(ei_child, parent, false); | ^~~~~~~~~~~~~~~~~ fs/tracefs/event_inode.c:348:1: note: declared here 348 | create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, | ^~~~~~~~~~~~~~~~~ fs/tracefs/event_inode.c:625:19: error: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion] 625 | d = create_dir_dentry(ei, ei_child, parent, false); | ^ fs/tracefs/event_inode.c:626:46: error: left-hand operand of comma expression has no effect [-Werror=unused-value] 626 | parent, name, mode, cdata, fops, false); | ^ fs/tracefs/event_inode.c:626:52: error: left-hand operand of comma expression has no effect [-Werror=unused-value] 626 | parent, name, mode, cdata, fops, false); | ^ fs/tracefs/event_inode.c:626:58: error: left-hand operand of comma expression has no effect [-Werror=unused-value] 626 | parent, name, mode, cdata, fops, false); | ^ fs/tracefs/event_inode.c:626:65: error: left-hand operand of comma expression has no effect [-Werror=unused-value] 626 | parent, name, mode, cdata, fops, false); | ^ fs/tracefs/event_inode.c:626:71: error: left-hand operand of comma expression has no effect [-Werror=unused-value] 626 | parent, name, mode, cdata, fops, false); | ^ fs/tracefs/event_inode.c:626:71: error: statement with no effect [-Werror=unused-value] fs/tracefs/event_inode.c:626:78: error: expected ';' before ')' token 626 | parent, name, mode, cdata, fops, false); | ^ | ; fs/tracefs/event_inode.c:626:78: error: expected statement before ')' token fs/tracefs/event_inode.c: In function 'eventfs_remove_dir': fs/tracefs/event_inode.c:921:1: error: invalid use of void expression 921 | + call_srcu(&eventfs_srcu, &ei->rcu, free_rcu_ei); | ^ cc1: all warnings being treated as errors

Links: - https://storage.tuxsuite.com/public/linaro/naresh/builds/2XQUK9V1Fm5uX0Gdoar...

---

Changes since v1: https://lore.kernel.org/all/20231028163749.0d3429a1@rorschach.local.home/

• Add comment about ei->is_freed is a union along with ei->rcu and ei->del_list so that others can find where ei->is_freed is set and not get confused about why ei->dentry is being removed but ei->is_freed isn't mentioned.

• And fixed change log to remove the double "Reported-by".

fs/tracefs/event_inode.c | 65 +++++++++++++++++++++++++++++++++------- fs/tracefs/internal.h | 3 +- 2 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 4d2da7480e5f..45bddce7c747 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, @@ -234,6 +247,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 */
  

@@ -307,6 +324,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,
  

@@ -320,6 +339,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

@@ -327,12 +347,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;
  

@@ -435,7 +460,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);
  
  

@@ -449,7 +474,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;
    }
  

@@ -583,7 +608,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)
  

@@ -637,6 +662,13 @@ static int dcache_readdir_wrapper(struct file *file, struct dir_context *ctx) return ret; }

+static void free_ei(struct eventfs_inode *ei) +{

•   kfree_const(ei->name);
  

•   kfree(ei->d_children);
  

•   kfree(ei);
  

+}

/**

• eventfs_create_dir - Create the eventfs_inode for this directory
• @name: The name of the directory to create.

@@ -700,12 +732,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;
  

}

@@ -787,13 +827,11 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry return ERR_PTR(-ENOMEM); }

-static void free_ei(struct rcu_head *head) +static void free_rcu_ei(struct rcu_head *head) { struct eventfs_inode *ei = container_of(head, struct eventfs_inode, rcu);

•   kfree_const(ei->name);
  

•   kfree(ei->d_children);
  

•   kfree(ei);
  

•   free_ei(ei);
  

}

/** @@ -880,7 +918,12 @@ 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);

•           call_srcu(&eventfs_srcu, &ei->rcu, free_ei);
  

•           /*
  

•            * 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