Re: [PATCH] kernfs: use atomics for open_node refcounts

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@arista.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