Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH -next v10 3/4] block, bfq: refactor the counting of
 'num_groups_with_pending_reqs'
To:     Paolo VALENTE <paolo.valente@unimore.it>,
        Yu Kuai <yukuai1@huaweicloud.com>
Cc:     Jan Kara <jack@suse.cz>, cgroups@vger.kernel.org,
        linux-block <linux-block@vger.kernel.org>,
        Tejun Heo <tj@kernel.org>, Jens Axboe <axboe@kernel.dk>,
        LKML <linux-kernel@vger.kernel.org>, yi.zhang@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20220610021701.2347602-1-yukuai3@huawei.com>
 <20220610021701.2347602-4-yukuai3@huawei.com>
 <27F2DF19-7CC6-42C5-8CEB-43583EB4AE46@linaro.org>
 <abdbb5db-e280-62f8-0670-536fcb8ec4d9@huaweicloud.com>
 <C2CF100A-9A7C-4300-9A70-1295BC939C66@unimore.it>
 <9b2d667f-6636-9347-08a1-8bd0aa2346f2@huaweicloud.com>
 <2f94f241-445f-1beb-c4a8-73f6efce5af2@huaweicloud.com>
 <55A07102-BE55-4606-9E32-64E884064FB9@unimore.it>
 <5cb0e5bc-feec-86d6-6f60-3c28ee625efd@huaweicloud.com>
 <D89DCF20-27D8-4F8F-B8B0-FD193FC4F18D@unimore.it>
 <e6b53794-f93f-92b2-1f45-35ae81a28a5c@huaweicloud.com>
 <F758A356-EE6B-4B7B-95E2-6414616C77E4@unimore.it>
 <5e0b44b4-46cc-b3c6-1d93-00a0a683eda8@huaweicloud.com>
 <f89eb61b-7912-5916-1a12-039e32bebe70@huaweicloud.com>
 <BF3909EA-4659-48CB-917A-639DC3318916@unimore.it>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <97534773-484f-5c2c-a371-446cc0680b73@huaweicloud.com>
Date:   Wed, 14 Sep 2022 16:15:26 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <BF3909EA-4659-48CB-917A-639DC3318916@unimore.it>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Paolo

在 2022/09/14 15:50, Paolo VALENTE 写道:
> 
> 
>> Il giorno 14 set 2022, alle ore 03:55, Yu Kuai <yukuai1@huaweicloud.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@huaweicloud.com> ha scritto:
>>>>>
>>>>> Hi, Paolo!
>>>>>
>>>>> 在 2022/08/25 22:59, Paolo Valente 写道:
>>>>>>> Il giorno 11 ago 2022, alle ore 03:19, Yu Kuai <yukuai1@huaweicloud.com <mailto:yukuai1@huaweicloud.com>> ha scritto:
>>>>>>>
>>>>>>> Hi, Paolo
>>>>>>>
>>>>>>> 在 2022/08/10 18:49, Paolo Valente 写道:
>>>>>>>>> Il giorno 27 lug 2022, alle ore 14:11, Yu Kuai <yukuai1@huaweicloud.com <mailto:yukuai1@huaweicloud.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
> 
> .
>