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

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@suse.de/T/ [2] https://lore.kernel.org/all/4d331659-badd-749d-fba1-271543631a8a@huawei.com/

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@huawei.com

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