All of lore.kernel.org
 help / color / mirror / Atom feed
From: Shakeel Butt <shakeel.butt@linux.dev>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tejun Heo <tj@kernel.org>,
	Christian Brauner <christian@brauner.io>,
	 Meta kernel team <kernel-team@meta.com>,
	linux-kselftest@vger.kernel.org, driver-core@lists.linux.dev,
	 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] kernfs: take kernfs_rename_lock for same-parent renames too
Date: Wed, 2 Sep 2026 22:37:21 -0700	[thread overview]
Message-ID: <20260903053226.GA15581@shakeel.butt@linux.dev> (raw)
In-Reply-To: <2026090330-cytoplasm-resilient-900b@gregkh>

On Thu, Sep 03, 2026 at 06:31:58AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 02, 2026 at 09:02:51PM -0700, Shakeel Butt wrote:
> > kernfs_rename_ns() only takes kernfs_rename_lock when the rename moves
> > the node to a new parent.  A rename that keeps the same parent, like
> > renaming a network interface, changes kernfs_node::name with only
> > kernfs_rwsem held.  So the lock protects ->__parent but not ->name, and
> > a reader that wants a stable name has to take kernfs_rwsem, the same
> > lock every path lookup needs.
> > 
> > That also makes for a small but real bug.  kernfs_path_from_node()
> > takes kernfs_rename_lock for reading, and kernfs_path_from_node_locked()
> > then reads the name of each ancestor.  It reads each one once, so a
> > single same-parent rename only moves the answer from the old path to the
> > new one, but two of them landing inside one walk build a path that never
> > existed:
> > 
> >   CPU0                                   CPU1
> >   kernfs_path_from_node() on /a/b/c
> >     reads the name of a, gets "a"
> >                                          renames a to a2
> >                                          renames b to b2
> >     reads the name of b, gets "b2"
> >     returns "/a/b2/c"
> > 
> > This hits roots without KERNFS_ROOT_INVARIANT_PARENT: sysfs, where the
> > bad path can reach sysfs_warn_dup() and pr_cont_kernfs_path(), and
> > resctrl, which renames a mon group inside its mon_groups directory.
> > cgroup sets the flag, so it skips the lock and reads names under RCU
> > alone; that case needs something else and is not addressed here.
> > 
> > So take the lock in both cases, and let kernfs_rcu_name() accept it the
> > way kernfs_parent() already does for ->__parent.  Same-parent renames
> > are rare, the lock is per filesystem, and the locked section is at most
> > three stores.  It also gives a future rename sequence counter one place
> > to sit that covers every rename.
> > 
> > Fixes: 741c10b096bc ("kernfs: Use RCU to access kernfs_node::name.")
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> > ---
> >  fs/kernfs/dir.c             | 28 +++++++++++++++-------------
> >  fs/kernfs/kernfs-internal.h |  9 ++++++++-
> >  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> How was this found and tested?  Did you forget an Assisted-by: tag?

I am working on a series to improve kernfs_rwsem and going through
review-prompt with AI to review my series and these were existing 
issues AI found. I have created reproducers with AI for these and
tested that these patches those.


  reply	other threads:[~2026-09-03  5:37 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-03  4:02 [PATCH 1/3] kernfs: take kernfs_rename_lock for same-parent renames too Shakeel Butt
2026-09-03  4:02 ` [PATCH 2/3] kernfs: don't lose IN_DELETE_SELF when decoding a file handle Shakeel Butt
2026-09-03 20:23   ` Tejun Heo
2026-09-03  4:02 ` [PATCH 3/3] kernfs: fix up the unlocked attribute reads on the creation paths Shakeel Butt
2026-09-03 20:26   ` Tejun Heo
2026-09-03 21:15     ` Shakeel Butt
2026-09-03  4:31 ` [PATCH 1/3] kernfs: take kernfs_rename_lock for same-parent renames too Greg Kroah-Hartman
2026-09-03  5:37   ` Shakeel Butt [this message]
     [not found]   ` <6a990797.3e7a366d.3bd849.72d1SMTPIN_ADDED_BROKEN@mx.google.com>
2026-09-03  5:41     ` Greg Kroah-Hartman
2026-09-03  6:15       ` Shakeel Butt
2026-09-03 16:08         ` Shakeel Butt
2026-09-03 20:21 ` Tejun Heo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260903053226.GA15581@shakeel.butt@linux.dev \
    --to=shakeel.butt@linux.dev \
    --cc=christian@brauner.io \
    --cc=driver-core@lists.linux.dev \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-team@meta.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.