| Message ID | 20230221082905.3389012-1-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block, bfq: free 'sync_bfqq' after bic_set_bfqq() in bfq_sync_bfqq_move() | expand |
On 2/21/23 17:29, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > As explained in commit b600de2d7d3a ("block, bfq: fix uaf for bfqq in > bic_set_bfqq()"), bfqq should not be freed before bic_set_bfqq(). > However, this is broken while merging commit 9778369a2d6c ("block, bfq: > split sync bfq_queues on a per-actuator basis") from branch > for-6.3/block. The patch looks OK to me, but the commit message is not super clear. What is broken exactly ? > > Fixes: 9778369a2d6c ("block, bfq: split sync bfq_queues on a per-actuator basis") > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-cgroup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c > index ea3638e06e04..89ffb3aa992c 100644 > --- a/block/bfq-cgroup.c > +++ b/block/bfq-cgroup.c > @@ -746,8 +746,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, > * old cgroup. > */ > bfq_put_cooperator(sync_bfqq); > - bfq_release_process_ref(bfqd, sync_bfqq); > bic_set_bfqq(bic, NULL, true, act_idx); > + bfq_release_process_ref(bfqd, sync_bfqq); > } > } >
Hi, 在 2023/02/21 17:14, Damien Le Moal 写道: > On 2/21/23 17:29, Yu Kuai wrote: >> From: Yu Kuai <yukuai3@huawei.com> >> >> As explained in commit b600de2d7d3a ("block, bfq: fix uaf for bfqq in >> bic_set_bfqq()"), bfqq should not be freed before bic_set_bfqq(). >> However, this is broken while merging commit 9778369a2d6c ("block, bfq: >> split sync bfq_queues on a per-actuator basis") from branch >> for-6.3/block. > > The patch looks OK to me, but the commit message is not super clear. What is > broken exactly ? 1) bfq_sync_bfqq_move() is introduced in commit 9778369a2d6c ("block, bfq: split sync bfq_queues on a per-actuator basis"), which is merged to block/for-6.3 branch. 2) commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") is merged to mainline. 3) later, the fix for 2) b600de2d7d3a ("block, bfq: fix uaf for bfqq in bic_set_bfqq()") is merged to mainline as well, however, bfq_sync_bfqq_move() in block/for-6.3 branch is not changed. 4) At last, 1) is merged to mainline and bfq_sync_bfqq_move() is still problematic. Thanks, Kuai > >> >> Fixes: 9778369a2d6c ("block, bfq: split sync bfq_queues on a per-actuator basis") >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> block/bfq-cgroup.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c >> index ea3638e06e04..89ffb3aa992c 100644 >> --- a/block/bfq-cgroup.c >> +++ b/block/bfq-cgroup.c >> @@ -746,8 +746,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, >> * old cgroup. >> */ >> bfq_put_cooperator(sync_bfqq); >> - bfq_release_process_ref(bfqd, sync_bfqq); >> bic_set_bfqq(bic, NULL, true, act_idx); >> + bfq_release_process_ref(bfqd, sync_bfqq); >> } >> } >> >
On 2/21/23 1:29 AM, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > As explained in commit b600de2d7d3a ("block, bfq: fix uaf for bfqq in > bic_set_bfqq()"), bfqq should not be freed before bic_set_bfqq(). > However, this is broken while merging commit 9778369a2d6c ("block, bfq: > split sync bfq_queues on a per-actuator basis") from branch > for-6.3/block. > > Fixes: 9778369a2d6c ("block, bfq: split sync bfq_queues on a per-actuator basis") > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-cgroup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c > index ea3638e06e04..89ffb3aa992c 100644 > --- a/block/bfq-cgroup.c > +++ b/block/bfq-cgroup.c > @@ -746,8 +746,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, > * old cgroup. > */ > bfq_put_cooperator(sync_bfqq); > - bfq_release_process_ref(bfqd, sync_bfqq); > bic_set_bfqq(bic, NULL, true, act_idx); > + bfq_release_process_ref(bfqd, sync_bfqq); > } > } This is already in -git, see my reply to Linus. Eg this will apply to for-6.3/block, but it will not apply to current master that has it merged because we got that from the 6.2 side.
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index ea3638e06e04..89ffb3aa992c 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -746,8 +746,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, * old cgroup. */ bfq_put_cooperator(sync_bfqq); - bfq_release_process_ref(bfqd, sync_bfqq); bic_set_bfqq(bic, NULL, true, act_idx); + bfq_release_process_ref(bfqd, sync_bfqq); } }