Hi,
在 2022/09/14 17:00, Jan Kara 写道:
Hi guys!
On Wed 14-09-22 16:15:26, Yu Kuai wrote:
在 2022/09/14 15:50, Paolo VALENTE 写道:
Il giorno 14 set 2022, alle ore 03:55, Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> ha scritto:
Hi, Paolo!
在 2022/08/25 22:59, Paolo Valente 写道:
Il giorno 11 ago 2022, alle ore 03:19, Yu Kuai <yukuai1@xxxxxxxxxxxxxxx <mailto:yukuai1@xxxxxxxxxxxxxxx>> ha scritto:
Hi, Paolo
在 2022/08/10 18:49, Paolo Valente 写道:
Il giorno 27 lug 2022, alle ore 14:11, Yu Kuai <yukuai1@xxxxxxxxxxxxxxx <mailto:yukuai1@xxxxxxxxxxxxxxx>> 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...
I'm bit confused which comment you are speaking about but
bfq_completed_request() gets called only from bfq_finish_requeue_request()
and the request itself still holds a reference to bfqq. Only later in
bfq_finish_requeue_request() when we do:
bfqq_request_freed(bfqq);
bfq_put_queue(bfqq);
bfqq can get freed.
Yes, you're right. Then I think the only place that
bfq_weights_tree_remove() can free bfqq is from bfq_del_bfqq_busy().
I'll move the following comment with a little adjustment here, which is
from bfq_weights_tree_remove() before this patchset:
/*
┊* Next function is invoked last, because it causes bfqq to be
┊* freed. DO NOT use bfqq after the next function invocation.
┊*/
Thanks,
Kuai
Honza
