On Sun, 29 Oct 2023 at 02:16, Steven Rostedt <rost...@goodmis.org> wrote:
>
> From: "Steven Rostedt (Google)" <rost...@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...@mail.gmail.com/
> Link:
> https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunSJf=MTe=6...@mail.gmail.com/
>
> Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use
> eventfs_inode")
> Reported-by: Linux Kernel Functional Testing <l...@linaro.org>
> Reported-by: Naresh Kamboju <naresh.kamb...@linaro.org>
> Reported-by: Beau Belgrave <be...@linux.microsoft.com>
> Signed-off-by: Steven Rostedt (Google) <rost...@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.0d342...@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
>
