| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZAdOgUdqwLpUyPlc@redhat.com>
Date: Tue, 7 Mar 2023 09:47:29 -0500
From: Mike Snitzer <snitzer@...nel.org>
To: Hou Tao <houtao@...weicloud.com>
Cc: dm-devel@...hat.com, houtao1@...wei.com,
linux-kernel@...r.kernel.org, Alasdair Kergon <agk@...hat.com>,
Ignat Korchagin <ignat@...udflare.com>, mpatocka@...hat.com
Subject: Re: dm crypt: initialize tasklet in crypt_io_init()
On Mon, Mar 06 2023 at 9:12P -0500,
Hou Tao <houtao@...weicloud.com> wrote:
> Hi,
>
> On 3/7/2023 3:31 AM, Mike Snitzer wrote:
> > On Mon, Mar 06 2023 at 8:49P -0500,
> > Hou Tao <houtao@...weicloud.com> wrote:
> >
> >> From: Hou Tao <houtao1@...wei.com>
> >>
> >> When neither no_read_workqueue nor no_write_workqueue are enabled,
> >> tasklet_trylock() in crypt_dec_pending() may still return false due to
> >> an uninitialized state, and dm-crypt will do io completion in io_queue
> >> instead of current context unnecessarily.
> > Have you actually experienced this?
>
> Yes. I had written a bpftrace script to check the completion context of
> blkdev_bio_end_io_simple() when doing direct io read on dm-crypt device. The
> expected context should be unbound workers of crypt_queue, but sometimes the
> context is the bound worker of io_queue.
OK, thanks for clarifying. Curious to know the circumstance (I
thought per-bio-data is zero'd -- but it may be I'm mistaken).
I won't be marking this commit for stable@ but if others feel
differently please let me know and I'll do so. (We can always propose
it to stable@, after the fact, even if the commit header doesn't Cc
stable@)
> >> Fix it by initializing io->tasklet in crypt_io_init().
> > Really would rather avoid always calling tasklet_init(). But I can
> > optimize it away with a later patch.
>
> My first though was "io->tasklet.state = 0", but it may be fragile because it
> operated on the internal status of tasklet, so I switch to tasklet_init().
Yes, I looked into it and came up with the same hack.. and I too felt
it was too fragile due to open-coding direct access to the tasklet's
members.
I have a patch I just staged that staged that uses jump_labels to
optimize this code. If you might review/test/verify it works well for
you that'd be appreciated:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.3&id=ae75a25bd83f7c541240449d2fff3a44433e506b
It builds on your patch, which I added a comment to:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.3&id=d9fe0a98a2e0a1cf585e8a6555afb33be968bd13
From: Mike Snitzer <snitzer@...nel.org>
Date: Mon, 6 Mar 2023 15:58:33 -0500
Subject: [PATCH] dm crypt: conditionally enable code needed for tasklet usecases
Use jump_label to limit the need for branching, and tasklet_init(),
unless either of the optional "no_read_workqueue" and/or
"no_write_workqueue" features are used.
Signed-off-by: Mike Snitzer <snitzer@...nel.org>
---
drivers/md/dm-crypt.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 641457e72603..2d0309ca07f5 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -40,6 +40,7 @@
#include <keys/user-type.h>
#include <keys/encrypted-type.h>
#include <keys/trusted-type.h>
+#include <linux/jump_label.h>
#include <linux/device-mapper.h>
@@ -85,6 +86,8 @@ struct dm_crypt_io {
struct rb_node rb_node;
} CRYPTO_MINALIGN_ATTR;
+static DEFINE_STATIC_KEY_FALSE(use_tasklet_enabled);
+
struct dm_crypt_request {
struct convert_context *ctx;
struct scatterlist sg_in[4];
@@ -1730,12 +1733,15 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
io->sector = sector;
io->error = 0;
io->ctx.r.req = NULL;
- /*
- * tasklet_init() here to ensure crypt_dec_pending()'s
- * tasklet_trylock() doesn't incorrectly return false
- * even when tasklet isn't in use.
- */
- tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
+ if (static_branch_unlikely(&use_tasklet_enabled)) {
+ /*
+ * tasklet_init() here to ensure crypt_dec_pending()'s
+ * tasklet_trylock() doesn't incorrectly return false
+ * even when tasklet isn't in use.
+ */
+ tasklet_init(&io->tasklet, kcryptd_crypt_tasklet,
+ (unsigned long)&io->work);
+ }
io->integrity_metadata = NULL;
io->integrity_metadata_from_pool = false;
atomic_set(&io->io_pending, 0);
@@ -1775,6 +1781,10 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
kfree(io->integrity_metadata);
base_bio->bi_status = error;
+ if (!static_branch_unlikely(&use_tasklet_enabled)) {
+ bio_endio(base_bio);
+ return;
+ }
/*
* If we are running this function from our tasklet,
@@ -2232,8 +2242,9 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
{
struct crypt_config *cc = io->cc;
- if ((bio_data_dir(io->base_bio) == READ && test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags)) ||
- (bio_data_dir(io->base_bio) == WRITE && test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))) {
+ if (static_branch_unlikely(&use_tasklet_enabled) &&
+ ((bio_data_dir(io->base_bio) == READ && test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags)) ||
+ (bio_data_dir(io->base_bio) == WRITE && test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags)))) {
/*
* in_hardirq(): Crypto API's skcipher_walk_first() refuses to work in hard IRQ context.
* irqs_disabled(): the kernel may run some IO completion from the idle thread, but
@@ -2746,6 +2757,10 @@ static void crypt_dtr(struct dm_target *ti)
crypt_calculate_pages_per_client();
spin_unlock(&dm_crypt_clients_lock);
+ if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) ||
+ test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
+ static_branch_dec(&use_tasklet_enabled);
+
dm_audit_log_dtr(DM_MSG_PREFIX, ti, 1);
}
@@ -3375,6 +3390,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
ti->limit_swap_bios = true;
ti->accounts_remapped_io = true;
+ if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) ||
+ test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
+ static_branch_inc(&use_tasklet_enabled);
+
dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1);
return 0;
--
2.37.1 (Apple Git-137.1)
Powered by blists - more mailing lists