lkml.org 
[lkml]   [2022]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -next v10 3/4] block, bfq: refactor the counting of 'num_groups_with_pending_reqs'
From
Date
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
>
> .
>

\
 
 \ /
  Last update: 2022-09-14 10:16    [W:0.121 / U:2.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site