[PATCH] block: bfq fix blkio cgroup leakage v2
From: Dmitry Monakhov <hidden>
Date: 2020-07-20 17:04:37
Also in:
lkml
Subsystem:
bfq i/o scheduler, block layer, the rest · Maintainers:
Yu Kuai, Jens Axboe, Linus Torvalds
commit db37a34c563b ("block, bfq: get a ref to a group when adding it to a service tree")
introduce leak forbfq_group and blkcg_gq objects because of get/put
imbalance. See trace balow:
-> blkg_alloc
-> bfq_pq_alloc
-> bfqg_get (+1)
->bfq_activate_bfqq
->bfq_activate_requeue_entity
-> __bfq_activate_entity
->bfq_get_entity
->bfqg_and_blkg_get (+1) <==== : Note1
->bfq_del_bfqq_busy
->bfq_deactivate_entity+0x53/0xc0 [bfq]
->__bfq_deactivate_entity+0x1b8/0x210 [bfq]
-> bfq_forget_entity(is_in_service = true)
entity->on_st_or_in_serv = false <=== :Note2
if (is_in_service)
return; ==> do not touch reference
-> blkcg_css_offline
-> blkcg_destroy_blkgs
-> blkg_destroy
-> bfq_pd_offline
-> __bfq_deactivate_entity
if (!entity->on_st_or_in_serv) /* true, because (Note2)
return false;
-> bfq_pd_free
-> bfqg_put() (-1, byt bfqg->ref == 2) because of (Note2)
So bfq_group and blkcg_gq will leak forever, see test-case below.
We should drop group reference once it finaly removed from service
inside __bfq_bfqd_reset_in_service, as we do with queue entities.
##TESTCASE_BEGIN:
#!/bin/bash
max_iters=${1:-100}
#prep cgroup mounts
mount -t tmpfs cgroup_root /sys/fs/cgroup
mkdir /sys/fs/cgroup/blkio
mount -t cgroup -o blkio none /sys/fs/cgroup/blkio
# Prepare blkdev
grep blkio /proc/cgroups
truncate -s 1M img
losetup /dev/loop0 img
echo bfq > /sys/block/loop0/queue/scheduler
grep blkio /proc/cgroups
for ((i=0;i<max_iters;i++))
do
mkdir -p /sys/fs/cgroup/blkio/a
echo 0 > /sys/fs/cgroup/blkio/a/cgroup.procs
dd if=/dev/loop0 bs=4k count=1 of=/dev/null iflag=direct 2> /dev/null
echo 0 > /sys/fs/cgroup/blkio/cgroup.procs
rmdir /sys/fs/cgroup/blkio/a
grep blkio /proc/cgroups
done
##TESTCASE_END:
Signed-off-by: Dmitry Monakhov <redacted>
---
block/bfq-wf2q.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index 8113138..93b236c 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c@@ -635,14 +635,10 @@ static void bfq_idle_insert(struct bfq_service_tree *st, * @entity: the entity being removed. * @is_in_service: true if entity is currently the in-service entity. * - * Forget everything about @entity. In addition, if entity represents - * a queue, and the latter is not in service, then release the service - * reference to the queue (the one taken through bfq_get_entity). In - * fact, in this case, there is really no more service reference to - * the queue, as the latter is also outside any service tree. If, - * instead, the queue is in service, then __bfq_bfqd_reset_in_service - * will take care of putting the reference when the queue finally - * stops being served. + * Forget everything about @entity. If entity is not in service, then release + * the service reference to the entity (the one taken through bfq_get_entity). + * If the entity is in service, then __bfq_bfqd_reset_in_service will take care + * of putting the reference when the entity finally stops being served. */ static void bfq_forget_entity(struct bfq_service_tree *st, struct bfq_entity *entity,
@@ -1626,9 +1622,16 @@ bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) * execute the final step: reset in_service_entity along the * path from entity to the root. */ - for_each_entity(entity) + for_each_entity(entity) { entity->sched_data->in_service_entity = NULL; - + /* + * Release bfq_groups reference if it was not released in + * bfq_forget_entity, which was taken in bfq_get_entity. + */ + if (!bfq_entity_to_bfqq(entity) && !entity->on_st_or_in_serv) + bfqg_and_blkg_put(container_of(entity, struct bfq_group, + entity)); + } /* * in_serv_entity is no longer in service, so, if it is in no * service tree either, then release the service reference to
--
2.7.4