[PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[Steven Rostedt]

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

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

Re: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[Masami Hiramatsu]

On Sat, 28 Oct 2023 16:46:50 -0400
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/
> 

Looks good to me.

Revieewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

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


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[Naresh Kamboju]

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
>

Re: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[Steven Rostedt]

On Sun, 29 Oct 2023 12:31:54 +0530
Naresh Kamboju <naresh.kamboju@linaro.org> wrote:

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

This patch passed all my tests, I have no idea how the above happened.
The "return type defaults to int" tells me the "static struct dentry *"
part of the patch may have been clipped.

Are you sure it was applied correctly? Perhaps check out the branch I
have and let me know if you get the same errors.

git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace/core

-- Steve

Re: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[Naresh Kamboju]

Hi Steven,

> Are you sure it was applied correctly?

Please ignore the build warnings / errors it was due to apply patch was
not successful.

> Perhaps check out the branch I
> have and let me know if you get the same errors.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace/core

I have tested the linux-trace.git trace/core and run selftests ftrace
the reported kernel panic [1] & [2] has been fixed but found
"general protection fault"  at  kernel/trace/trace_events.c:2439.

[1] https://lore.kernel.org/all/CA+G9fYt6pY+tMZEOg=SoEywQOe19fGP3uR15SGowkdK+_X85Cg@mail.gmail.com/
[2] https://lore.kernel.org/linux-trace-kernel/20231028164650.4f5ea18a@rorschach.local.home/

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>

Test log:
==========
kselftest: Running tests in ftrace
TAP version 13
1..1
# timeout set to 0
# selftests: ftrace: ftracetest-ktap

...

# ok 46 Test creation and deletion of trace instances while setting an event
[  332.783872] general protection fault, probably for non-canonical
address 0x6b6b6b6b6b6b6bcb: 0000 [#1] PREEMPT SMP PTI
[  332.794585] CPU: 1 PID: 5165 Comm: ls Not tainted 6.6.0-rc4 #1
[  332.800429] Hardware name: Supermicro SYS-5019S-ML/X11SSH-F, BIOS
2.7 12/07/2021
[  332.807820] RIP: 0010:event_callback (kernel/trace/trace_events.c:2439)
[ 332.812353] Code: f6 48 c7 c6 09 78 6b a0 41 55 49 89 cd 41 54 49 89
d4 53 48 8b 02 48 89 fb 4c 8b 78 10 e8 10 8b 19 01 85 c0 0f 84 d2 00
00 00 <41> 8b 47 60 a8 08 0f 85 9c 00 00 00 49 8b 47 10 48 83 78 18 00
74
All code
========
   0: f6 48 c7 c6          testb  $0xc6,-0x39(%rax)
   4: 09 78 6b              or     %edi,0x6b(%rax)
   7: a0 41 55 49 89 cd 41 movabs 0x495441cd89495541,%al
   e: 54 49
  10: 89 d4                mov    %edx,%esp
  12: 53                    push   %rbx
  13: 48 8b 02              mov    (%rdx),%rax
  16: 48 89 fb              mov    %rdi,%rbx
  19: 4c 8b 78 10          mov    0x10(%rax),%r15
  1d: e8 10 8b 19 01        callq  0x1198b32
  22: 85 c0                test   %eax,%eax
  24: 0f 84 d2 00 00 00    je     0xfc
  2a:* 41 8b 47 60          mov    0x60(%r15),%eax <-- trapping instruction
  2e: a8 08                test   $0x8,%al
  30: 0f 85 9c 00 00 00    jne    0xd2
  36: 49 8b 47 10          mov    0x10(%r15),%rax
  3a: 48 83 78 18 00        cmpq   $0x0,0x18(%rax)
  3f: 74                    .byte 0x74

Code starting with the faulting instruction
===========================================
   0: 41 8b 47 60          mov    0x60(%r15),%eax
   4: a8 08                test   $0x8,%al
   6: 0f 85 9c 00 00 00    jne    0xa8
   c: 49 8b 47 10          mov    0x10(%r15),%rax
  10: 48 83 78 18 00        cmpq   $0x0,0x18(%rax)
  15: 74                    .byte 0x74
[  332.831089] RSP: 0018:ffffba5700967b50 EFLAGS: 00010202
[  332.836315] RAX: 0000000000000001 RBX: ffffffffa06fb5d7 RCX: ffffba5700967bc8
[  332.843439] RDX: 0000000000000069 RSI: ffffffffa06b7809 RDI: ffffffffa06fb5d7
[  332.850563] RBP: ffffba5700967b78 R08: ffff8dbe12250134 R09: 0000000000000000
[  332.857722] R10: ffff8dbe122500e0 R11: ffff8dbe0d8f8fc8 R12: ffffba5700967bd0
[  332.864846] R13: ffffba5700967bc8 R14: ffffba5700967bc6 R15: 6b6b6b6b6b6b6b6b
[  332.871971] FS:  00007fc5a5062d00(0000) GS:ffff8dc167a80000(0000)
knlGS:0000000000000000
[  332.880057] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  332.885801] CR2: 00007f7f8dc50a1c CR3: 0000000113c84003 CR4: 00000000003706e0
[  332.892928] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  332.900058] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  332.907183] Call Trace:
[  332.909658]  <TASK>
[  332.911760] ? show_regs (arch/x86/kernel/dumpstack.c:479)
[  332.915166] ? die_addr (arch/x86/kernel/dumpstack.c:421
arch/x86/kernel/dumpstack.c:460)
[  332.918486] ? exc_general_protection (arch/x86/kernel/traps.c:697
arch/x86/kernel/traps.c:642)
[  332.923191] ? asm_exc_general_protection
(arch/x86/include/asm/idtentry.h:564)
[  332.928073] ? event_callback (kernel/trace/trace_events.c:2439)
[  332.931996] dcache_dir_open_wrapper (fs/tracefs/event_inode.c:745)
[  332.936636] ? __pfx_dcache_dir_open_wrapper (fs/tracefs/event_inode.c:683)
[  332.941772] do_dentry_open (fs/open.c:929)
[  332.945646] vfs_open (fs/open.c:1064)
[  332.948800] path_openat (fs/namei.c:3639 fs/namei.c:3796)
[  332.952465] ? ___slab_alloc (mm/slub.c:810 mm/slub.c:3265)
[  332.956384] ? nfs_ctx_key_to_expire (fs/nfs/write.c:1281)
[  332.960917] ? trace_preempt_on (kernel/trace/trace_preemptirq.c:105)
[  332.964929] do_filp_open (fs/namei.c:3823)
[  332.968511] do_sys_openat2 (fs/open.c:1422)
[  332.972173] __x64_sys_openat (fs/open.c:1448)
[  332.976014] do_syscall_64 (arch/x86/entry/common.c:50
arch/x86/entry/common.c:80)
[  332.979601] ? trace_hardirqs_on_prepare
(kernel/trace/trace_preemptirq.c:47 (discriminator 16)
kernel/trace/trace_preemptirq.c:42 (discriminator 16))
[  332.984436] ? syscall_exit_to_user_mode (kernel/entry/common.c:299)
[  332.989219] ? do_syscall_64 (arch/x86/entry/common.c:87)
[  332.992964] ? do_syscall_64 (arch/x86/entry/common.c:87)
[  332.996709] ? do_syscall_64 (arch/x86/entry/common.c:87)
[  333.000460] ? do_syscall_64 (arch/x86/entry/common.c:87)
[  333.004204] ? do_syscall_64 (arch/x86/entry/common.c:87)
[  333.007948] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
[  333.012994] RIP: 0033:0x7fc5a51fadf1
[ 333.016600] Code: 44 24 18 31 c0 41 83 e2 40 75 3e 89 f0 25 00 00 41
00 3d 00 00 41 00 74 30 89 f2 b8 01 01 00 00 48 89 fe bf 9c ff ff ff
0f 05 <48> 3d 00 f0 ff ff 77 3f 48 8b 54 24 18 64 48 2b 14 25 28 00 00
00
All code
========
   0: 44 24 18              rex.R and $0x18,%al
   3: 31 c0                xor    %eax,%eax
   5: 41 83 e2 40          and    $0x40,%r10d
   9: 75 3e                jne    0x49
   b: 89 f0                mov    %esi,%eax
   d: 25 00 00 41 00        and    $0x410000,%eax
  12: 3d 00 00 41 00        cmp    $0x410000,%eax
  17: 74 30                je     0x49
  19: 89 f2                mov    %esi,%edx
  1b: b8 01 01 00 00        mov    $0x101,%eax
  20: 48 89 fe              mov    %rdi,%rsi
  23: bf 9c ff ff ff        mov    $0xffffff9c,%edi
  28: 0f 05                syscall
  2a:* 48 3d 00 f0 ff ff    cmp    $0xfffffffffffff000,%rax <--
trapping instruction
  30: 77 3f                ja     0x71
  32: 48 8b 54 24 18        mov    0x18(%rsp),%rdx
  37: 64 48 2b 14 25 28 00 sub    %fs:0x28,%rdx
  3e: 00 00

Code starting with the faulting instruction
===========================================
   0: 48 3d 00 f0 ff ff    cmp    $0xfffffffffffff000,%rax
   6: 77 3f                ja     0x47
   8: 48 8b 54 24 18        mov    0x18(%rsp),%rdx
   d: 64 48 2b 14 25 28 00 sub    %fs:0x28,%rdx
  14: 00 00
[  333.035361] RSP: 002b:00007ffebef93680 EFLAGS: 00000287 ORIG_RAX:
0000000000000101
[  333.042919] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fc5a51fadf1
[  333.050042] RDX: 0000000000090800 RSI: 0000562c92f2cc20 RDI: 00000000ffffff9c
[  333.057166] RBP: 0000000000000000 R08: 0000000000000007 R09: 0000562c92f2cc90
[  333.064290] R10: 0000000000000000 R11: 0000000000000287 R12: 00007fc5a5062b98
[  333.071415] R13: 00007ffebef93ad0 R14: 0000562c92f2cc20 R15: 0000562c9122ffc8
[  333.078541]  </TASK>
[  333.080733] Modules linked in: x86_pkg_temp_thermal fuse configfs
ip_tables x_tables [last unloaded: trace_printk]
[  333.091109] ---[ end trace 0000000000000000 ]---
[  333.095757] RIP: 0010:event_callback (kernel/trace/trace_events.c:2439)
[ 333.100295] Code: f6 48 c7 c6 09 78 6b a0 41 55 49 89 cd 41 54 49 89
d4 53 48 8b 02 48 89 fb 4c 8b 78 10 e8 10 8b 19 01 85 c0 0f 84 d2 00
00 00 <41> 8b 47 60 a8 08 0f 85 9c 00 00 00 49 8b 47 10 48 83 78 18 00
74
All code
========
   0: f6 48 c7 c6          testb  $0xc6,-0x39(%rax)
   4: 09 78 6b              or     %edi,0x6b(%rax)
   7: a0 41 55 49 89 cd 41 movabs 0x495441cd89495541,%al
   e: 54 49
  10: 89 d4                mov    %edx,%esp
  12: 53                    push   %rbx
  13: 48 8b 02              mov    (%rdx),%rax
  16: 48 89 fb              mov    %rdi,%rbx
  19: 4c 8b 78 10          mov    0x10(%rax),%r15
  1d: e8 10 8b 19 01        callq  0x1198b32
  22: 85 c0                test   %eax,%eax
  24: 0f 84 d2 00 00 00    je     0xfc
  2a:* 41 8b 47 60          mov    0x60(%r15),%eax <-- trapping instruction
  2e: a8 08                test   $0x8,%al
  30: 0f 85 9c 00 00 00    jne    0xd2
  36: 49 8b 47 10          mov    0x10(%r15),%rax
  3a: 48 83 78 18 00        cmpq   $0x0,0x18(%rax)
  3f: 74                    .byte 0x74

Code starting with the faulting instruction
===========================================
   0: 41 8b 47 60          mov    0x60(%r15),%eax
   4: a8 08                test   $0x8,%al
   6: 0f 85 9c 00 00 00    jne    0xa8
   c: 49 8b 47 10          mov    0x10(%r15),%rax
  10: 48 83 78 18 00        cmpq   $0x0,0x18(%rax)
  15: 74                    .byte 0x74
[  333.119040] RSP: 0018:ffffba5700967b50 EFLAGS: 00010202
[  333.124290] RAX: 0000000000000001 RBX: ffffffffa06fb5d7 RCX: ffffba5700967bc8
[  333.131451] RDX: 0000000000000069 RSI: ffffffffa06b7809 RDI: ffffffffa06fb5d7
[  333.138623] RBP: ffffba5700967b78 R08: ffff8dbe12250134 R09: 0000000000000000
[  333.145811] R10: ffff8dbe122500e0 R11: ffff8dbe0d8f8fc8 R12: ffffba5700967bd0
[  333.152969] R13: ffffba5700967bc8 R14: ffffba5700967bc6 R15: 6b6b6b6b6b6b6b6b
[  333.160127] FS:  00007fc5a5062d00(0000) GS:ffff8dc167a80000(0000)
knlGS:0000000000000000
[  333.168215] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  333.173961] CR2: 00007f7f8dc50a1c CR3: 0000000113c84003 CR4: 00000000003706e0
[  333.181092] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  333.188251] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
# ok 47 Test creation and deletion of trace instances

Log link:
 - https://lkft.validation.linaro.org/scheduler/job/6983932#L1655
 - https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/tuxmake_reproducer.sh

metadata:
  git_ref: master
  git_repo: https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
  git_ref: trace/core
  kernel-config:
    https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/config
  artifact-location:
    https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/
  toolchain: gcc-13

--
Linaro LKFT
https://lkft.linaro.org


> -- Steve

Re: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[Steven Rostedt]

On Mon, 30 Oct 2023 12:37:08 +0530
Naresh Kamboju <naresh.kamboju@linaro.org> wrote:

> 
> I have tested the linux-trace.git trace/core and run selftests ftrace
> the reported kernel panic [1] & [2] has been fixed but found

Good to know. Can I add "Tested-by" from you for that bug fix?

> "general protection fault"  at  kernel/trace/trace_events.c:2439.

Can you test with the below patch?

Also, can I ask what are you testing this on that makes it trigger so
easily? As I'm not able to trigger these in my tests, even though they are
indeed bugs.

-- Steve

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 7ad7496bd597..7a0b54ddda24 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -609,7 +609,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
 		entry = &ei->entries[i];
 		if (strcmp(name, entry->name) == 0) {
 			void *cdata = data;
-			r = entry->callback(name, &mode, &cdata, &fops);
+			mutex_lock(&eventfs_mutex);
+			/* If ei->is_freed, then the event itself may be too */
+			if (!ei->is_freed)
+				r = entry->callback(name, &mode, &cdata, &fops);
+			else
+				r = -1;
+			mutex_unlock(&eventfs_mutex);
 			if (r <= 0)
 				continue;
 			ret = simple_lookup(dir, dentry, flags);
@@ -743,7 +749,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
 		void *cdata = data;
 		entry = &ei->entries[i];
 		name = entry->name;
-		r = entry->callback(name, &mode, &cdata, &fops);
+		mutex_lock(&eventfs_mutex);
+		/* If ei->is_freed, then the event itself may be too */
+		if (!ei->is_freed)
+			r = entry->callback(name, &mode, &cdata, &fops);
+		else
+			r = -1;
+		mutex_unlock(&eventfs_mutex);
 		if (r <= 0)
 			continue;
 		d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);

Re: [PATCH v2] eventfs: Test for ei->is_freed when accessing ei->dentry

[Naresh Kamboju]

On Mon, 30 Oct 2023 at 20:11, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 30 Oct 2023 12:37:08 +0530
> Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>
> >
> > I have tested the linux-trace.git trace/core and run selftests ftrace
> > the reported kernel panic [1] & [2] has been fixed but found
>
> Good to know. Can I add "Tested-by" from you for that bug fix?

Please add,
Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

>
> > "general protection fault"  at  kernel/trace/trace_events.c:2439.
>
> Can you test with the below patch?

Applied this patch on top of linux-trace.git trace/core and test passed.
All the reported issues have been resolved.

>
> Also, can I ask what are you testing this on that makes it trigger so
> easily? As I'm not able to trigger these in my tests, even though they are
> indeed bugs.

I use following build artifacts and running selftests: ftrace: ftracetest-ktap

   kernel:
      url:
        https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/bzImage
    modules:
      url:
        https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/modules.tar.xz
      compression: xz
    nfsrootfs:
      url: https://storage.tuxboot.com/debian/bookworm/amd64/rootfs.tar.xz
 kselftest:
          url:
            https://storage.tuxsuite.com/public/linaro/naresh/builds/2XT84xYJIMmKApmqOOtKhnLdCyz/kselftest.tar.xz


Steps to test:
==========

1) Boot x86 machine with above bzImage, modules, rootfs and
copy kselftest.tar.xz from above url and unzip.
2) cd /opt/kselftests/default-in-kernel/ftrace
3) ./run_kselftest.sh  -c ftrace


# selftests: ftrace: ftracetest-ktap
# unlink: cannot unlink
'/opt/kselftests/default-in-kernel/ftrace/logs/latest': No such file
or directory
# TAP version 13
# 1..129
# ok 1 Basic trace file check
...
# # Totals: pass:126 faii:0 xfail:1 xpass:0 skip:1 error:1


Test logs:
https://lkft.validation.linaro.org/scheduler/job/6985018#L4742




>
> -- Steve
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 7ad7496bd597..7a0b54ddda24 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -609,7 +609,13 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
>                 entry = &ei->entries[i];
>                 if (strcmp(name, entry->name) == 0) {
>                         void *cdata = data;
> -                       r = entry->callback(name, &mode, &cdata, &fops);
> +                       mutex_lock(&eventfs_mutex);
> +                       /* If ei->is_freed, then the event itself may be too */
> +                       if (!ei->is_freed)
> +                               r = entry->callback(name, &mode, &cdata, &fops);
> +                       else
> +                               r = -1;
> +                       mutex_unlock(&eventfs_mutex);
>                         if (r <= 0)
>                                 continue;
>                         ret = simple_lookup(dir, dentry, flags);
> @@ -743,7 +749,13 @@ static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
>                 void *cdata = data;
>                 entry = &ei->entries[i];
>                 name = entry->name;
> -               r = entry->callback(name, &mode, &cdata, &fops);
> +               mutex_lock(&eventfs_mutex);
> +               /* If ei->is_freed, then the event itself may be too */
> +               if (!ei->is_freed)
> +                       r = entry->callback(name, &mode, &cdata, &fops);
> +               else
> +                       r = -1;
> +               mutex_unlock(&eventfs_mutex);
>                 if (r <= 0)
>                         continue;
>                 d = create_file_dentry(ei, i, parent, name, mode, cdata, fops, false);