[PATCH v2] blk-crypto: dynamically allocate fallback profile

[PATCH v2] blk-crypto: dynamically allocate fallback profile

[Sweet Tea Dorminy]

blk_crypto_profile_init() calls lockdep_register_key(), which warns and
does not register if the provided memory is a static object.
blk-crypto-fallback currently has a static blk_crypto_profile and calls
blk_crypto_profile_init() thereupon, resulting in the warning and
failure to register.

Fortunately it is simple enough to use a dynamically allocated profile
and make lockdep function correctly.

Fixes: 2fb48d88e77f ("blk-crypto: use dynamic lock class for blk_crypto_profile::lock")
Cc: stable@vger.kernel.org
Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
v2: reworded commit message, fixed Fixes tag, as pointed out by Eric
Biggers.

 block/blk-crypto-fallback.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index ad9844c5b40c..de94e9bffec6 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -78,7 +78,7 @@ static struct blk_crypto_fallback_keyslot {
 	struct crypto_skcipher *tfms[BLK_ENCRYPTION_MODE_MAX];
 } *blk_crypto_keyslots;
 
-static struct blk_crypto_profile blk_crypto_fallback_profile;
+static struct blk_crypto_profile *blk_crypto_fallback_profile;
 static struct workqueue_struct *blk_crypto_wq;
 static mempool_t *blk_crypto_bounce_page_pool;
 static struct bio_set crypto_bio_split;
@@ -292,7 +292,7 @@ static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
 	 * Get a blk-crypto-fallback keyslot that contains a crypto_skcipher for
 	 * this bio's algorithm and key.
 	 */
-	blk_st = blk_crypto_get_keyslot(&blk_crypto_fallback_profile,
+	blk_st = blk_crypto_get_keyslot(blk_crypto_fallback_profile,
 					bc->bc_key, &slot);
 	if (blk_st != BLK_STS_OK) {
 		src_bio->bi_status = blk_st;
@@ -395,7 +395,7 @@ static void blk_crypto_fallback_decrypt_bio(struct work_struct *work)
 	 * Get a blk-crypto-fallback keyslot that contains a crypto_skcipher for
 	 * this bio's algorithm and key.
 	 */
-	blk_st = blk_crypto_get_keyslot(&blk_crypto_fallback_profile,
+	blk_st = blk_crypto_get_keyslot(blk_crypto_fallback_profile,
 					bc->bc_key, &slot);
 	if (blk_st != BLK_STS_OK) {
 		bio->bi_status = blk_st;
@@ -499,7 +499,7 @@ bool blk_crypto_fallback_bio_prep(struct bio **bio_ptr)
 		return false;
 	}
 
-	if (!__blk_crypto_cfg_supported(&blk_crypto_fallback_profile,
+	if (!__blk_crypto_cfg_supported(blk_crypto_fallback_profile,
 					&bc->bc_key->crypto_cfg)) {
 		bio->bi_status = BLK_STS_NOTSUPP;
 		return false;
@@ -526,7 +526,7 @@ bool blk_crypto_fallback_bio_prep(struct bio **bio_ptr)
 
 int blk_crypto_fallback_evict_key(const struct blk_crypto_key *key)
 {
-	return __blk_crypto_evict_key(&blk_crypto_fallback_profile, key);
+	return __blk_crypto_evict_key(blk_crypto_fallback_profile, key);
 }
 
 static bool blk_crypto_fallback_inited;
@@ -534,29 +534,32 @@ static int blk_crypto_fallback_init(void)
 {
 	int i;
 	int err;
-	struct blk_crypto_profile *profile = &blk_crypto_fallback_profile;
 
 	if (blk_crypto_fallback_inited)
 		return 0;
 
 	get_random_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
 
+	blk_crypto_fallback_profile =
+		kzalloc(sizeof(*blk_crypto_fallback_profile), GFP_KERNEL);
+
 	err = bioset_init(&crypto_bio_split, 64, 0, 0);
 	if (err)
 		goto out;
 
-	err = blk_crypto_profile_init(profile, blk_crypto_num_keyslots);
+	err = blk_crypto_profile_init(blk_crypto_fallback_profile,
+				      blk_crypto_num_keyslots);
 	if (err)
 		goto fail_free_bioset;
 	err = -ENOMEM;
 
-	profile->ll_ops = blk_crypto_fallback_ll_ops;
-	profile->max_dun_bytes_supported = BLK_CRYPTO_MAX_IV_SIZE;
+	blk_crypto_fallback_profile->ll_ops = blk_crypto_fallback_ll_ops;
+	blk_crypto_fallback_profile->max_dun_bytes_supported = BLK_CRYPTO_MAX_IV_SIZE;
 
 	/* All blk-crypto modes have a crypto API fallback. */
 	for (i = 0; i < BLK_ENCRYPTION_MODE_MAX; i++)
-		profile->modes_supported[i] = 0xFFFFFFFF;
-	profile->modes_supported[BLK_ENCRYPTION_MODE_INVALID] = 0;
+		blk_crypto_fallback_profile->modes_supported[i] = 0xFFFFFFFF;
+	blk_crypto_fallback_profile->modes_supported[BLK_ENCRYPTION_MODE_INVALID] = 0;
 
 	blk_crypto_wq = alloc_workqueue("blk_crypto_wq",
 					WQ_UNBOUND | WQ_HIGHPRI |
@@ -597,7 +600,7 @@ static int blk_crypto_fallback_init(void)
 fail_free_wq:
 	destroy_workqueue(blk_crypto_wq);
 fail_destroy_profile:
-	blk_crypto_profile_destroy(profile);
+	blk_crypto_profile_destroy(blk_crypto_fallback_profile);
 fail_free_bioset:
 	bioset_exit(&crypto_bio_split);
 out:
-- 
2.41.0

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Bart Van Assche]

On 8/9/23 05:56, Sweet Tea Dorminy wrote:
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Signed-off-by should be followed by a real name. Sweet Tea doesn't look
like a real name to me.

Bart.

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Sweet Tea Dorminy]

On 8/9/23 17:57, Bart Van Assche wrote:
> On 8/9/23 05:56, Sweet Tea Dorminy wrote:
>> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> 
> Signed-off-by should be followed by a real name. Sweet Tea doesn't look
> like a real name to me.

I do go by Sweet Tea in real life as well as online.

And the DCO changed to say 'known identity' instead of 'real name' in 
6.3: 
https://github.com/torvalds/linux/commit/d4563201f33a022fc0353033d9dfeb1606a88330. 

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Jens Axboe]

On 8/9/23 6:56 AM, Sweet Tea Dorminy wrote:
> blk_crypto_profile_init() calls lockdep_register_key(), which warns and
> does not register if the provided memory is a static object.
> blk-crypto-fallback currently has a static blk_crypto_profile and calls
> blk_crypto_profile_init() thereupon, resulting in the warning and
> failure to register.
> 
> Fortunately it is simple enough to use a dynamically allocated profile
> and make lockdep function correctly.
> 
> Fixes: 2fb48d88e77f ("blk-crypto: use dynamic lock class for blk_crypto_profile::lock")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

The offending commit went into 6.5, so there should be no need for a
stable tag on this one. But I can edit that while applying, waiting on
Eric to ack it.

-- 
Jens Axboe

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Greg KH]

On Wed, Aug 09, 2023 at 04:08:52PM -0600, Jens Axboe wrote:
> On 8/9/23 6:56 AM, Sweet Tea Dorminy wrote:
> > blk_crypto_profile_init() calls lockdep_register_key(), which warns and
> > does not register if the provided memory is a static object.
> > blk-crypto-fallback currently has a static blk_crypto_profile and calls
> > blk_crypto_profile_init() thereupon, resulting in the warning and
> > failure to register.
> > 
> > Fortunately it is simple enough to use a dynamically allocated profile
> > and make lockdep function correctly.
> > 
> > Fixes: 2fb48d88e77f ("blk-crypto: use dynamic lock class for blk_crypto_profile::lock")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> 
> The offending commit went into 6.5, so there should be no need for a
> stable tag on this one. But I can edit that while applying, waiting on
> Eric to ack it.

That commit has been backported to stable releases, so it would be nice
to keep it there so our tools automatically pick it up properly.  Once
the authorship name is fixed up of course.

thanks,

greg k-h

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Jens Axboe]

On 8/9/23 10:53 PM, Greg KH wrote:
> On Wed, Aug 09, 2023 at 04:08:52PM -0600, Jens Axboe wrote:
>> On 8/9/23 6:56 AM, Sweet Tea Dorminy wrote:
>>> blk_crypto_profile_init() calls lockdep_register_key(), which warns and
>>> does not register if the provided memory is a static object.
>>> blk-crypto-fallback currently has a static blk_crypto_profile and calls
>>> blk_crypto_profile_init() thereupon, resulting in the warning and
>>> failure to register.
>>>
>>> Fortunately it is simple enough to use a dynamically allocated profile
>>> and make lockdep function correctly.
>>>
>>> Fixes: 2fb48d88e77f ("blk-crypto: use dynamic lock class for blk_crypto_profile::lock")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
>>
>> The offending commit went into 6.5, so there should be no need for a
>> stable tag on this one. But I can edit that while applying, waiting on
>> Eric to ack it.
> 
> That commit has been backported to stable releases, so it would be nice
> to keep it there so our tools automatically pick it up properly.  Once
> the authorship name is fixed up of course.

But that stable tag should not be necessary? If stable has backported a
commit, surely it'll pick a commit that has that in Fixes? Otherwise
that seems broken and implies that people need to potentially check
every commit for a stable presence.

I can keep the tag, just a bit puzzled as to why that would be
necessary.

The authorship is fine, but looks like the patch needs changes anyway as
per Eric.

-- 
Jens Axboe

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Greg KH]

On Thu, Aug 10, 2023 at 07:18:27AM -0600, Jens Axboe wrote:
> On 8/9/23 10:53 PM, Greg KH wrote:
> > On Wed, Aug 09, 2023 at 04:08:52PM -0600, Jens Axboe wrote:
> >> On 8/9/23 6:56 AM, Sweet Tea Dorminy wrote:
> >>> blk_crypto_profile_init() calls lockdep_register_key(), which warns and
> >>> does not register if the provided memory is a static object.
> >>> blk-crypto-fallback currently has a static blk_crypto_profile and calls
> >>> blk_crypto_profile_init() thereupon, resulting in the warning and
> >>> failure to register.
> >>>
> >>> Fortunately it is simple enough to use a dynamically allocated profile
> >>> and make lockdep function correctly.
> >>>
> >>> Fixes: 2fb48d88e77f ("blk-crypto: use dynamic lock class for blk_crypto_profile::lock")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> >>
> >> The offending commit went into 6.5, so there should be no need for a
> >> stable tag on this one. But I can edit that while applying, waiting on
> >> Eric to ack it.
> > 
> > That commit has been backported to stable releases, so it would be nice
> > to keep it there so our tools automatically pick it up properly.  Once
> > the authorship name is fixed up of course.
> 
> But that stable tag should not be necessary? If stable has backported a
> commit, surely it'll pick a commit that has that in Fixes? Otherwise
> that seems broken and implies that people need to potentially check
> every commit for a stable presence.
> 
> I can keep the tag, just a bit puzzled as to why that would be
> necessary.

It's not necessary, no, our scripts will pick it out and get it merged
eventually.  But if you know it's needed to start with, it's always nice
to add it if possible, saves me the extra work :)

thanks,

greg k-h

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Jens Axboe]

On 8/10/23 7:41 AM, Greg KH wrote:
> On Thu, Aug 10, 2023 at 07:18:27AM -0600, Jens Axboe wrote:
>> On 8/9/23 10:53 PM, Greg KH wrote:
>>> On Wed, Aug 09, 2023 at 04:08:52PM -0600, Jens Axboe wrote:
>>>> On 8/9/23 6:56 AM, Sweet Tea Dorminy wrote:
>>>>> blk_crypto_profile_init() calls lockdep_register_key(), which warns and
>>>>> does not register if the provided memory is a static object.
>>>>> blk-crypto-fallback currently has a static blk_crypto_profile and calls
>>>>> blk_crypto_profile_init() thereupon, resulting in the warning and
>>>>> failure to register.
>>>>>
>>>>> Fortunately it is simple enough to use a dynamically allocated profile
>>>>> and make lockdep function correctly.
>>>>>
>>>>> Fixes: 2fb48d88e77f ("blk-crypto: use dynamic lock class for blk_crypto_profile::lock")
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
>>>>
>>>> The offending commit went into 6.5, so there should be no need for a
>>>> stable tag on this one. But I can edit that while applying, waiting on
>>>> Eric to ack it.
>>>
>>> That commit has been backported to stable releases, so it would be nice
>>> to keep it there so our tools automatically pick it up properly.  Once
>>> the authorship name is fixed up of course.
>>
>> But that stable tag should not be necessary? If stable has backported a
>> commit, surely it'll pick a commit that has that in Fixes? Otherwise
>> that seems broken and implies that people need to potentially check
>> every commit for a stable presence.
>>
>> I can keep the tag, just a bit puzzled as to why that would be
>> necessary.
> 
> It's not necessary, no, our scripts will pick it out and get it merged
> eventually.  But if you know it's needed to start with, it's always nice
> to add it if possible, saves me the extra work :)

OK, makes sense.

-- 
Jens Axboe

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Eric Biggers]

On Wed, Aug 09, 2023 at 08:56:22AM -0400, Sweet Tea Dorminy wrote:
>  
> +	blk_crypto_fallback_profile =
> +		kzalloc(sizeof(*blk_crypto_fallback_profile), GFP_KERNEL);
> +

I think you missed part of my feedback on v1.

- Eric

Re: [PATCH v2] blk-crypto: dynamically allocate fallback profile

[Sweet Tea Dorminy]

On 8/10/23 00:59, Eric Biggers wrote:
> On Wed, Aug 09, 2023 at 08:56:22AM -0400, Sweet Tea Dorminy wrote:
>>   
>> +	blk_crypto_fallback_profile =
>> +		kzalloc(sizeof(*blk_crypto_fallback_profile), GFP_KERNEL);
>> +
> 
> I think you missed part of my feedback on v1.
> 
> - Eric

You're absolutely right, I completely missed that there were code 
changes. I'll fixup and resend.