Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH block/for-5.4-fixes] blkcg: make blkcg_print_stat() print stats only for online blkgs
@ 2019-11-05 16:09 Tejun Heo
  2019-11-07  0:08 ` Jens Axboe
  0 siblings, 1 reply; 2+ messages in thread
From: Tejun Heo @ 2019-11-05 16:09 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Roman Gushchin, linux-block, cgroups, linux-kernel, kernel-team,
	Josef Bacik

blkcg_print_stat() iterates blkgs under RCU and doesn't test whether
the blkg is online.  This can call into pd_stat_fn() on a pd which is
still being initialized leading to an oops.

The heaviest operation - recursively summing up rwstat counters - is
already done while holding the queue_lock.  Expand queue_lock to cover
the other operations and skip the blkg if it isn't online yet.  The
online state is protected by both blkcg and queue locks, so this
guarantees that only online blkgs are processed.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Roman Gushchin <guro@fb.com>
Cc: Josef Bacik <jbacik@fb.com>
Fixes: 903d23f0a354 ("blk-cgroup: allow controllers to output their own stats")
Cc: stable@vger.kernel.org # v4.19+
---
 block/blk-cgroup.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -934,9 +934,14 @@ static int blkcg_print_stat(struct seq_f
 		int i;
 		bool has_stats = false;
 
+		spin_lock_irq(&blkg->q->queue_lock);
+
+		if (!blkg->online)
+			goto skip;
+
 		dname = blkg_dev_name(blkg);
 		if (!dname)
-			continue;
+			goto skip;
 
 		/*
 		 * Hooray string manipulation, count is the size written NOT
@@ -946,8 +951,6 @@ static int blkcg_print_stat(struct seq_f
 		 */
 		off += scnprintf(buf+off, size-off, "%s ", dname);
 
-		spin_lock_irq(&blkg->q->queue_lock);
-
 		blkg_rwstat_recursive_sum(blkg, NULL,
 				offsetof(struct blkcg_gq, stat_bytes), &rwstat);
 		rbytes = rwstat.cnt[BLKG_RWSTAT_READ];
@@ -960,8 +963,6 @@ static int blkcg_print_stat(struct seq_f
 		wios = rwstat.cnt[BLKG_RWSTAT_WRITE];
 		dios = rwstat.cnt[BLKG_RWSTAT_DISCARD];
 
-		spin_unlock_irq(&blkg->q->queue_lock);
-
 		if (rbytes || wbytes || rios || wios) {
 			has_stats = true;
 			off += scnprintf(buf+off, size-off,
@@ -999,6 +1000,8 @@ static int blkcg_print_stat(struct seq_f
 				seq_commit(sf, -1);
 			}
 		}
+	skip:
+		spin_unlock_irq(&blkg->q->queue_lock);
 	}
 
 	rcu_read_unlock();

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH block/for-5.4-fixes] blkcg: make blkcg_print_stat() print stats only for online blkgs
  2019-11-05 16:09 [PATCH block/for-5.4-fixes] blkcg: make blkcg_print_stat() print stats only for online blkgs Tejun Heo
@ 2019-11-07  0:08 ` Jens Axboe
  0 siblings, 0 replies; 2+ messages in thread
From: Jens Axboe @ 2019-11-07  0:08 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Roman Gushchin, linux-block, cgroups, linux-kernel, kernel-team,
	Josef Bacik

On 11/5/19 9:09 AM, Tejun Heo wrote:
> blkcg_print_stat() iterates blkgs under RCU and doesn't test whether
> the blkg is online.  This can call into pd_stat_fn() on a pd which is
> still being initialized leading to an oops.
> 
> The heaviest operation - recursively summing up rwstat counters - is
> already done while holding the queue_lock.  Expand queue_lock to cover
> the other operations and skip the blkg if it isn't online yet.  The
> online state is protected by both blkcg and queue locks, so this
> guarantees that only online blkgs are processed.

Applied, thanks Tejun.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 16:09 [PATCH block/for-5.4-fixes] blkcg: make blkcg_print_stat() print stats only for online blkgs Tejun Heo
2019-11-07  0:08 ` Jens Axboe

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git