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

[Eric Biggers <ebiggers@kernel.org>]

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