Re: [PATCH v2 7/7] nvmem: synchronize nvmem device unregistering with SRCU

[Johan Hovold <johan@kernel.org>]

On Mon, Feb 23, 2026 at 11:57:08AM +0100, Bartosz Golaszewski wrote:
> With the provider-owned data split out into a separate structure, we can
> now protect it with SRCU.
> 
> Protect all dereferences of nvmem->impl with an SRCU read lock.
> Synchronize SRCU in nvmem_unregister() after setting the implementation
> pointer to NULL. This has the effect of numbing down the device after
> nvmem_unregister() returns - it will no longer accept any consumer calls
> and return -ENODEV. The actual device will live on for as long as there
> are references to it but we will no longer reach into the consumer's
> memory which may be gone by this time.
> 
> The change has the added benefit of dropping the - now redundant -
> reference counting with kref. We are left with a single release()
> function depending on the kobject reference counting provided by struct
> device.

You're actually fixing two separate issues here. And one of those issues
stem from the broken kref implementation added by:

	c1de7f43bd84 ("nvmem: use kref")

Sure, the code was already broken (by returning -EBUSY when there were
references) but the above commit deferred deregistration until the last
reference is gone which is just wrong.

So the first issue is the deferred deregistration (kref) which allows
further lookups after the provider is gone and which keeps the sysfs
interface around which can lead to use-after-free.

The second issue is that other driver can try to access the nvmem after
it's gone and that bit you can fix with SRCU. But note that you don't
need that for sysfs as kernfs already drains any active user at
deregistration.

So I think this should be split in two after dropping some of the
unnecessary sysfs bits.

> @@ -289,10 +298,14 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>  
>  static umode_t nvmem_bin_attr_get_umode(struct nvmem_device *nvmem)
>  {
> -	struct nvmem_impl *impl = nvmem->impl;
> -
> +	struct nvmem_impl *impl;
>  	umode_t mode = 0400;
>  
> +	guard(srcu)(&nvmem->srcu);
> +	impl = rcu_dereference(nvmem->impl);
> +	if (!impl)
> +		return 0;
> +
>  	if (!nvmem->root_only)
>  		mode |= 0044;
>  
> @@ -333,7 +346,12 @@ static umode_t nvmem_attr_is_visible(struct kobject *kobj,
>  {
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct nvmem_device *nvmem = to_nvmem_device(dev);
> -	struct nvmem_impl *impl = nvmem->impl;
> +	struct nvmem_impl *impl;
> +
> +	guard(srcu)(&nvmem->srcu);
> +	impl = rcu_dereference(nvmem->impl);
> +	if (!impl)
> +		return 0;
>  
>  	/*
>  	 * If the device has no .reg_write operation, do not allow

These functions are only called during registration (and don't even
dereference the callback pointers) so adding locking here is a bit
misleading.

Perhaps you can just add another flag in a preparatory patch to stop
accessing the ops directly.

Note that read_only is already set if there is no write callback, so
you'd only need a flag for write_only.

> @@ -460,10 +478,9 @@ static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
>  	return 0;
>  }
>  
> -static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem,
> -			      const struct nvmem_config *config)
> +static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem)
>  {
> -	if (config->compat)
> +	if (nvmem->flags & FLAG_COMPAT)
>  		device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
>  }

The compat attribute should also be removed at deregistration so this
may not be needed.
 
> +static void nvmem_release(struct device *dev)
> +{
> +	struct nvmem_device *nvmem = to_nvmem_device(dev);
> +
> +	gpiod_put(nvmem->wp_gpio);

> +	nvmem_sysfs_remove_compat(nvmem);

This one should be moved to nvmem_unregister().

> +	nvmem_device_remove_all_cells(nvmem);
> +	ida_free(&nvmem_ida, nvmem->id);
> +	cleanup_srcu_struct(&nvmem->srcu);
> +	kfree(nvmem);
> +}

Johan