linux-kernel - Re: [PATCH -next v10 3/4] block, bfq: refactor the counting of 'num_groups_with_pending

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <97534773-484f-5c2c-a371-446cc0680b73@huaweicloud.com>
Date:   Wed, 14 Sep 2022 16:15:26 +0800
From:   Yu Kuai <yukuai1@...weicloud.com>
To:     Paolo VALENTE <paolo.valente@...more.it>,
        Yu Kuai <yukuai1@...weicloud.com>
Cc:     Jan Kara <jack@...e.cz>, cgroups@...r.kernel.org,
        linux-block <linux-block@...r.kernel.org>,
        Tejun Heo <tj@...nel.org>, Jens Axboe <axboe@...nel.dk>,
        LKML <linux-kernel@...r.kernel.org>, yi.zhang@...wei.com,
        "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next v10 3/4] block, bfq: refactor the counting of
 'num_groups_with_pending_reqs'

Hi, Paolo

在 2022/09/14 15:50, Paolo VALENTE 写道:
> 
> 
>> Il giorno 14 set 2022, alle ore 03:55, Yu Kuai <yukuai1@...weicloud.com> ha scritto:
>>
>>
>>
>> 在 2022/09/07 9:16, Yu Kuai 写道:
>>> Hi, Paolo!
>>> 在 2022/09/06 17:37, Paolo Valente 写道:
>>>>
>>>>
>>>>> Il giorno 26 ago 2022, alle ore 04:34, Yu Kuai <yukuai1@...weicloud.com> ha scritto:
>>>>>
>>>>> Hi, Paolo!
>>>>>
>>>>> 在 2022/08/25 22:59, Paolo Valente 写道:
>>>>>>> Il giorno 11 ago 2022, alle ore 03:19, Yu Kuai <yukuai1@...weicloud.com <mailto:yukuai1@...weicloud.com>> ha scritto:
>>>>>>>
>>>>>>> Hi, Paolo
>>>>>>>
>>>>>>> 在 2022/08/10 18:49, Paolo Valente 写道:
>>>>>>>>> Il giorno 27 lug 2022, alle ore 14:11, Yu Kuai <yukuai1@...weicloud.com <mailto:yukuai1@...weicloud.com>> ha scritto:
>>>>>>>>>
>>>>>>>>> Hi, Paolo
>>>>>>>>>
>>>>>>>> hi
>>>>>>>>> Are you still interested in this patchset?
>>>>>>>>>
>>>>>>>> Yes. Sorry for replying very late again.
>>>>>>>> Probably the last fix that you suggest is enough, but I'm a little bit
>>>>>>>> concerned that it may be a little hasty.  In fact, before this fix, we
>>>>>>>> exchanged several messages, and I didn't seem to be very good at
>>>>>>>> convincing you about the need to keep into account also in-service
>>>>>>>> I/O.  So, my question is: are you sure that now you have a
>>>>>>>
>>>>>>> I'm confused here, I'm pretty aware that in-service I/O(as said pending
>>>>>>> requests is the patchset) should be counted, as you suggested in v7, are
>>>>>>> you still thinking that the way in this patchset is problematic?
>>>>>>>
>>>>>>> I'll try to explain again that how to track is bfqq has pending pending
>>>>>>> requests, please let me know if you still think there are some problems:
>>>>>>>
>>>>>>> patch 1 support to track if bfqq has pending requests, it's
>>>>>>> done by setting the flag 'entity->in_groups_with_pending_reqs' when the
>>>>>>> first request is inserted to bfqq, and it's cleared when the last
>>>>>>> request is completed. specifically the flag is set in
>>>>>>> bfq_add_bfqq_busy() when 'bfqq->dispatched' if false, and it's cleared
>>>>>>> both in bfq_completed_request() and bfq_del_bfqq_busy() when
>>>>>>> 'bfqq->diapatched' is false.
>>>>>>>
>>>>>> This general description seems correct to me. Have you already sent a new version of your patchset?
>>>>>
>>>>> It's glad that we finially on the same page here.
>>>>>
>>>>
>>>> Yep. Sorry for my chronicle delay.
>>> Better late than never 😁
>>>>
>>>>> Please take a look at patch 1, which already impelement the above
>>>>> descriptions, it seems to me there is no need to send a new version
>>>>> for now. If you think there are still some other problems, please let
>>>>> me know.
>>>>>
>>>>
>>>> Patch 1 seems ok to me. I seem to have only one pending comment on this patch (3/4) instead. Let me paste previous stuff here for your convenience:
>>> That sounds good.
>>>>
>>>>>>
>>>>>> -    /*
>>>>>> -     * Next function is invoked last, because it causes bfqq to be
>>>>>> -     * freed if the following holds: bfqq is not in service and
>>>>>> -     * has no dispatched request. DO NOT use bfqq after the next
>>>>>> -     * function invocation.
>>>>>> -     */
>>>>> I would really love it if you leave this comment.  I added it after
>>>>> suffering a lot for a nasty UAF.  Of course the first sentence may
>>>>> need to be adjusted if the code that precedes it is to be removed.
>>>>> Same as above, if this patch is applied, this function will be gone.
>>
>> Hi, I'm curious while I'm trying to add the comment, before this
>> patchset, can bfqq be freed when bfq_weights_tree_remove is called?
>>
>> bfq_completed_request
>> bfqq->dispatched--
>> if (!bfqq->dispatched && !bfq_bfqq_busy(bfqq))
>>   bfq_weights_tree_remove(bfqd, bfqq);
>>
>> // continue to use bfqq
>>
>> It seems to me this is problematic if so, because bfqq is used after
>> bfq_weights_tree_remove() is called.
>>
> 
> It is.  Yet, IIRC, I verified that bfqq was not used after that free,
> and I added that comment as a heads-up.  What is a scenario (before
> your pending modifications) where this use-after-free happens?
> 

No, it never happens, I just notice it because it'll be weird if I
place the comment where bfq_weights_tree_remove() is called, since bfqq
will still be accessed.

If the suituation that the comment says is possible, perhaps we should
move bfq_weights_tree_remove() to the last of bfq_completed_request().
However, it seems that we haven't meet the problem for quite a long
time...

Thanks,
Kuai

> Thanks,
> Paolo
> 
>> Thanks,
>> Kuai
> 
> .
>