Re: [PATCH 2/2] blk-crypto: fix the blk_crypto_profile liftime - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
	Mike Snitzer <snitzer@kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Ritesh Harjani <riteshh@codeaurora.org>,
	Asutosh Das <asutoshd@codeaurora.org>,
	Alim Akhtar <alim.akhtar@samsung.com>,
	Avri Altman <avri.altman@wdc.com>,
	dm-devel@redhat.com, linux-block@vger.kernel.org,
	linux-mmc@vger.kernel.org, linux-scsi@vger.kernel.org
Subject: Re: [PATCH 2/2] blk-crypto: fix the blk_crypto_profile liftime
Date: Thu, 21 Apr 2022 16:40:15 +0200	[thread overview]
Message-ID: <YmFsz3lZt7jPUCGV@kroah.com> (raw)
In-Reply-To: <YmBiAQ/IZbFhRc6o@sol.localdomain>

On Wed, Apr 20, 2022 at 12:41:53PM -0700, Eric Biggers wrote:
> On Wed, Apr 20, 2022 at 08:47:45AM +0200, Christoph Hellwig wrote:
> > Once the blk_crypto_profile is exposed in sysfs it needs to stay alive
> > as long as sysfs accesses are possibly pending.  Ensure that by removing
> > the blk_crypto_kobj wrapper and just embedding the kobject into the
> > actual blk_crypto_profile.  This requires the blk_crypto_profile
> > structure to be dynamically allocated, which in turn requires a private
> > data pointer for driver use.
> > 
> > Fixes: 20f01f163203 ("blk-crypto: show crypto capabilities in sysfs")
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Can you elaborate on what you think the actual problem is here?  The lifetime of
> the blk_crypto_profile matches that of the host controller kobject, and I
> thought that it is not destroyed until after higher-level objects such as
> gendisks and request_queues are destroyed.  Similar assumptions are made by the
> queue kobject, which assumes it is safe to access the gendisk, and by the
> independent_access_ranges kobject which assumes it is safe to access the queue.
> 
> I suppose this wouldn't have worked with the original sysfs design where opening
> a file in sysfs actually got a refcount to the kobject.  But that's long gone,
> having been changed in Linux v2.6.23 (https://lwn.net/Articles/229774).
> 
> Note that commit 20f01f163203 which added this code got an "all looks good" from
> Greg KH (https://lore.kernel.org/r/YaH1CmHClx5WvDWD@kroah.com).  I'd have hoped
> that he would've noticed if there was a major problem with how kobjects are used
> here!  Greg, would you mind taking a look at this part again?

I do not know the model for the block devices and queues and all of that
well at all.  My first glance this seemed sane, but if the lifetime
rules are not normal in any way, I do not know.  I defer to Christoph
for all of this, he knows it way better than I do.

thanks,

greg k-h