From: Yu Kuai <[email protected]>
Changes in v3:
- add ack tag from Tejun for patch 1,2
- as suggested by Tejun, update commit message and comments in patch 3
The problem was found in iocost orignally([1]) that ioc can be freed in
ioc_pd_free(). And later we found that there are more problem in
iocost([2]).
After some discussion, as suggested by Tejun([3]), we decide to fix the
problem that parent pd can be freed before child pd in cgroup layer
first. And the problem in [1] will be fixed later if this patchset is
applied.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
Yu Kuai (3):
blk-cgroup: dropping parent refcount after pd_free_fn() is done
blk-cgroup: support to track if policy is online
blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and
blkcg_deactivate_policy()
block/blk-cgroup.c | 63 ++++++++++++++++++++++++++++++++----------
block/blk-cgroup.h | 1 +
include/linux/blkdev.h | 1 +
3 files changed, 50 insertions(+), 15 deletions(-)
--
2.31.1
From: Yu Kuai <[email protected]>
Some cgroup policies will access parent pd through child pd even
after pd_offline_fn() is done. If pd_free_fn() for parent is called
before child, then UAF can be triggered. Hence it's better to guarantee
the order of pd_free_fn().
Currently refcount of parent blkg is dropped in __blkg_release(), which
is before pd_free_fn() is called in blkg_free_work_fn() while
blkg_free_work_fn() is called asynchronously.
This patch make sure pd_free_fn() called from removing cgroup is ordered
by delaying dropping parent refcount after calling pd_free_fn() for
child.
BTW, pd_free_fn() will also be called from blkcg_deactivate_policy()
from deleting device, and following patches will guarantee the order.
Signed-off-by: Yu Kuai <[email protected]>
Acked-by: Tejun Heo <[email protected]>
---
block/blk-cgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 4c94a6560f62..c6d7d1fce65a 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -124,6 +124,8 @@ static void blkg_free_workfn(struct work_struct *work)
if (blkg->pd[i])
blkcg_policy[i]->pd_free_fn(blkg->pd[i]);
+ if (blkg->parent)
+ blkg_put(blkg->parent);
if (blkg->q)
blk_put_queue(blkg->q);
free_percpu(blkg->iostat_cpu);
@@ -158,8 +160,6 @@ static void __blkg_release(struct rcu_head *rcu)
/* release the blkcg and parent blkg refs this blkg has been holding */
css_put(&blkg->blkcg->css);
- if (blkg->parent)
- blkg_put(blkg->parent);
blkg_free(blkg);
}
--
2.31.1
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On 1/19/23 4:03 AM, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> Changes in v3:
> - add ack tag from Tejun for patch 1,2
> - as suggested by Tejun, update commit message and comments in patch 3
>
> The problem was found in iocost orignally([1]) that ioc can be freed in
> ioc_pd_free(). And later we found that there are more problem in
> iocost([2]).
>
> After some discussion, as suggested by Tejun([3]), we decide to fix the
> problem that parent pd can be freed before child pd in cgroup layer
> first. And the problem in [1] will be fixed later if this patchset is
> applied.
Doesn't apply against for-6.3/block (or linux-next or my for-next, for
that matter). Can you resend a tested one against for-6.3/block?
--
Jens Axboe
Hi, Jens
在 2023/01/20 2:54, Jens Axboe 写道:
> On 1/19/23 4:03 AM, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> Changes in v3:
>> - add ack tag from Tejun for patch 1,2
>> - as suggested by Tejun, update commit message and comments in patch 3
>>
>> The problem was found in iocost orignally([1]) that ioc can be freed in
>> ioc_pd_free(). And later we found that there are more problem in
>> iocost([2]).
>>
>> After some discussion, as suggested by Tejun([3]), we decide to fix the
>> problem that parent pd can be freed before child pd in cgroup layer
>> first. And the problem in [1] will be fixed later if this patchset is
>> applied.
>
> Doesn't apply against for-6.3/block (or linux-next or my for-next, for
> that matter). Can you resend a tested one against for-6.3/block?
>
This is weird, I just test latest linux-next, and I can apply this
patchset on the top of following commit:
For latest for-6.3/block, this patch 2 can't be applied because
following commit is not here:
e3ff8887e7db blk-cgroup: fix missing pd_online_fn() while activating policy
But this patch is already merged into 6.2-rc5.
Thanks,
Kuai
On 1/28/23 11:06 PM, Yu Kuai wrote:
> Hi, Jens
>
> 在 2023/01/20 2:54, Jens Axboe 写道:
>> On 1/19/23 4:03 AM, Yu Kuai wrote:
>>> From: Yu Kuai <[email protected]>
>>>
>>> Changes in v3:
>>> - add ack tag from Tejun for patch 1,2
>>> - as suggested by Tejun, update commit message and comments in patch 3
>>>
>>> The problem was found in iocost orignally([1]) that ioc can be freed in
>>> ioc_pd_free(). And later we found that there are more problem in
>>> iocost([2]).
>>>
>>> After some discussion, as suggested by Tejun([3]), we decide to fix the
>>> problem that parent pd can be freed before child pd in cgroup layer
>>> first. And the problem in [1] will be fixed later if this patchset is
>>> applied.
>>
>> Doesn't apply against for-6.3/block (or linux-next or my for-next, for
>> that matter). Can you resend a tested one against for-6.3/block?
>>
>
> This is weird, I just test latest linux-next, and I can apply this
> patchset on the top of following commit:
>
> For latest for-6.3/block, this patch 2 can't be applied because
> following commit is not here:
>
> e3ff8887e7db blk-cgroup: fix missing pd_online_fn() while activating policy
>
> But this patch is already merged into 6.2-rc5.
Since I have one more conflict, I think we'll just rebase for-6.3/block
when -rc6 is out, and then it should apply cleanly.
--
Jens Axboe
On Thu, 19 Jan 2023 19:03:47 +0800, Yu Kuai wrote:
> Changes in v3:
> - add ack tag from Tejun for patch 1,2
> - as suggested by Tejun, update commit message and comments in patch 3
>
> The problem was found in iocost orignally([1]) that ioc can be freed in
> ioc_pd_free(). And later we found that there are more problem in
> iocost([2]).
>
> [...]
Applied, thanks!
[1/3] blk-cgroup: dropping parent refcount after pd_free_fn() is done
commit: c7241babf0855d8a6180cd1743ff0ec34de40b4e
[2/3] blk-cgroup: support to track if policy is online
commit: dfd6200a095440b663099d8d42f1efb0175a1ce3
[3/3] blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()
commit: f1c006f1c6850c14040f8337753a63119bba39b9
Best regards,
--
Jens Axboe