All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
To: linux-kernel@vger.kernel.org
Cc: linux-block@vger.kernel.org, axboe@kernel.dk,
	paolo.valente@linaro.org,
	Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
Subject: [PATCH] block: bfq fix blkio cgroup leakage v2
Date: Mon, 20 Jul 2020 17:04:11 +0000	[thread overview]
Message-ID: <20200720170411.21250-1-dmtrmonakhov@yandex-team.ru> (raw)
In-Reply-To: <b4561a09-f00f-568b-9d55-0a2893de4be5@kernel.dk>

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
-- 
2.7.4


  reply	other threads:[~2020-07-20 17:04 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-02 10:57 [PATCH] bfq: fix blkio cgroup leakage Dmitry Monakhov
2020-07-08  9:03 ` Dmitry Monakhov
2020-07-08 16:35 ` Paolo Valente
2020-07-08 17:48   ` Dmitry Monakhov
2020-07-09  7:19     ` Paolo Valente
2020-07-09  8:19       ` Dmitry Monakhov
2020-07-09  8:35         ` Paolo Valente
2020-07-20 12:19           ` Dmitry Monakhov
2020-07-20 16:34             ` Jens Axboe
2020-07-20 17:04               ` Dmitry Monakhov [this message]
2020-07-26 11:31                 ` [PATCH] block: bfq fix blkio cgroup leakage v2 Oleksandr Natalenko
2020-07-27  8:01                   ` [PATCH] block: bfq fix blkio cgroup leakage v3 Dmitry Monakhov
2020-07-27  9:17                     ` Oleksandr Natalenko
2020-08-11  8:11           ` [PATCH] bfq: fix blkio cgroup leakage Dmitry Monakhov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200720170411.21250-1-dmtrmonakhov@yandex-team.ru \
    --to=dmtrmonakhov@yandex-team.ru \
    --cc=axboe@kernel.dk \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paolo.valente@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.