| Message ID | 20200720170411.21250-1-dmtrmonakhov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: bfq fix blkio cgroup leakage v2 | expand |
Hello. On 20.07.2020 19:04, Dmitry Monakhov wrote: > 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 <dmtrmonakhov@yandex-team.ru> > --- > 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 As reported by one of my customers, this patch causes the following crash: [1] [1] http://pix.academ.info/images/img/2020/07/26/52d097c02b6061657443bba92de75e8a.jpg
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
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 <dmtrmonakhov@yandex-team.ru> --- block/bfq-wf2q.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)