[dm-devel] [PATCH] dm crypt: initialize tasklet in crypt_io_init()

[dm-devel] [PATCH] dm crypt: initialize tasklet in crypt_io_init()

[Hou Tao]

From: Hou Tao <houtao1@huawei.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.

Fix it by initializing io->tasklet in crypt_io_init().

Fixes: 8e14f610159d ("dm crypt: do not call bio_endio() from the dm-crypt tasklet")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3aeeb8f2802f..caee6ce3b79f 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -238,6 +238,7 @@ static void crypt_endio(struct bio *clone);
 static void kcryptd_queue_crypt(struct dm_crypt_io *io);
 static struct scatterlist *crypt_get_sg_data(struct crypt_config *cc,
 					     struct scatterlist *sg);
+static void kcryptd_crypt_tasklet(unsigned long work);
 
 static bool crypt_integrity_aead(struct crypt_config *cc);
 
@@ -1725,6 +1726,7 @@ 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(&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);
@@ -2226,7 +2228,6 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 		 * it is being executed with irqs disabled.
 		 */
 		if (in_hardirq() || irqs_disabled()) {
-			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
 			tasklet_schedule(&io->tasklet);
 			return;
 		}
-- 
2.29.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH] dm crypt: initialize tasklet in crypt_io_init()

[Hou Tao]

From: Hou Tao <houtao1@huawei.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.

Fix it by initializing io->tasklet in crypt_io_init().

Fixes: 8e14f610159d ("dm crypt: do not call bio_endio() from the dm-crypt tasklet")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/dm-crypt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3aeeb8f2802f..caee6ce3b79f 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -238,6 +238,7 @@ static void crypt_endio(struct bio *clone);
 static void kcryptd_queue_crypt(struct dm_crypt_io *io);
 static struct scatterlist *crypt_get_sg_data(struct crypt_config *cc,
 					     struct scatterlist *sg);
+static void kcryptd_crypt_tasklet(unsigned long work);
 
 static bool crypt_integrity_aead(struct crypt_config *cc);
 
@@ -1725,6 +1726,7 @@ 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(&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);
@@ -2226,7 +2228,6 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 		 * it is being executed with irqs disabled.
 		 */
 		if (in_hardirq() || irqs_disabled()) {
-			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
 			tasklet_schedule(&io->tasklet);
 			return;
 		}
-- 
2.29.2

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Mon, Mar 06 2023 at  8:49P -0500,
Hou Tao <houtao@huaweicloud.com> wrote:

> From: Hou Tao <houtao1@huawei.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?

> 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.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Mon, Mar 06 2023 at  8:49P -0500,
Hou Tao <houtao@huaweicloud.com> wrote:

> From: Hou Tao <houtao1@huawei.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?

> 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.

Mike

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Hou Tao]

Hi,

On 3/7/2023 3:31 AM, Mike Snitzer wrote:
> On Mon, Mar 06 2023 at  8:49P -0500,
> Hou Tao <houtao@huaweicloud.com> wrote:
>
>> From: Hou Tao <houtao1@huawei.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.
>
>> 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().
>
> Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Hou Tao]

Hi,

On 3/7/2023 3:31 AM, Mike Snitzer wrote:
> On Mon, Mar 06 2023 at  8:49P -0500,
> Hou Tao <houtao@huaweicloud.com> wrote:
>
>> From: Hou Tao <houtao1@huawei.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.
>
>> 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().
>
> Mike

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Mon, Mar 06 2023 at  9:12P -0500,
Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> >
> >> From: Hou Tao <houtao1@huawei.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@kernel.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@kernel.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)

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Mon, Mar 06 2023 at  9:12P -0500,
Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> >
> >> From: Hou Tao <houtao1@huawei.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@kernel.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@kernel.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)

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Hou Tao]

Hi,

On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> On Mon, Mar 06 2023 at  9:12P -0500,
> Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
>>>
>>>> From: Hou Tao <houtao1@huawei.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).
The circumstance is just a normal qemu VM running the vanilla kernel for test
purpose. According to the implementation of bio_alloc_bioset(), the front pad of
bio is not initialized and only bio itself is initialized. AFAIK if
CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.
>
> 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
Thanks for the comments. It is fine to me.
>
> From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> +	}
Because use_tasklet_enabled can be enabled concurrently, so I think it is still
possible that crypt_dec_pending will try-lock an unitialized tasklet if
use_tasklet_enabled is enabled when invoking crypt_dec_pending().
>  
>  	/*
>  	 * 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;
>  

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Hou Tao]

Hi,

On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> On Mon, Mar 06 2023 at  9:12P -0500,
> Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
>>>
>>>> From: Hou Tao <houtao1@huawei.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).
The circumstance is just a normal qemu VM running the vanilla kernel for test
purpose. According to the implementation of bio_alloc_bioset(), the front pad of
bio is not initialized and only bio itself is initialized. AFAIK if
CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.
>
> 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
Thanks for the comments. It is fine to me.
>
> From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> +	}
Because use_tasklet_enabled can be enabled concurrently, so I think it is still
possible that crypt_dec_pending will try-lock an unitialized tasklet if
use_tasklet_enabled is enabled when invoking crypt_dec_pending().
>  
>  	/*
>  	 * 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;
>  

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Ignat Korchagin]

On Wed, Mar 8, 2023 at 2:56 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> > On Mon, Mar 06 2023 at  9:12P -0500,
> > Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> >>>
> >>>> From: Hou Tao <houtao1@huawei.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).
> The circumstance is just a normal qemu VM running the vanilla kernel for test
> purpose. According to the implementation of bio_alloc_bioset(), the front pad of
> bio is not initialized and only bio itself is initialized. AFAIK if
> CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.
> >
> > 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
> Thanks for the comments. It is fine to me.
> >
> > From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> > +     }
> Because use_tasklet_enabled can be enabled concurrently, so I think it is still
> possible that crypt_dec_pending will try-lock an unitialized tasklet if
> use_tasklet_enabled is enabled when invoking crypt_dec_pending().

Perhaps instead we can just pass an additional flag from
tasklet_schedule to indicate to the function that we're running in a
tasklet. I originally have chosen the tasklet_trylock/unlock hack to
avoid passing an extra flag. But unitialized memory makes sense as
well as the desire to avoid calling tasklet_init unconditionally. So
an extra member in dm_crypt_io might be the most straightforward here.

Ignat

> >       /*
> >        * 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;
> >
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Ignat Korchagin]

On Wed, Mar 8, 2023 at 2:56 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi,
>
> On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> > On Mon, Mar 06 2023 at  9:12P -0500,
> > Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> >>>
> >>>> From: Hou Tao <houtao1@huawei.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).
> The circumstance is just a normal qemu VM running the vanilla kernel for test
> purpose. According to the implementation of bio_alloc_bioset(), the front pad of
> bio is not initialized and only bio itself is initialized. AFAIK if
> CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.
> >
> > 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
> Thanks for the comments. It is fine to me.
> >
> > From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> > +     }
> Because use_tasklet_enabled can be enabled concurrently, so I think it is still
> possible that crypt_dec_pending will try-lock an unitialized tasklet if
> use_tasklet_enabled is enabled when invoking crypt_dec_pending().

Perhaps instead we can just pass an additional flag from
tasklet_schedule to indicate to the function that we're running in a
tasklet. I originally have chosen the tasklet_trylock/unlock hack to
avoid passing an extra flag. But unitialized memory makes sense as
well as the desire to avoid calling tasklet_init unconditionally. So
an extra member in dm_crypt_io might be the most straightforward here.

Ignat

> >       /*
> >        * 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;
> >
>

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Wed, Mar 08 2023 at  8:55P -0500,
Ignat Korchagin <ignat@cloudflare.com> wrote:

> On Wed, Mar 8, 2023 at 2:56 AM Hou Tao <houtao@huaweicloud.com> wrote:
> >
> > Hi,
> >
> > On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> > > On Mon, Mar 06 2023 at  9:12P -0500,
> > > Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> > >>>
> > >>>> From: Hou Tao <houtao1@huawei.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).
> > The circumstance is just a normal qemu VM running the vanilla kernel for test
> > purpose. According to the implementation of bio_alloc_bioset(), the front pad of
> > bio is not initialized and only bio itself is initialized. AFAIK if
> > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.

OK.

> > > From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> > > +     }
> > Because use_tasklet_enabled can be enabled concurrently, so I think it is still
> > possible that crypt_dec_pending will try-lock an unitialized tasklet if
> > use_tasklet_enabled is enabled when invoking crypt_dec_pending().

Good point, while I think it is probably acceptable given the worst
case is punting the bio_endio to a workqueue for a time ...

> Perhaps instead we can just pass an additional flag from
> tasklet_schedule to indicate to the function that we're running in a
> tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> avoid passing an extra flag. But unitialized memory makes sense as
> well as the desire to avoid calling tasklet_init unconditionally. So
> an extra member in dm_crypt_io might be the most straightforward here.

... I think we should certainly evaluate the use of an extra flag.

Ignat: I'll have a look at implementing it but if you have a patch
already developed please do share.

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Wed, Mar 08 2023 at  8:55P -0500,
Ignat Korchagin <ignat@cloudflare.com> wrote:

> On Wed, Mar 8, 2023 at 2:56 AM Hou Tao <houtao@huaweicloud.com> wrote:
> >
> > Hi,
> >
> > On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> > > On Mon, Mar 06 2023 at  9:12P -0500,
> > > Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> > >>>
> > >>>> From: Hou Tao <houtao1@huawei.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).
> > The circumstance is just a normal qemu VM running the vanilla kernel for test
> > purpose. According to the implementation of bio_alloc_bioset(), the front pad of
> > bio is not initialized and only bio itself is initialized. AFAIK if
> > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.

OK.

> > > From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> > > +     }
> > Because use_tasklet_enabled can be enabled concurrently, so I think it is still
> > possible that crypt_dec_pending will try-lock an unitialized tasklet if
> > use_tasklet_enabled is enabled when invoking crypt_dec_pending().

Good point, while I think it is probably acceptable given the worst
case is punting the bio_endio to a workqueue for a time ...

> Perhaps instead we can just pass an additional flag from
> tasklet_schedule to indicate to the function that we're running in a
> tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> avoid passing an extra flag. But unitialized memory makes sense as
> well as the desire to avoid calling tasklet_init unconditionally. So
> an extra member in dm_crypt_io might be the most straightforward here.

... I think we should certainly evaluate the use of an extra flag.

Ignat: I'll have a look at implementing it but if you have a patch
already developed please do share.

Thanks,
Mike

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Wed, Mar 08 2023 at  2:19P -0500,
Mike Snitzer <snitzer@kernel.org> wrote:

> On Wed, Mar 08 2023 at  8:55P -0500,
> Ignat Korchagin <ignat@cloudflare.com> wrote:
> 
> > Perhaps instead we can just pass an additional flag from
> > tasklet_schedule to indicate to the function that we're running in a
> > tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> > avoid passing an extra flag. But unitialized memory makes sense as
> > well as the desire to avoid calling tasklet_init unconditionally. So
> > an extra member in dm_crypt_io might be the most straightforward here.
> 
> ... I think we should certainly evaluate the use of an extra flag.
> 
> Ignat: I'll have a look at implementing it but if you have a patch
> already developed please do share.

I've staged the following in linux-next for 6.3 via the linux-dm.git,
but if you see anything wrong with it I can obviously fix:

From: Mike Snitzer <snitzer@kernel.org>
Date: Wed, 8 Mar 2023 14:39:54 -0500
Subject: [PATCH] dm crypt: avoid accessing uninitialized tasklet

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 unnecessarily do io completion
in io_queue workqueue instead of current context.

Fix this by adding an 'in_tasklet' flag to dm_crypt_io struct and
initialize it to false in crypt_io_init(). Set this flag to true in
kcryptd_queue_crypt() before calling tasklet_schedule(). If set
crypt_dec_pending() will punt io completion to a workqueue.

This also nicely avoids the tasklet_trylock/unlock hack when tasklets
aren't in use.

Fixes: 8e14f610159d ("dm crypt: do not call bio_endio() from the dm-crypt tasklet")
Cc: stable@vger.kernel.org
Reported-by: Hou Tao <houtao1@huawei.com>
Suggested-by: Ignat Korchagin <ignat@cloudflare.com>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-crypt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index faba1be572f9..de08ff4f7c98 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -72,7 +72,9 @@ struct dm_crypt_io {
 	struct crypt_config *cc;
 	struct bio *base_bio;
 	u8 *integrity_metadata;
-	bool integrity_metadata_from_pool;
+	bool integrity_metadata_from_pool:1;
+	bool in_tasklet:1;
+
 	struct work_struct work;
 	struct tasklet_struct tasklet;
 
@@ -1731,6 +1733,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
 	io->ctx.r.req = NULL;
 	io->integrity_metadata = NULL;
 	io->integrity_metadata_from_pool = false;
+	io->in_tasklet = false;
 	atomic_set(&io->io_pending, 0);
 }
 
@@ -1777,8 +1780,7 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
 	 * our tasklet. In this case we need to delay bio_endio()
 	 * execution to after the tasklet is done and dequeued.
 	 */
-	if (tasklet_trylock(&io->tasklet)) {
-		tasklet_unlock(&io->tasklet);
+	if (!io->in_tasklet) {
 		bio_endio(base_bio);
 		return;
 	}
@@ -2233,6 +2235,7 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 		 * it is being executed with irqs disabled.
 		 */
 		if (in_hardirq() || irqs_disabled()) {
+			io->in_tasklet = true;
 			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
 			tasklet_schedule(&io->tasklet);
 			return;
-- 
2.37.1 (Apple Git-137.1)

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Mike Snitzer]

On Wed, Mar 08 2023 at  2:19P -0500,
Mike Snitzer <snitzer@kernel.org> wrote:

> On Wed, Mar 08 2023 at  8:55P -0500,
> Ignat Korchagin <ignat@cloudflare.com> wrote:
> 
> > Perhaps instead we can just pass an additional flag from
> > tasklet_schedule to indicate to the function that we're running in a
> > tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> > avoid passing an extra flag. But unitialized memory makes sense as
> > well as the desire to avoid calling tasklet_init unconditionally. So
> > an extra member in dm_crypt_io might be the most straightforward here.
> 
> ... I think we should certainly evaluate the use of an extra flag.
> 
> Ignat: I'll have a look at implementing it but if you have a patch
> already developed please do share.

I've staged the following in linux-next for 6.3 via the linux-dm.git,
but if you see anything wrong with it I can obviously fix:

From: Mike Snitzer <snitzer@kernel.org>
Date: Wed, 8 Mar 2023 14:39:54 -0500
Subject: [PATCH] dm crypt: avoid accessing uninitialized tasklet

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 unnecessarily do io completion
in io_queue workqueue instead of current context.

Fix this by adding an 'in_tasklet' flag to dm_crypt_io struct and
initialize it to false in crypt_io_init(). Set this flag to true in
kcryptd_queue_crypt() before calling tasklet_schedule(). If set
crypt_dec_pending() will punt io completion to a workqueue.

This also nicely avoids the tasklet_trylock/unlock hack when tasklets
aren't in use.

Fixes: 8e14f610159d ("dm crypt: do not call bio_endio() from the dm-crypt tasklet")
Cc: stable@vger.kernel.org
Reported-by: Hou Tao <houtao1@huawei.com>
Suggested-by: Ignat Korchagin <ignat@cloudflare.com>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-crypt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index faba1be572f9..de08ff4f7c98 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -72,7 +72,9 @@ struct dm_crypt_io {
 	struct crypt_config *cc;
 	struct bio *base_bio;
 	u8 *integrity_metadata;
-	bool integrity_metadata_from_pool;
+	bool integrity_metadata_from_pool:1;
+	bool in_tasklet:1;
+
 	struct work_struct work;
 	struct tasklet_struct tasklet;
 
@@ -1731,6 +1733,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
 	io->ctx.r.req = NULL;
 	io->integrity_metadata = NULL;
 	io->integrity_metadata_from_pool = false;
+	io->in_tasklet = false;
 	atomic_set(&io->io_pending, 0);
 }
 
@@ -1777,8 +1780,7 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
 	 * our tasklet. In this case we need to delay bio_endio()
 	 * execution to after the tasklet is done and dequeued.
 	 */
-	if (tasklet_trylock(&io->tasklet)) {
-		tasklet_unlock(&io->tasklet);
+	if (!io->in_tasklet) {
 		bio_endio(base_bio);
 		return;
 	}
@@ -2233,6 +2235,7 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 		 * it is being executed with irqs disabled.
 		 */
 		if (in_hardirq() || irqs_disabled()) {
+			io->in_tasklet = true;
 			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
 			tasklet_schedule(&io->tasklet);
 			return;
-- 
2.37.1 (Apple Git-137.1)

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Ignat Korchagin]

On Wed, Mar 8, 2023 at 8:27 PM Mike Snitzer <snitzer@kernel.org> wrote:
>
> On Wed, Mar 08 2023 at  2:19P -0500,
> Mike Snitzer <snitzer@kernel.org> wrote:
>
> > On Wed, Mar 08 2023 at  8:55P -0500,
> > Ignat Korchagin <ignat@cloudflare.com> wrote:
> >
> > > Perhaps instead we can just pass an additional flag from
> > > tasklet_schedule to indicate to the function that we're running in a
> > > tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> > > avoid passing an extra flag. But unitialized memory makes sense as
> > > well as the desire to avoid calling tasklet_init unconditionally. So
> > > an extra member in dm_crypt_io might be the most straightforward here.
> >
> > ... I think we should certainly evaluate the use of an extra flag.
> >
> > Ignat: I'll have a look at implementing it but if you have a patch
> > already developed please do share.
>
> I've staged the following in linux-next for 6.3 via the linux-dm.git,
> but if you see anything wrong with it I can obviously fix:
>
> From: Mike Snitzer <snitzer@kernel.org>
> Date: Wed, 8 Mar 2023 14:39:54 -0500
> Subject: [PATCH] dm crypt: avoid accessing uninitialized tasklet
>
> 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 unnecessarily do io completion
> in io_queue workqueue instead of current context.
>
> Fix this by adding an 'in_tasklet' flag to dm_crypt_io struct and
> initialize it to false in crypt_io_init(). Set this flag to true in
> kcryptd_queue_crypt() before calling tasklet_schedule(). If set
> crypt_dec_pending() will punt io completion to a workqueue.
>
> This also nicely avoids the tasklet_trylock/unlock hack when tasklets
> aren't in use.
>
> Fixes: 8e14f610159d ("dm crypt: do not call bio_endio() from the dm-crypt tasklet")
> Cc: stable@vger.kernel.org
> Reported-by: Hou Tao <houtao1@huawei.com>
> Suggested-by: Ignat Korchagin <ignat@cloudflare.com>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  drivers/md/dm-crypt.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index faba1be572f9..de08ff4f7c98 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -72,7 +72,9 @@ struct dm_crypt_io {
>         struct crypt_config *cc;
>         struct bio *base_bio;
>         u8 *integrity_metadata;
> -       bool integrity_metadata_from_pool;
> +       bool integrity_metadata_from_pool:1;
> +       bool in_tasklet:1;
> +
>         struct work_struct work;
>         struct tasklet_struct tasklet;
>
> @@ -1731,6 +1733,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
>         io->ctx.r.req = NULL;
>         io->integrity_metadata = NULL;
>         io->integrity_metadata_from_pool = false;
> +       io->in_tasklet = false;
>         atomic_set(&io->io_pending, 0);
>  }
>
> @@ -1777,8 +1780,7 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>          * our tasklet. In this case we need to delay bio_endio()
>          * execution to after the tasklet is done and dequeued.
>          */
> -       if (tasklet_trylock(&io->tasklet)) {
> -               tasklet_unlock(&io->tasklet);
> +       if (!io->in_tasklet) {

nitpick: maybe invert the logic here for better readability? (so it
becomes "if (in_tasklet) queue..." else just falls through bio_endio()
)

>                 bio_endio(base_bio);
>                 return;
>         }
> @@ -2233,6 +2235,7 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>                  * it is being executed with irqs disabled.
>                  */
>                 if (in_hardirq() || irqs_disabled()) {
> +                       io->in_tasklet = true;
>                         tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
>                         tasklet_schedule(&io->tasklet);
>                         return;
> --
> 2.37.1 (Apple Git-137.1)
>

Reviewed-by: Ignat Korchagin <ignat@cloudflare.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Ignat Korchagin]

On Wed, Mar 8, 2023 at 8:27 PM Mike Snitzer <snitzer@kernel.org> wrote:
>
> On Wed, Mar 08 2023 at  2:19P -0500,
> Mike Snitzer <snitzer@kernel.org> wrote:
>
> > On Wed, Mar 08 2023 at  8:55P -0500,
> > Ignat Korchagin <ignat@cloudflare.com> wrote:
> >
> > > Perhaps instead we can just pass an additional flag from
> > > tasklet_schedule to indicate to the function that we're running in a
> > > tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> > > avoid passing an extra flag. But unitialized memory makes sense as
> > > well as the desire to avoid calling tasklet_init unconditionally. So
> > > an extra member in dm_crypt_io might be the most straightforward here.
> >
> > ... I think we should certainly evaluate the use of an extra flag.
> >
> > Ignat: I'll have a look at implementing it but if you have a patch
> > already developed please do share.
>
> I've staged the following in linux-next for 6.3 via the linux-dm.git,
> but if you see anything wrong with it I can obviously fix:
>
> From: Mike Snitzer <snitzer@kernel.org>
> Date: Wed, 8 Mar 2023 14:39:54 -0500
> Subject: [PATCH] dm crypt: avoid accessing uninitialized tasklet
>
> 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 unnecessarily do io completion
> in io_queue workqueue instead of current context.
>
> Fix this by adding an 'in_tasklet' flag to dm_crypt_io struct and
> initialize it to false in crypt_io_init(). Set this flag to true in
> kcryptd_queue_crypt() before calling tasklet_schedule(). If set
> crypt_dec_pending() will punt io completion to a workqueue.
>
> This also nicely avoids the tasklet_trylock/unlock hack when tasklets
> aren't in use.
>
> Fixes: 8e14f610159d ("dm crypt: do not call bio_endio() from the dm-crypt tasklet")
> Cc: stable@vger.kernel.org
> Reported-by: Hou Tao <houtao1@huawei.com>
> Suggested-by: Ignat Korchagin <ignat@cloudflare.com>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  drivers/md/dm-crypt.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index faba1be572f9..de08ff4f7c98 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -72,7 +72,9 @@ struct dm_crypt_io {
>         struct crypt_config *cc;
>         struct bio *base_bio;
>         u8 *integrity_metadata;
> -       bool integrity_metadata_from_pool;
> +       bool integrity_metadata_from_pool:1;
> +       bool in_tasklet:1;
> +
>         struct work_struct work;
>         struct tasklet_struct tasklet;
>
> @@ -1731,6 +1733,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
>         io->ctx.r.req = NULL;
>         io->integrity_metadata = NULL;
>         io->integrity_metadata_from_pool = false;
> +       io->in_tasklet = false;
>         atomic_set(&io->io_pending, 0);
>  }
>
> @@ -1777,8 +1780,7 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>          * our tasklet. In this case we need to delay bio_endio()
>          * execution to after the tasklet is done and dequeued.
>          */
> -       if (tasklet_trylock(&io->tasklet)) {
> -               tasklet_unlock(&io->tasklet);
> +       if (!io->in_tasklet) {

nitpick: maybe invert the logic here for better readability? (so it
becomes "if (in_tasklet) queue..." else just falls through bio_endio()
)

>                 bio_endio(base_bio);
>                 return;
>         }
> @@ -2233,6 +2235,7 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>                  * it is being executed with irqs disabled.
>                  */
>                 if (in_hardirq() || irqs_disabled()) {
> +                       io->in_tasklet = true;
>                         tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
>                         tasklet_schedule(&io->tasklet);
>                         return;
> --
> 2.37.1 (Apple Git-137.1)
>

Reviewed-by: Ignat Korchagin <ignat@cloudflare.com>

Re: [dm-devel] dm crypt: initialize tasklet in crypt_io_init()

[Ignat Korchagin]

On Wed, Mar 8, 2023 at 7:19 PM Mike Snitzer <snitzer@kernel.org> wrote:
>
> On Wed, Mar 08 2023 at  8:55P -0500,
> Ignat Korchagin <ignat@cloudflare.com> wrote:
>
> > On Wed, Mar 8, 2023 at 2:56 AM Hou Tao <houtao@huaweicloud.com> wrote:
> > >
> > > Hi,
> > >
> > > On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> > > > On Mon, Mar 06 2023 at  9:12P -0500,
> > > > Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> > > >>>
> > > >>>> From: Hou Tao <houtao1@huawei.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).
> > > The circumstance is just a normal qemu VM running the vanilla kernel for test
> > > purpose. According to the implementation of bio_alloc_bioset(), the front pad of
> > > bio is not initialized and only bio itself is initialized. AFAIK if
> > > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.
>
> OK.
>
> > > > From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> > > > +     }
> > > Because use_tasklet_enabled can be enabled concurrently, so I think it is still
> > > possible that crypt_dec_pending will try-lock an unitialized tasklet if
> > > use_tasklet_enabled is enabled when invoking crypt_dec_pending().
>
> Good point, while I think it is probably acceptable given the worst
> case is punting the bio_endio to a workqueue for a time ...
>
> > Perhaps instead we can just pass an additional flag from
> > tasklet_schedule to indicate to the function that we're running in a
> > tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> > avoid passing an extra flag. But unitialized memory makes sense as
> > well as the desire to avoid calling tasklet_init unconditionally. So
> > an extra member in dm_crypt_io might be the most straightforward here.
>
> ... I think we should certainly evaluate the use of an extra flag.
>
> Ignat: I'll have a look at implementing it but if you have a patch
> already developed please do share.

I don't have but it seems your latest patch is exactly what I had in mind.

> Thanks,
> Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: dm crypt: initialize tasklet in crypt_io_init()

[Ignat Korchagin]

On Wed, Mar 8, 2023 at 7:19 PM Mike Snitzer <snitzer@kernel.org> wrote:
>
> On Wed, Mar 08 2023 at  8:55P -0500,
> Ignat Korchagin <ignat@cloudflare.com> wrote:
>
> > On Wed, Mar 8, 2023 at 2:56 AM Hou Tao <houtao@huaweicloud.com> wrote:
> > >
> > > Hi,
> > >
> > > On 3/7/2023 10:47 PM, Mike Snitzer wrote:
> > > > On Mon, Mar 06 2023 at  9:12P -0500,
> > > > Hou Tao <houtao@huaweicloud.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@huaweicloud.com> wrote:
> > > >>>
> > > >>>> From: Hou Tao <houtao1@huawei.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).
> > > The circumstance is just a normal qemu VM running the vanilla kernel for test
> > > purpose. According to the implementation of bio_alloc_bioset(), the front pad of
> > > bio is not initialized and only bio itself is initialized. AFAIK if
> > > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed.
>
> OK.
>
> > > > From: Mike Snitzer <snitzer@kernel.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@kernel.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;
> > > > +     }
> > > Because use_tasklet_enabled can be enabled concurrently, so I think it is still
> > > possible that crypt_dec_pending will try-lock an unitialized tasklet if
> > > use_tasklet_enabled is enabled when invoking crypt_dec_pending().
>
> Good point, while I think it is probably acceptable given the worst
> case is punting the bio_endio to a workqueue for a time ...
>
> > Perhaps instead we can just pass an additional flag from
> > tasklet_schedule to indicate to the function that we're running in a
> > tasklet. I originally have chosen the tasklet_trylock/unlock hack to
> > avoid passing an extra flag. But unitialized memory makes sense as
> > well as the desire to avoid calling tasklet_init unconditionally. So
> > an extra member in dm_crypt_io might be the most straightforward here.
>
> ... I think we should certainly evaluate the use of an extra flag.
>
> Ignat: I'll have a look at implementing it but if you have a patch
> already developed please do share.

I don't have but it seems your latest patch is exactly what I had in mind.

> Thanks,
> Mike