| Message ID | 20240411091539.361470-1-lilingfeng3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [6.6] Revert "dm-crypt, dm-verity: disable tasklets" | expand |
On Thu, Apr 11, 2024 at 05:15:39PM +0800, 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/ > > 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(-) What is the commit id of this in Linus's tree? And why only 6.6.y? thanks, greg k-h
There is no need to revert it in Linus's tree since the expanded workqueue can work like tasklet. https://lore.kernel.org/lkml/20240130091300.2968534-1-tj@kernel.org/ I'm not sure if the problem described in [1] could be triggered by a scenario I haven't considered. So I only send the revert patch to 6.6 mainly for comments. Of course, I think the other branches need it too. Thanks 在 2024/4/11 17:52, Greg KH 写道: > On Thu, Apr 11, 2024 at 05:15:39PM +0800, 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/ >> >> 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(-) > What is the commit id of this in Linus's tree? And why only 6.6.y? > > thanks, > > greg k-h >
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 >
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 >>
On Fri, 12 Apr 2024, Li Lingfeng wrote: > 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 Yes, you are right that scheduling is disabled when ksoftirqd processes a softirq task. But the upstream kernel switched to bh workqueues anyway, so there is no need to submit a different solution to the stable kernels. Mikulas
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;
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/ 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(-)