linux-kernel - Re: [PATCH] kernfs: use atomics for open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2025112056-acorn-skeptic-63cc@gregkh>
Date: Thu, 20 Nov 2025 02:20:39 -0500
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Kevin Mitchell <kevmitch@...sta.com>
Cc: Tejun Heo <tj@...nel.org>, Chengming Zhou <zhouchengming@...edance.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernfs: use atomics for open_node refcounts

On Wed, Nov 19, 2025 at 11:17:57AM -0800, Kevin Mitchell wrote:
> Counts of files to be released and mmapped files were added to
> kernfs_open_node in commit bdb2fd7fc56e ("kernfs: Skip
> kernfs_drain_open_files() more aggressively") to optimize
> kernfs_drain_open_files(). A WARN_ON_ONCE sanity check was also added in
> kernfs_drain_open_files to ensure that these counters were brought to
> zero once all files had been drained.
> 
> This WARNING was found to trigger on occasion during pci hotplug link
> down when mmapped pci resource files were removed in
> pci_remove_resource_files. This was caused by a race condition during
> the initial mmappings where incrementing nr_mmapped from multiple CPUs
> such that 2 (or possibly more) simultaneous increments overwrote each
> other thus appearing as only a single increment. The correct number of
> kernfs_open_files with mmapped == true were nevertheless still present
> in the kernfs_open_node's files list. Consequently, the iteration in
> kernfs_drain_open_files would underflow nr_mmapped and the WARNING would
> fire.
> 
> Since both nr_mmapped and nr_to_release may be incremented and
> decremented from multiple CPUs simultaneously without acquiring the
> kernfs_open_file_mutex_lock, make these members atomic. This should
> ensure that simultaneous increments and decrements don't overwrite each
> other.
> 
> Fixes: bdb2fd7fc56e ("kernfs: Skip kernfs_drain_open_files() more aggressively")
> Signed-off-by: Kevin Mitchell <kevmitch@...sta.com>
> ---
>  fs/kernfs/file.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 9adf36e6364b..a9d0bf82ceba 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -23,8 +23,8 @@ struct kernfs_open_node {
>  	atomic_t		event;
>  	wait_queue_head_t	poll;
>  	struct list_head	files; /* goes through kernfs_open_file.list */
> -	unsigned int		nr_mmapped;
> -	unsigned int		nr_to_release;
> +	atomic_t		nr_mmapped;
> +	atomic_t		nr_to_release;

This feels wrong, shouldn't a lock be handling the access to these files
instead?  Having 3 atomic_t variables in the same structure feels like
overkill, just use a single lock to make the contention be less?

thanks,

greg k-h