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@xxxxxx>
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@xxxxxxxxx). 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
