lkml.org 
[lkml]   [2023]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry
    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+_X85Cg@mail.gmail.com/
    > Link: https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunSJf=MTe=6aA@mail.gmail.com/
    >
    > 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/2XQUK9V1Fm5uX0GdoaraRkzAN29/


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

    \
     
     \ /
      Last update: 2023-10-29 08:02    [W:5.688 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site