| Message ID | 20230809125628.529884-1-sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] blk-crypto: dynamically allocate fallback profile | expand |
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.
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.
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
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
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.
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.
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.
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
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.
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:
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(-)