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

From: Eric Biggers <ebiggers@kernel.org>
To: Christoph Hellwig <hch@lst.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: 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: Wed, 20 Apr 2022 12:41:53 -0700	[thread overview]
Message-ID: <YmBiAQ/IZbFhRc6o@sol.localdomain> (raw)
In-Reply-To: <20220420064745.1119823-3-hch@lst.de>

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?

>  int blk_crypto_sysfs_register(struct request_queue *q)
>  {
> -	struct blk_crypto_kobj *obj;
>  	int err;
>  
>  	if (!q->crypto_profile)
>  		return 0;
>  
> -	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> -	if (!obj)
> -		return -ENOMEM;
> -	obj->profile = q->crypto_profile;
> -
> -	err = kobject_init_and_add(&obj->kobj, &blk_crypto_ktype, &q->kobj,
> -				   "crypto");
> -	if (err) {
> -		kobject_put(&obj->kobj);
> -		return err;
> -	}
> -	q->crypto_kobject = &obj->kobj;
> -	return 0;
> +	err = kobject_add(&q->crypto_profile->kobj, &q->kobj, "crypto");
> +	if (err)
> +		kobject_put(&q->crypto_profile->kobj);
> +	return err;
>  }

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.

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.

- Eric