From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 83393357A20 for ; Tue, 17 Mar 2026 11:31:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773747086; cv=none; b=SAxrp8cRvtEeQtdASUVWKsrSHCZYMA9tiD3ml5fQKNKZITNeoCnc45jqbziSaboTKAcP7GN4IOxfyLSAFFFBfgStzOgGXRbXhlnprXlB1nAiJbciBVY7ysalKmPKO8CSyZ4yVFhUllIqAuXhpqlQIlWzRxAsxHooP2WkyqHbrxg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773747086; c=relaxed/simple; bh=gb0g6Pc9FRwbGnSeWWVSEuB8Phl4j+e88wpdyZQtVdo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=E0Cs2flDBm/v/v4AbDMMAO4AaALAGbHxpJOUFHcyDrAIb2ssDl3p/OnPNhg3LmejwO1ZLoDgzhqdgJ/FO8LtV9qLul6UGdh3sqlnqO6A3GfymzpwQ1Gm4vcUnU0OIClL7FO9cdJEYzCpeH8DOP4Oh1WraTET9cUXWusvbeMKrmg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=JXAxADsG; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="JXAxADsG" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 28E2DC4CEF7; Tue, 17 Mar 2026 11:31:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1773747086; bh=gb0g6Pc9FRwbGnSeWWVSEuB8Phl4j+e88wpdyZQtVdo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JXAxADsGJNlkHqabl42lb98mCZdghA1WnzPLjX5a41SkwsBjSXrxdaNyTh0Zyg95q NG0A6LhnKm807VHe+1IUrb3pf2Ax8gpOQxywlraJLB32wDwnHMFNQT3ZYCEYIE89ql IkTFiVDsfogd80fj2EGFcY0WsVe4gFFO2QSbqnxRs9lENR3ZbH3rwOzfQ56JeV9rGf WZOnf3UG7CoDI3Ws97mzg2VIJoOlIjS/F3djt7wiMWvNZAx7VDjezueT9fTW7mM62t IIGa1XOTcJPbpiTM40upm870hHglafbl6XBrnT11VIVZ1t2EivErWHga2qcyp4Q6U1 G00B5HPsXk5zA== Received: from johan by xi.lan with local (Exim 4.98.2) (envelope-from ) id 1w2Sdw-000000007h6-01NT; Tue, 17 Mar 2026 12:31:24 +0100 Date: Tue, 17 Mar 2026 12:31:24 +0100 From: Johan Hovold To: Bartosz Golaszewski Cc: Srinivas Kandagatla , Bartosz Golaszewski , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 7/7] nvmem: synchronize nvmem device unregistering with SRCU Message-ID: References: <20260223-nvmem-unbind-v2-0-0df33a933dca@oss.qualcomm.com> <20260223-nvmem-unbind-v2-7-0df33a933dca@oss.qualcomm.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260223-nvmem-unbind-v2-7-0df33a933dca@oss.qualcomm.com> 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