From: Yu Kuai <yukuai1@huaweicloud.com>
To: Jan Kara <jack@suse.cz>, Yu Kuai <yukuai1@huaweicloud.com>
Cc: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
Gabriele Felici <felicigb@gmail.com>,
Gianmarco Lusvardi <glusvardi@posteo.net>,
Giulio Barabino <giuliobarabino99@gmail.com>,
Emiliano Maccaferri <inbox@emilianomaccaferri.com>,
Paolo Valente <paolo.valente@linaro.org>,
Damien Le Moal <damien.lemoal@opensource.wdc.com>,
"yukuai (C)" <yukuai3@huawei.com>
Subject: Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
Date: Tue, 7 Mar 2023 18:28:41 +0800 [thread overview]
Message-ID: <73a84d09-9b64-f23a-1d24-a41cc1187b4b@huaweicloud.com> (raw)
In-Reply-To: <20230307102040.3t6qiojxj72fqrlc@quack3>
Hi, Jan
在 2023/03/07 18:20, Jan Kara 写道:
> On Tue 07-03-23 17:36:19, Yu Kuai wrote:
>> Hi,
>>
>> 在 2023/03/07 17:13, Shinichiro Kawasaki 写道:
>>> On Mar 07, 2023 / 16:57, Yu Kuai wrote:
>>> [...]
>>>> Thanks for the report, can you help to provide the result of add2line of
>>>> following?
>>>>
>>>> bfq_setup_cooperator+0x120b/0x1650
>>>> bfq_setup_cooperator+0xa41/0x1650
>>>>
>>>> That will help to locate the problem.
>>>
>>> Hi, Yu thanks for looking into this. Here are the faddr2line outputs:
>>>
>>> $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0x120b/0x1650
>>> bfq_setup_cooperator+0x120b/0x1650:
>>> bfqq_process_refs at block/bfq-iosched.c:1200
>>> (inlined by) bfq_setup_stable_merge at block/bfq-iosched.c:2855
>>> (inlined by) bfq_setup_cooperator at block/bfq-iosched.c:2941
>>>
>>> $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0xa41/0x1650
>>> bfq_setup_cooperator+0xa41/0x1650:
>>> bfq_setup_cooperator at block/bfq-iosched.c:2939
>>>
>>
>> So, after a quick look at the code, the difference is that fd571df0ac5b
>> changes the logic when bfqq_proces_refs(stable_merge_bfqq) is 0.
>>
>> I think perhaps following patch can work, at least 'stable_merge_bfqq'
>> won't be accessed after bfq_put_stable_ref() in this case:
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 8a8d4441519c..881f74b3a556 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -2856,6 +2856,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct
>> bfq_queue *bfqq,
>> bfqq_process_refs(stable_merge_bfqq));
>> struct bfq_queue *new_bfqq;
>>
>> + /* deschedule stable merge, because done or aborted here */
>> + bfq_put_stable_ref(stable_merge_bfqq);
>> +
>> + bfqq_data->stable_merge_bfqq = NULL;
>> +
>
> I don't think this is going to help. stable_merge_bfqq is used just a few
> lines below again in bfq_setup_merge(). The problem really is that the
> reference from stable merge can be the last one keeping bfqq alive so in
> bfq_setup_cooperator() we need to see if stable_merge_bfqq still has
> process references (and cancel the merge if not) before dropping our ref.
I was thinking that bfq_setup_merge() will only be called if 'proc_ref'
is not 0, which means that stable_merge_bfqq won't be freed.
>
> So rather doing something like:
>
> bfqq_data->stable_merge_bfqq = NULL;
> new_bfqq = bfq_setup_stable_merge(bfqd, bfqq,
> stable_merge_bfqq, bfqq_data);
> bfq_put_stable_ref(stable_merge_bfqq);
> return new_bfqq;
>
> should work in bfq_setup_cooperator().
Yes, this will work.
Thanks,
Kuai
>
> Honza
>
next prev parent reply other threads:[~2023-03-07 10:30 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-07 7:14 [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator Shinichiro Kawasaki
2023-03-07 8:57 ` Yu Kuai
2023-03-07 9:13 ` Shinichiro Kawasaki
2023-03-07 9:36 ` Yu Kuai
2023-03-07 10:20 ` Jan Kara
2023-03-07 10:28 ` Yu Kuai [this message]
2023-03-07 11:49 ` Shinichiro Kawasaki
2023-03-07 14:26 ` Jens Axboe
2023-03-08 2:32 ` [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq' Yu Kuai
2023-03-08 5:56 ` Shinichiro Kawasaki
2023-03-08 10:21 ` Jan Kara
2023-03-08 14:35 ` Jens Axboe
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=73a84d09-9b64-f23a-1d24-a41cc1187b4b@huaweicloud.com \
--to=yukuai1@huaweicloud.com \
--cc=damien.lemoal@opensource.wdc.com \
--cc=felicigb@gmail.com \
--cc=giuliobarabino99@gmail.com \
--cc=glusvardi@posteo.net \
--cc=inbox@emilianomaccaferri.com \
--cc=jack@suse.cz \
--cc=linux-block@vger.kernel.org \
--cc=paolo.valente@linaro.org \
--cc=shinichiro.kawasaki@wdc.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.