From nobody Wed Dec 31 18:17:06 2025 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D6000C41535 for ; Tue, 31 Oct 2023 19:34:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235040AbjJaTeh (ORCPT ); Tue, 31 Oct 2023 15:34:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48540 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234415AbjJaTec (ORCPT ); Tue, 31 Oct 2023 15:34:32 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2A1D9F3 for ; Tue, 31 Oct 2023 12:34:30 -0700 (PDT) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A0DACC433CC; Tue, 31 Oct 2023 19:34:29 +0000 (UTC) Received: from rostedt by gandalf with local (Exim 4.96) (envelope-from ) id 1qxuVQ-00EK8x-1j; Tue, 31 Oct 2023 15:34:28 -0400 Message-ID: <20231031193428.347017769@goodmis.org> User-Agent: quilt/0.66 Date: Tue, 31 Oct 2023 15:31:11 -0400 From: Steven Rostedt To: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org Cc: Masami Hiramatsu , Mark Rutland , Andrew Morton , "Arnd Bergmann" , "Ajay Kaher" , Linux Kernel Functional Testing , Naresh Kamboju , Beau Belgrave Subject: [PATCH v4 2/3] eventfs: Test for ei->is_freed when accessing ei->dentry References: <20231031193109.018322397@goodmis.org> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" From: "Steven Rostedt (Google)" 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=3DSoEywQOe19fGP3uR15SG= owkdK+_X85Cg@mail.gmail.com/ Link: https://lore.kernel.org/all/CA+G9fYuDP3hVQ3t7FfrBAjd_WFVSurMgCepTxunS= Jf=3DMTe=3D6aA@mail.gmail.com/ Cc: Mark Rutland Cc: "Arnd Bergmann" Cc: "Ajay Kaher" Cc: Andrew Morton Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_in= ode") Reported-by: Linux Kernel Functional Testing Reported-by: Naresh Kamboju Reported-by: Beau Belgrave Reviewed-by: Masami Hiramatsu (Google) Tested-by: Linux Kernel Functional Testing Tested-by: Naresh Kamboju Tested-by: Beau Belgrave Signed-off-by: Steven Rostedt (Google) --- Changes since v2: https://lore.kernel.org/linux-trace-kernel/20231028164650= .4f5ea18a@rorschach.local.home/ - Separated out free_ei() into its own patch fs/tracefs/event_inode.c | 50 +++++++++++++++++++++++++++++++++++----- fs/tracefs/internal.h | 3 ++- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c index 76323f48d5f0..362a2dba82c2 100644 --- a/fs/tracefs/event_inode.c +++ b/fs/tracefs/event_inode.c @@ -24,7 +24,20 @@ #include #include "internal.h" =20 +/* + * 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); =20 static struct dentry *eventfs_root_lookup(struct inode *dir, @@ -239,6 +252,10 @@ create_file_dentry(struct eventfs_inode *ei, struct de= ntry **e_dentry, bool invalidate =3D false; =20 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_inod= e *ei) struct eventfs_inode *ei_child; struct tracefs_inode *ti; =20 + 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_inod= e *ei) =20 /** * 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_in= ode *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 lo= okup) +create_dir_dentry(struct eventfs_inode *pei, struct eventfs_inode *ei, + struct dentry *parent, bool lookup) { bool invalidate =3D false; struct dentry *dentry =3D NULL; =20 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 =3D ei->dentry; @@ -440,7 +465,7 @@ static struct dentry *eventfs_root_lookup(struct inode = *dir, */ mutex_lock(&eventfs_mutex); ei =3D READ_ONCE(ti->private); - if (ei) + if (ei && !ei->is_freed) ei_dentry =3D READ_ONCE(ei->dentry); mutex_unlock(&eventfs_mutex); =20 @@ -454,7 +479,7 @@ static struct dentry *eventfs_root_lookup(struct inode = *dir, if (strcmp(ei_child->name, name) !=3D 0) continue; ret =3D simple_lookup(dir, dentry, flags); - create_dir_dentry(ei_child, ei_dentry, true); + create_dir_dentry(ei, ei_child, ei_dentry, true); created =3D true; break; } @@ -588,7 +613,7 @@ static int dcache_dir_open_wrapper(struct inode *inode,= struct file *file) =20 list_for_each_entry_srcu(ei_child, &ei->children, list, srcu_read_lock_held(&eventfs_srcu)) { - d =3D create_dir_dentry(ei_child, parent, false); + d =3D create_dir_dentry(ei, ei_child, parent, false); if (d) { ret =3D 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 =3D size; ei->data =3D data; INIT_LIST_HEAD(&ei->children); + INIT_LIST_HEAD(&ei->list); =20 mutex_lock(&eventfs_mutex); - list_add_tail(&ei->list, &parent->children); - ei->d_parent =3D parent->dentry; + if (!parent->is_freed) { + list_add_tail(&ei->list, &parent->children); + ei->d_parent =3D parent->dentry; + } mutex_unlock(&eventfs_mutex); =20 + /* Was the parent freed? */ + if (list_empty(&ei->list)) { + free_ei(ei); + ei =3D NULL; + } return ei; } =20 @@ -884,6 +917,11 @@ void eventfs_remove_dir(struct eventfs_inode *ei) for (i =3D 0; i < ei->nr_entries; i++) unhook_dentry(&ei->d_children[i], &dentry_list); unhook_dentry(&ei->dentry, &dentry_list); + /* + * 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; --=20 2.42.0