[block:for-linus 28/28] block/blk-cgroup.c:1429:18-29: ERROR: reference preceded by free on line 1419 (fwd)
9:33 p.m.
The combination of lines 1419 and 1429 looks suspicious. Perhaps
pd_prealloc should be set to NULL after it is freed.
julia
---------- Forwarded message ----------
Date: Tue, 15 Oct 2019 13:18:49 +0800
From: kbuild test robot <lkp(a)intel.com>
To: kbuild(a)lists.01.org
Cc: Julia Lawall <julia.lawall(a)lip6.fr>
Subject: [block:for-linus 28/28] block/blk-cgroup.c:1429:18-29: ERROR: reference
preceded by free on line 1419
CC: kbuild-all(a)lists.01.org
TO: Tejun Heo <tj(a)kernel.org>
CC: Jens Axboe <axboe(a)kernel.dk>
tree: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-linus
head: ab94b0382d81d1e5384ccf8b088f8e45f280b131
commit: ab94b0382d81d1e5384ccf8b088f8e45f280b131 [28/28] blkcg: Fix ->pd_alloc_fn()
being called with the wrong blkcg on policy activation
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp(a)intel.com>
Reported-by: Julia Lawall <julia.lawall(a)lip6.fr>
> block/blk-cgroup.c:1429:18-29: ERROR: reference preceded by free
on line 1419
#
https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/com...
git remote add block
https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
git remote update block
git checkout ab94b0382d81d1e5384ccf8b088f8e45f280b131
vim +1429 block/blk-cgroup.c
a2b1693bac45ea Tejun Heo 2012-04-13 1344
a2b1693bac45ea Tejun Heo 2012-04-13 1345 /**
a2b1693bac45ea Tejun Heo 2012-04-13 1346 * blkcg_activate_policy - activate a
blkcg policy on a request_queue
a2b1693bac45ea Tejun Heo 2012-04-13 1347 * @q: request_queue of interest
a2b1693bac45ea Tejun Heo 2012-04-13 1348 * @pol: blkcg policy to activate
a2b1693bac45ea Tejun Heo 2012-04-13 1349 *
a2b1693bac45ea Tejun Heo 2012-04-13 1350 * Activate @pol on @q. Requires
%GFP_KERNEL context. @q goes through
a2b1693bac45ea Tejun Heo 2012-04-13 1351 * bypass mode to populate its blkgs
with policy_data for @pol.
a2b1693bac45ea Tejun Heo 2012-04-13 1352 *
a2b1693bac45ea Tejun Heo 2012-04-13 1353 * Activation happens with @q bypassed,
so nobody would be accessing blkgs
a2b1693bac45ea Tejun Heo 2012-04-13 1354 * from IO path. Update of each blkg
is protected by both queue and blkcg
a2b1693bac45ea Tejun Heo 2012-04-13 1355 * locks so that holding either lock
and testing blkcg_policy_enabled() is
a2b1693bac45ea Tejun Heo 2012-04-13 1356 * always enough for dereferencing
policy data.
a2b1693bac45ea Tejun Heo 2012-04-13 1357 *
a2b1693bac45ea Tejun Heo 2012-04-13 1358 * The caller is responsible for
synchronizing [de]activations and policy
a2b1693bac45ea Tejun Heo 2012-04-13 1359 * [un]registerations. Returns 0 on
success, -errno on failure.
a2b1693bac45ea Tejun Heo 2012-04-13 1360 */
a2b1693bac45ea Tejun Heo 2012-04-13 1361 int blkcg_activate_policy(struct
request_queue *q,
3c798398e393e5 Tejun Heo 2012-04-16 1362 const struct blkcg_policy *pol)
923adde1be1df5 Tejun Heo 2012-03-05 1363 {
4c55f4f9ad3001 Tejun Heo 2015-08-18 1364 struct blkg_policy_data *pd_prealloc =
NULL;
ab94b0382d81d1 Tejun Heo 2019-10-14 1365 struct blkcg_gq *blkg, *pinned_blkg =
NULL;
4c55f4f9ad3001 Tejun Heo 2015-08-18 1366 int ret;
923adde1be1df5 Tejun Heo 2012-03-05 1367
a2b1693bac45ea Tejun Heo 2012-04-13 1368 if (blkcg_policy_enabled(q, pol))
a2b1693bac45ea Tejun Heo 2012-04-13 1369 return 0;
923adde1be1df5 Tejun Heo 2012-03-05 1370
344e9ffcbd1898 Jens Axboe 2018-11-15 1371 if (queue_is_mq(q))
bd166ef183c263 Jens Axboe 2017-01-17 1372 blk_mq_freeze_queue(q);
ab94b0382d81d1 Tejun Heo 2019-10-14 1373 retry:
0d945c1f966b2b Christoph Hellwig 2018-11-15 1374 spin_lock_irq(&q->queue_lock);
a2b1693bac45ea Tejun Heo 2012-04-13 1375
71c814077de60b Tejun Heo 2019-06-13 1376 /* blkg_list is pushed at the head,
reverse walk to init parents first */
71c814077de60b Tejun Heo 2019-06-13 1377 list_for_each_entry_reverse(blkg,
&q->blkg_list, q_node) {
4c55f4f9ad3001 Tejun Heo 2015-08-18 1378 struct blkg_policy_data *pd;
a2b1693bac45ea Tejun Heo 2012-04-13 1379
4c55f4f9ad3001 Tejun Heo 2015-08-18 1380 if (blkg->pd[pol->plid])
4c55f4f9ad3001 Tejun Heo 2015-08-18 1381 continue;
4c55f4f9ad3001 Tejun Heo 2015-08-18 1382
ab94b0382d81d1 Tejun Heo 2019-10-14 1383 if (blkg == pinned_blkg) {
ab94b0382d81d1 Tejun Heo 2019-10-14 1384 pd = pd_prealloc;
ab94b0382d81d1 Tejun Heo 2019-10-14 1385 pd_prealloc = NULL;
ab94b0382d81d1 Tejun Heo 2019-10-14 1386 } else {
ab94b0382d81d1 Tejun Heo 2019-10-14 1387 pd = pol->pd_alloc_fn(GFP_NOWAIT
| __GFP_NOWARN, q,
ab94b0382d81d1 Tejun Heo 2019-10-14 1388 blkg->blkcg);
ab94b0382d81d1 Tejun Heo 2019-10-14 1389 }
ab94b0382d81d1 Tejun Heo 2019-10-14 1390
4c55f4f9ad3001 Tejun Heo 2015-08-18 1391 if (!pd) {
ab94b0382d81d1 Tejun Heo 2019-10-14 1392 if (pinned_blkg)
ab94b0382d81d1 Tejun Heo 2019-10-14 1393 blkg_put(pinned_blkg);
ab94b0382d81d1 Tejun Heo 2019-10-14 1394 blkg_get(blkg);
ab94b0382d81d1 Tejun Heo 2019-10-14 1395 pinned_blkg = blkg;
ab94b0382d81d1 Tejun Heo 2019-10-14 1396
0d945c1f966b2b Christoph Hellwig 2018-11-15 1397
spin_unlock_irq(&q->queue_lock);
ab94b0382d81d1 Tejun Heo 2019-10-14 1398
ab94b0382d81d1 Tejun Heo 2019-10-14 1399 kfree(pd_prealloc);
ab94b0382d81d1 Tejun Heo 2019-10-14 1400 pd_prealloc =
pol->pd_alloc_fn(GFP_KERNEL, q,
ab94b0382d81d1 Tejun Heo 2019-10-14 1401 blkg->blkcg);
ab94b0382d81d1 Tejun Heo 2019-10-14 1402 if (pd_prealloc) {
ab94b0382d81d1 Tejun Heo 2019-10-14 1403 goto retry;
ab94b0382d81d1 Tejun Heo 2019-10-14 1404 } else {
ab94b0382d81d1 Tejun Heo 2019-10-14 1405 ret = -ENOMEM;
ab94b0382d81d1 Tejun Heo 2019-10-14 1406 goto out_bypass_end;
ab94b0382d81d1 Tejun Heo 2019-10-14 1407 }
4c55f4f9ad3001 Tejun Heo 2015-08-18 1408 }
a2b1693bac45ea Tejun Heo 2012-04-13 1409
a2b1693bac45ea Tejun Heo 2012-04-13 1410 blkg->pd[pol->plid] = pd;
a2b1693bac45ea Tejun Heo 2012-04-13 1411 pd->blkg = blkg;
b276a876a014c5 Tejun Heo 2013-01-09 1412 pd->plid = pol->plid;
3e41871046bfe0 Tejun Heo 2015-08-18 1413 if (pol->pd_init_fn)
a9520cd6f2ac1f Tejun Heo 2015-08-18 1414 pol->pd_init_fn(pd);
a2b1693bac45ea Tejun Heo 2012-04-13 1415 }
a2b1693bac45ea Tejun Heo 2012-04-13 1416
ab94b0382d81d1 Tejun Heo 2019-10-14 1417 if (pinned_blkg)
ab94b0382d81d1 Tejun Heo 2019-10-14 1418 blkg_put(pinned_blkg);
ab94b0382d81d1 Tejun Heo 2019-10-14 @1419 kfree(pd_prealloc);
ab94b0382d81d1 Tejun Heo 2019-10-14 1420
a2b1693bac45ea Tejun Heo 2012-04-13 1421 __set_bit(pol->plid,
q->blkcg_pols);
a2b1693bac45ea Tejun Heo 2012-04-13 1422 ret = 0;
4c55f4f9ad3001 Tejun Heo 2015-08-18 1423
0d945c1f966b2b Christoph Hellwig 2018-11-15 1424
spin_unlock_irq(&q->queue_lock);
4c55f4f9ad3001 Tejun Heo 2015-08-18 1425 out_bypass_end:
344e9ffcbd1898 Jens Axboe 2018-11-15 1426 if (queue_is_mq(q))
bd166ef183c263 Jens Axboe 2017-01-17 1427 blk_mq_unfreeze_queue(q);
001bea73e70efd Tejun Heo 2015-08-18 1428 if (pd_prealloc)
001bea73e70efd Tejun Heo 2015-08-18 @1429 pol->pd_free_fn(pd_prealloc);
a2b1693bac45ea Tejun Heo 2012-04-13 1430 return ret;
a2b1693bac45ea Tejun Heo 2012-04-13 1431 }
a2b1693bac45ea Tejun Heo 2012-04-13 1432
EXPORT_SYMBOL_GPL(blkcg_activate_policy);
923adde1be1df5 Tejun Heo 2012-03-05 1433
:::::: The code at line 1429 was first introduced by commit
:::::: 001bea73e70efdf48a9e00188cf302f6b6aed2bf blkcg: replace blkcg_policy->pd_size
with ->pd_alloc/free_fn() methods
:::::: TO: Tejun Heo <tj(a)kernel.org>
:::::: CC: Jens Axboe <axboe(a)fb.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
2:48 a.m.
New subject: [PATCH block/for-linus] blkcg: fix botched pd_prealloc error handling in blkcg_activate_policy()
While fixing ->pd_alloc_fn() bug, ab94b0382d81 ("blkcg: Fix
->pd_alloc_fn() being called with the wrong blkcg on policy
activation") broke the pd_prealloc error handling.
* pd's were freed using kfree(). They should be freed with
->pd_free_fn().
* pd_prealloc could be kfree()'d and then ->pd_free_fn()'d again.
* When GFP_KERNEL allocation fails, pinned_blkg wasn't put.
There are also a couple existing issues.
* Each pd is initialized as they get allocated. If alloc fails, the
policy will get freed with pd's initialized on it.
* After the above partial failure, the partial pds are not freed.
This patch fixes all of the above issues.
Signed-off-by: Tejun Heo <tj(a)kernel.org>
Reported-by: Julia Lawall <julia.lawall(a)lip6.fr>
Fixes: ab94b0382d81 ("blkcg: Fix ->pd_alloc_fn() being called with the wrong blkcg
on policy activation")
---
block/blk-cgroup.c | 44 +++++++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 13 deletions(-)
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1373,13 +1373,14 @@ int blkcg_activate_policy(struct request
retry:
spin_lock_irq(&q->queue_lock);
- /* blkg_list is pushed at the head, reverse walk to init parents first */
+ /* blkg_list is pushed at the head, reverse walk to allocate parents first */
list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) {
struct blkg_policy_data *pd;
if (blkg->pd[pol->plid])
continue;
+ /* If prealloc matches, use it; otherwise try GFP_NOWAIT */
if (blkg == pinned_blkg) {
pd = pd_prealloc;
pd_prealloc = NULL;
@@ -1389,6 +1390,10 @@ retry:
}
if (!pd) {
+ /*
+ * GFP_NOWAIT failed. Free the existing one and
+ * prealloc for @blkg w/ GFP_KERNEL.
+ */
if (pinned_blkg)
blkg_put(pinned_blkg);
blkg_get(blkg);
@@ -1396,38 +1401,51 @@ retry:
spin_unlock_irq(&q->queue_lock);
- kfree(pd_prealloc);
+ if (pd_prealloc)
+ pol->pd_free_fn(pd_prealloc);
pd_prealloc = pol->pd_alloc_fn(GFP_KERNEL, q,
blkg->blkcg);
- if (pd_prealloc) {
+ if (pd_prealloc)
goto retry;
- } else {
- ret = -ENOMEM;
- goto out_bypass_end;
- }
+ else
+ goto enomem;
}
blkg->pd[pol->plid] = pd;
pd->blkg = blkg;
pd->plid = pol->plid;
- if (pol->pd_init_fn)
- pol->pd_init_fn(pd);
}
- if (pinned_blkg)
- blkg_put(pinned_blkg);
- kfree(pd_prealloc);
+ /* all allocated, init in the same order */
+ if (pol->pd_init_fn)
+ list_for_each_entry_reverse(blkg, &q->blkg_list, q_node)
+ pol->pd_init_fn(blkg->pd[pol->plid]);
__set_bit(pol->plid, q->blkcg_pols);
ret = 0;
spin_unlock_irq(&q->queue_lock);
-out_bypass_end:
+out:
if (queue_is_mq(q))
blk_mq_unfreeze_queue(q);
+ if (pinned_blkg)
+ blkg_put(pinned_blkg);
if (pd_prealloc)
pol->pd_free_fn(pd_prealloc);
return ret;
+
+enomem:
+ /* alloc failed, nothing's initialized yet, free everything */
+ spin_lock_irq(&q->queue_lock);
+ list_for_each_entry(blkg, &q->blkg_list, q_node) {
+ if (blkg->pd[pol->plid]) {
+ pol->pd_free_fn(blkg->pd[pol->plid]);
+ blkg->pd[pol->plid] = NULL;
+ }
+ }
+ spin_unlock_irq(&q->queue_lock);
+ ret = -ENOMEM;
+ goto out;
}
EXPORT_SYMBOL_GPL(blkcg_activate_policy);
1071
days inactive
1071
days old
1 comments
2 participants
participants (2)
-
Julia Lawall -
Tejun Heo