lkml.org 
[lkml]   [2020]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] block: bfq fix blkio cgroup leakage v2
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

--
Oleksandr Natalenko (post-factum)

\
 
 \ /
  Last update: 2020-07-26 13:32    [W:0.091 / U:0.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site