linux-kernel - Re: [PATCH] bfq: fix blkio cgroup leakage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <429E50C6-83BA-4A3F-BE9C-06C7C762AF33@linaro.org>
Date:   Wed, 8 Jul 2020 18:35:34 +0200
From:   Paolo Valente <paolo.valente@...aro.org>
To:     Dmitry Monakhov <dmonakhov@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
        axboe@...nel.dk
Subject: Re: [PATCH] bfq: fix blkio cgroup leakage

Hi,
sorry for the delay.  The commit you propose to drop fix the issues
reported in [1].

Such a commit does introduce the leak that you report (thank you for
spotting it).  Yet, according to the threads mentioned in [1],
dropping that commit would take us back to those issues.

Maybe the solution is to fix the unbalance that you spotted?

I'll check it ASAP, unless you do it before me.

Thanks,
Paolo

[1] https://lkml.org/lkml/2020/1/31/94

> Il giorno 2 lug 2020, alle ore 12:57, Dmitry Monakhov <dmonakhov@...il.com> ha scritto:
> 
> 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.
> If fact bfq_group objects reference counting are quite different
> from bfq_queue. bfq_groups object are referenced by blkcg_gq via
> blkg_policy_data pointer, so  neither nor blkg_get() neither bfqg_get
> required here.
> 
> 
> This patch drop commit db37a34c563b ("block, bfq: get a ref to a group when adding it to a service tree")
> and add corresponding comment.
> 
> ##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 <dmonakhov@...il.com>
> ---
> block/bfq-cgroup.c  |  2 +-
> block/bfq-iosched.h |  1 -
> block/bfq-wf2q.c    | 15 +++++----------
> 3 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index 68882b9..b791e20 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -332,7 +332,7 @@ static void bfqg_put(struct bfq_group *bfqg)
> 		kfree(bfqg);
> }
> 
> -void bfqg_and_blkg_get(struct bfq_group *bfqg)
> +static void bfqg_and_blkg_get(struct bfq_group *bfqg)
> {
> 	/* see comments in bfq_bic_update_cgroup for why refcounting bfqg */
> 	bfqg_get(bfqg);
> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
> index cd224aa..7038952 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -986,7 +986,6 @@ struct bfq_group *bfq_find_set_group(struct bfq_data *bfqd,
> struct blkcg_gq *bfqg_to_blkg(struct bfq_group *bfqg);
> struct bfq_group *bfqq_group(struct bfq_queue *bfqq);
> struct bfq_group *bfq_create_group_hierarchy(struct bfq_data *bfqd, int node);
> -void bfqg_and_blkg_get(struct bfq_group *bfqg);
> void bfqg_and_blkg_put(struct bfq_group *bfqg);
> 
> #ifdef CONFIG_BFQ_GROUP_IOSCHED
> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
> index 34ad095..6a363bb 100644
> --- a/block/bfq-wf2q.c
> +++ b/block/bfq-wf2q.c
> @@ -529,13 +529,14 @@ static void bfq_get_entity(struct bfq_entity *entity)
> {
> 	struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity);
> 
> +	/* Grab reference only for bfq_queue's objects, bfq_group ones
> +	 * are owned by blkcg_gq
> +	 */
> 	if (bfqq) {
> 		bfqq->ref++;
> 		bfq_log_bfqq(bfqq->bfqd, bfqq, "get_entity: %p %d",
> 			     bfqq, bfqq->ref);
> -	} else
> -		bfqg_and_blkg_get(container_of(entity, struct bfq_group,
> -					       entity));
> +	}
> }
> 
> /**
> @@ -649,14 +650,8 @@ static void bfq_forget_entity(struct bfq_service_tree *st,
> 
> 	entity->on_st_or_in_serv = false;
> 	st->wsum -= entity->weight;
> -	if (is_in_service)
> -		return;
> -
> -	if (bfqq)
> +	if (bfqq && !is_in_service)
> 		bfq_put_queue(bfqq);
> -	else
> -		bfqg_and_blkg_put(container_of(entity, struct bfq_group,
> -					       entity));
> }
> 
> /**
> -- 
> 2.7.4
>