Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

I'm having difficulty understanding "Workqueues and ksoftirqd may be scheduled arbitrarily".
This is my understanding:
kcryptd_queue_crypt
 tasklet_schedule
  __tasklet_schedule
   __tasklet_schedule_common
    raise_softirq_irqoff
     wakeup_softirqd
      wake_up_process // ksoftirqd

run_ksoftirqd
 __do_softirq
  softirq_handle_begin
   __local_bh_disable_ip // Turn off preemption
<---------- [1] ---------->
  tasklet_action // h->action
   tasklet_action_common
    tasklet_trylock
     kcryptd_crypt_tasklet // t->func(t->data)
     ...
      queue_work(cc->io_queue, &io->work)
<---------- [2] ---------->
    tasklet_unlock

// workqueue process
kcryptd_io_bio_endio
 ...
 // free tasklet_struct

Since preemption has been turned off at [1], I'm confused about how the CPU can be scheduled out to do work first at [2].
Would you mind explaining it to me?

Thanks

在 2024/4/11 20:37, Mikulas Patocka 写道:
Hi

I NACK this.


On Thu, 11 Apr 2024, Li Lingfeng wrote:

This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.

Tasklet is thought to cause memory corruption [1], so it was disabled in
dm-crypt and dm-verity. However, memory corruption may not happen since
cc->io_queue is created without WQ_UNBOUND [2].
Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
bring tasklet back.

[1] https://lore.kernel.org/all/d390d7ee-f142-44d3-822a-87949e14608b@xxxxxxx/T/
[2] https://lore.kernel.org/all/4d331659-badd-749d-fba1-271543631a8a@xxxxxxxxxx/
Regarding [2] - if you add mdelay, you can make the race condition less
reproducible, but it is not a proper fix and the race condition stays
there unfixed.

Workqueues and ksoftirqd may be scheduled arbitrarily, there is no
guarantee that they will be executed in a particular order, even if they
are executed on the same CPU.

Mikulas

Signed-off-by: Li Lingfeng <lilingfeng3@xxxxxxxxxx>
---
  drivers/md/dm-crypt.c         | 38 +++++++++++++++++++++++++++++++++--
  drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
  drivers/md/dm-verity.h        |  1 +
  3 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index aa6bb5b4704b..a60d91d02e28 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -75,8 +75,10 @@ struct dm_crypt_io {
  	struct bio *base_bio;
  	u8 *integrity_metadata;
  	bool integrity_metadata_from_pool:1;
+	bool in_tasklet:1;
struct work_struct work;
+	struct tasklet_struct tasklet;
struct convert_context ctx; @@ -1775,6 +1777,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);
  }
@@ -1785,6 +1788,13 @@ static void crypt_inc_pending(struct dm_crypt_io *io) static void kcryptd_queue_read(struct dm_crypt_io *io); +static void kcryptd_io_bio_endio(struct work_struct *work)
+{
+	struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
+
+	bio_endio(io->base_bio);
+}
+
  /*
   * One of the bios was finished. Check for completion of
   * the whole request and correctly clean up the buffer.
@@ -1817,6 +1827,20 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
base_bio->bi_status = error; + /*
+	 * If we are running this function from our tasklet,
+	 * we can't call bio_endio() here, because it will call
+	 * clone_endio() from dm.c, which in turn will
+	 * free the current struct dm_crypt_io structure with
+	 * our tasklet. In this case we need to delay bio_endio()
+	 * execution to after the tasklet is done and dequeued.
+	 */
+	if (io->in_tasklet) {
+		INIT_WORK(&io->work, kcryptd_io_bio_endio);
+		queue_work(cc->io_queue, &io->work);
+		return;
+	}
+
  	bio_endio(base_bio);
  }
@@ -2291,6 +2315,11 @@ static void kcryptd_crypt(struct work_struct *work)
  		kcryptd_crypt_write_convert(io);
  }
+static void kcryptd_crypt_tasklet(unsigned long work)
+{
+	kcryptd_crypt((struct work_struct *)work);
+}
+
  static void kcryptd_queue_crypt(struct dm_crypt_io *io)
  {
  	struct crypt_config *cc = io->cc;
@@ -2302,10 +2331,15 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
  		 * irqs_disabled(): the kernel may run some IO completion from the idle thread, but
  		 * it is being executed with irqs disabled.
  		 */
-		if (!(in_hardirq() || irqs_disabled())) {
-			kcryptd_crypt(&io->work);
+		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;
  		}
+
+		kcryptd_crypt(&io->work);
+		return;
  	}
INIT_WORK(&io->work, kcryptd_crypt);
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 49e4a35d7019..0bb126eadc0d 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -701,6 +701,23 @@ static void verity_work(struct work_struct *w)
  	verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
  }
+static void verity_tasklet(unsigned long data)
+{
+	struct dm_verity_io *io = (struct dm_verity_io *)data;
+	int err;
+
+	io->in_tasklet = true;
+	err = verity_verify_io(io);
+	if (err == -EAGAIN || err == -ENOMEM) {
+		/* fallback to retrying with work-queue */
+		INIT_WORK(&io->work, verity_work);
+		queue_work(io->v->verify_wq, &io->work);
+		return;
+	}
+
+	verity_finish_io(io, errno_to_blk_status(err));
+}
+
  static void verity_end_io(struct bio *bio)
  {
  	struct dm_verity_io *io = bio->bi_private;
@@ -713,8 +730,13 @@ static void verity_end_io(struct bio *bio)
  		return;
  	}
- INIT_WORK(&io->work, verity_work);
-	queue_work(io->v->verify_wq, &io->work);
+	if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
+		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
+		tasklet_schedule(&io->tasklet);
+	} else {
+		INIT_WORK(&io->work, verity_work);
+		queue_work(io->v->verify_wq, &io->work);
+	}
  }
/*
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index db93a91169d5..7e495cc375b0 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -87,6 +87,7 @@ struct dm_verity_io {
  	bool in_tasklet;
struct work_struct work;
+	struct tasklet_struct tasklet;
char *recheck_buffer; --
2.31.1





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux