On Thu 13-01-22 15:08:50, yukuai (C) wrote:
在 2022/01/13 11:57, yukuai (C) 写道:
在 2022/01/12 19:39, Jan Kara 写道:
When bfqq is shared by multiple processes it can happen that one of the processes gets moved to a different cgroup (or just starts submitting IO for different cgroup). In case that happens we need to split the merged bfqq as otherwise we will have IO for multiple cgroups in one bfqq and we will just account IO time to wrong entities etc.
Similarly if the bfqq is scheduled to merge with another bfqq but the merge didn't happen yet, cancel the merge as it need not be valid anymore.
CC: stable@vger.kernel.org Fixes: e21b7a0b9887 ("block, bfq: add full hierarchical scheduling and cgroups support") Signed-off-by: Jan Kara jack@suse.cz
block/bfq-cgroup.c | 25 ++++++++++++++++++++++++- block/bfq-iosched.c | 2 +- block/bfq-iosched.h | 1 + 3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index 24a5c5329bcd..dbc117e00783 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -730,8 +730,31 @@ static struct bfq_group *__bfq_bic_change_cgroup(struct bfq_data *bfqd, if (sync_bfqq) { entity = &sync_bfqq->entity; - if (entity->sched_data != &bfqg->sched_data) + if (entity->sched_data != &bfqg->sched_data) { + /* + * Was the queue we use merged to a different queue? + * Detach process from the queue as merge need not be + * valid anymore. We cannot easily cancel the merge as + * there may be other processes scheduled to this + * queue. + */ + if (sync_bfqq->new_bfqq) { + bfq_put_cooperator(sync_bfqq);
Hi,
The patch " bfq: Simplify bfq_put_cooperator()" in last version is not in this patch set, thus bfq_put_cooperator() won't set sync_bfqq->new_bfqq to NULL. So I guess the problem still exist?
Thanks, Kuai
+ bfq_release_process_ref(bfqd, sync_bfqq); + bic_set_bfqq(bic, NULL, 1);
Hi,
I understand now that you set NULL here,
Yes.
however I still can repoduce the problem with this patch set applied.
OK.
Here I add some debug message:
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index dbc117e00783..2d3da8e73ec0 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -754,6 +754,7 @@ static struct bfq_group *__bfq_bic_change_cgroup(struct bfq_data *bfqd, bfq_mark_bfqq_split_coop(sync_bfqq); } bfq_bfqq_move(bfqd, sync_bfqq, bfqg);
printk("%s: bfqq %px move to %px\n", __func__,sync_bfqq, bfqg); } }
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 07be51bc229b..74d5b575626f 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -2797,6 +2797,7 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq) * are likely to increase the throughput. */ bfqq->new_bfqq = new_bfqq;
printk("%s: bfqq %px new_bfqq %px bfqg %px\n", __func__, bfqq,new_bfqq, bfqq_group(bfqq)); new_bfqq->ref += process_refs; return new_bfqq; } @@ -2963,8 +2964,14 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, if (bfq_too_late_for_merging(bfqq)) return NULL;
if (bfqq->new_bfqq)
if (bfqq->new_bfqq) {if (bfqq->entity.parent != bfqq->new_bfqq->entity.parent) {printk("%s: bfqq %px (%px), new_bfqq %px (%px)\n",__func__,
bfqq, bfqq_group(bfqq),bfqq->new_bfqq, bfqq_group(bfqq->new_bfqq));
BUG_ON(1);} return bfqq->new_bfqq;} if (!io_struct || unlikely(bfqq == &bfqd->oom_bfqq)) return NULL;And here is the messages when BUG_ON is triggered（much easier than the uaf problem):
[ 157.237821] bfq_setup_merge: bfqq ffff888140654dc0 new_bfqq ffff888174122100 bfqg ffff88817abc6000 [ 157.238739] __bfq_bic_change_cgroup: bfqq ffff888174122100 move to ffff888104b7c000 [ 157.239675] bfq_setup_cooperator: bfqq ffff888140654dc0 (ffff88817abc6000), new_bfqq ffff888174122100 (ffff888104b7c000)
Thanks for the debugging! So this shows that it is not 'bfqq' that changed parent but rather bfqq->new_bfqq that changed parent. And presumably it can even happen that the bfqq->new_bfqq parent gets offlined and UAF eventually triggered. Indeed, I didn't think about that possibility. I have to think how to handle this in the best way...
Honza