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

From: Christoph Hellwig <hch@lst.de>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	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:25:35 +0200	[thread overview]
Message-ID: <20220421142535.GA21025@lst.de> (raw)
In-Reply-To: <YmBiAQ/IZbFhRc6o@sol.localdomain>

On Wed, Apr 20, 2022 at 12:41:53PM -0700, Eric Biggers wrote:
> 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.

I don't think all driver authors agree with that assumption (and it isn't
documented anywhere). 

The most trivial case is device mapper, where the crypto profіle is freed
before putting the gendisk reference acquired by blk_alloc_disk.  So
anyone who opened the sysfs file at some point before the delete can
still have it open and triviall access freed memory when then doing
a read on it after the dm table is freed.

For UFS things seem to work out ok because the ufs_hba is part of
the Scsi_Host, which is the parent device of the gendisk.

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

Yes, every queue/ object that references the gendisk has a problem I think.
I've been wading through this code and trying to fix it, which made me
notice this code.

> In any case, this proposal is not correct since it is assuming that each
> blk_crypto_profile is referenced by only one request_queue, which is not
> necessarily the case since a host controller can have multiple disks.
> The same kobject can't be added to multiple places in the hierarchy.

Indeed.

> If we did need to do something differently here, I think we'd either need to put
> the blk_crypto_profile kobject under the host controller one and link to it from
> the queue directories (which I mentioned in commit 20f01f163203 as an
> alternative considered), or duplicate the crypto capabilities in each
> request_queue and only share the actual keyslot management data structures.

Do we even need the link instead of letting the user walk down the
directory hierachy?  But yes, having the sysfs attributes on the
actual object is a much better idea.

> 
> - Eric
---end quoted text---