Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Date:   Mon, 27 Jul 2020 11:17:35 +0200
From:   Oleksandr Natalenko <oleksandr@natalenko.name>
To:     Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
Cc:     linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        axboe@kernel.dk, paolo.valente@linaro.org
Subject: Re: [PATCH] block: bfq fix blkio cgroup leakage v3
In-Reply-To: <20200727080107.6431-1-dmtrmonakhov@yandex-team.ru>
References: <6422992afade0015d817a65c124e0c75@natalenko.name>
 <20200727080107.6431-1-dmtrmonakhov@yandex-team.ru>
User-Agent: Roundcube Webmail/1.4.7
Message-ID: <1bfaa79dd9d336b0943c863f918737b2@natalenko.name>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 27.07.2020 10:01, 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:
> 
> changes since v2:
>  - use safe iteration macro to prevent freed object dereference.
> 
> Signed-off-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
> ---
>  block/bfq-wf2q.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
> index 8113138..13b417a 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,
> @@ -1615,6 +1611,7 @@ bool __bfq_bfqd_reset_in_service(struct bfq_data 
> *bfqd)
>  	struct bfq_queue *in_serv_bfqq = bfqd->in_service_queue;
>  	struct bfq_entity *in_serv_entity = &in_serv_bfqq->entity;
>  	struct bfq_entity *entity = in_serv_entity;
> +	struct bfq_entity *parent = NULL;
> 
>  	bfq_clear_bfqq_wait_request(in_serv_bfqq);
>  	hrtimer_try_to_cancel(&bfqd->idle_slice_timer);
> @@ -1626,9 +1623,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_safe(entity, parent) {
>  		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

Reportedly, this one crashes too [1], and this happens even earlier than 
with v2.

[1] 
http://pix.academ.info/images/img/2020/07/27/91f656514707728730b0b67f8c9f4a04.jpg

-- 
   Oleksandr Natalenko (post-factum)