From: Li Lingfeng <lilingfeng3@huawei.com>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: <stable@vger.kernel.org>, <gregkh@linuxfoundation.org>,
<torvalds@linux-foundation.org>, <tglx@linutronix.de>,
<linux-kernel@vger.kernel.org>, <dm-devel@lists.linux.dev>,
<msnitzer@redhat.com>, <ignat@cloudflare.com>,
<damien.lemoal@wdc.com>, <houtao1@huawei.com>, <nhuck@google.com>,
<peterz@infradead.org>, <yukuai3@huawei.com>,
<yangerkun@huawei.com>, <yi.zhang@huawei.com>,
<lilingfeng@huaweicloud.com>
Subject: Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"
Date: Fri, 12 Apr 2024 10:06:14 +0800 [thread overview]
Message-ID: <4e5e4634-a2d0-ce35-3884-829d385c0879@huawei.com> (raw)
In-Reply-To: <7c17f31a-2cc3-1597-e2b5-832355de7647@redhat.com>
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
>>
next prev parent reply other threads:[~2024-04-12 2:06 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 9:15 [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets" Li Lingfeng
2024-04-11 9:52 ` Greg KH
2024-04-11 12:17 ` Li Lingfeng
2024-04-11 12:37 ` Mikulas Patocka
2024-04-12 2:06 ` Li Lingfeng [this message]
2024-04-15 15:04 ` Mikulas Patocka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4e5e4634-a2d0-ce35-3884-829d385c0879@huawei.com \
--to=lilingfeng3@huawei.com \
--cc=damien.lemoal@wdc.com \
--cc=dm-devel@lists.linux.dev \
--cc=gregkh@linuxfoundation.org \
--cc=houtao1@huawei.com \
--cc=ignat@cloudflare.com \
--cc=lilingfeng@huaweicloud.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mpatocka@redhat.com \
--cc=msnitzer@redhat.com \
--cc=nhuck@google.com \
--cc=peterz@infradead.org \
--cc=stable@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=yangerkun@huawei.com \
--cc=yi.zhang@huawei.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).