Commit 77a06c33 authored by Steven Rostedt (Google)'s avatar Steven Rostedt (Google)
Browse files

eventfs: Test for ei->is_freed when accessing ei->dentry 

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/
Link: https://lkml.kernel.org/r/20231101172649.477608228@goodmis.org



Cc: Ajay Kaher <akaher@vmware.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: 5790b1fb ("eventfs: Remove eventfs_file and just use eventfs_inode")
Reported-by: default avatarLinux Kernel Functional Testing <lkft@linaro.org>
Reported-by: default avatarNaresh Kamboju <naresh.kamboju@linaro.org>
Reported-by: default avatarBeau Belgrave <beaub@linux.microsoft.com>
Reviewed-by: default avatarMasami Hiramatsu (Google) <mhiramat@kernel.org>
Tested-by: default avatarLinux Kernel Functional Testing <lkft@linaro.org>
Tested-by: default avatarNaresh Kamboju <naresh.kamboju@linaro.org>
Tested-by: default avatarBeau Belgrave <beaub@linux.microsoft.com>
Signed-off-by: default avatarSteven Rostedt (Google) <rostedt@goodmis.org>
parent db3a3972

fs/tracefs/event_inode.c

+39 −6
Original line number Diff line number Diff line
@@ -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);

	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;

}

fs/tracefs/internal.h

+2 −1
Original line number Diff line number Diff line
@@ -24,6 +24,7 @@ struct tracefs_inode { 
 * @d_children: The array of dentries to represent the files when created

 * @data:	The private data to pass to the callbacks

 * @is_freed:	Flag set if the eventfs is on its way to be freed

 *                Note if is_freed is set, then dentry is corrupted.

 * @nr_entries: The number of items in @entries

 */

struct eventfs_inode {
@@ -31,7 +32,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;