[PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[Ming Lei]

blkcg->lock depends on q->queue_lock which may depend on another driver
lock required in irq context, one example is dm-thin:

	Chain exists of:
	  &pool->lock#3 --> &q->queue_lock --> &blkcg->lock

	 Possible interrupt unsafe locking scenario:

	       CPU0                    CPU1
	       ----                    ----
	  lock(&blkcg->lock);
	                               local_irq_disable();
	                               lock(&pool->lock#3);
	                               lock(&q->queue_lock);
	  <Interrupt>
	    lock(&pool->lock#3);

Fix the issue by using spin_lock_irq(&blkcg->lock) in ioc_weight_write().

Cc: Tejun Heo <tj@kernel.org>
Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
Link: https://lore.kernel.org/linux-block/CA+QYu4rzz6079ighEanS3Qq_Dmnczcf45ZoJoHKVLVATTo1e4Q@mail.gmail.com/T/#u
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-iocost.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 5fac3757e6e0..0e56557cacf2 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3061,19 +3061,19 @@ static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf,
 		if (v < CGROUP_WEIGHT_MIN || v > CGROUP_WEIGHT_MAX)
 			return -EINVAL;
 
-		spin_lock(&blkcg->lock);
+		spin_lock_irq(&blkcg->lock);
 		iocc->dfl_weight = v * WEIGHT_ONE;
 		hlist_for_each_entry(blkg, &blkcg->blkg_list, blkcg_node) {
 			struct ioc_gq *iocg = blkg_to_iocg(blkg);
 
 			if (iocg) {
-				spin_lock_irq(&iocg->ioc->lock);
+				spin_lock(&iocg->ioc->lock);
 				ioc_now(iocg->ioc, &now);
 				weight_updated(iocg, &now);
-				spin_unlock_irq(&iocg->ioc->lock);
+				spin_unlock(&iocg->ioc->lock);
 			}
 		}
-		spin_unlock(&blkcg->lock);
+		spin_unlock_irq(&blkcg->lock);
 
 		return nbytes;
 	}
-- 
2.31.1

Re: [PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[Jens Axboe]

On 8/3/21 1:06 AM, Ming Lei wrote:
> blkcg->lock depends on q->queue_lock which may depend on another driver
> lock required in irq context, one example is dm-thin:
> 
> 	Chain exists of:
> 	  &pool->lock#3 --> &q->queue_lock --> &blkcg->lock
> 
> 	 Possible interrupt unsafe locking scenario:
> 
> 	       CPU0                    CPU1
> 	       ----                    ----
> 	  lock(&blkcg->lock);
> 	                               local_irq_disable();
> 	                               lock(&pool->lock#3);
> 	                               lock(&q->queue_lock);
> 	  <Interrupt>
> 	    lock(&pool->lock#3);
> 
> Fix the issue by using spin_lock_irq(&blkcg->lock) in ioc_weight_write().

This looks fine to me for blk-iocost, but block/blk-cgroup.c:blkg_create()
also looks like it gets the IRQ state of the same lock wrong?

-- 
Jens Axboe

Re: [PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[Jens Axboe]

On 8/3/21 7:02 AM, Jens Axboe wrote:
> On 8/3/21 1:06 AM, Ming Lei wrote:
>> blkcg->lock depends on q->queue_lock which may depend on another driver
>> lock required in irq context, one example is dm-thin:
>>
>> 	Chain exists of:
>> 	  &pool->lock#3 --> &q->queue_lock --> &blkcg->lock
>>
>> 	 Possible interrupt unsafe locking scenario:
>>
>> 	       CPU0                    CPU1
>> 	       ----                    ----
>> 	  lock(&blkcg->lock);
>> 	                               local_irq_disable();
>> 	                               lock(&pool->lock#3);
>> 	                               lock(&q->queue_lock);
>> 	  <Interrupt>
>> 	    lock(&pool->lock#3);
>>
>> Fix the issue by using spin_lock_irq(&blkcg->lock) in ioc_weight_write().
> 
> This looks fine to me for blk-iocost, but block/blk-cgroup.c:blkg_create()
> also looks like it gets the IRQ state of the same lock wrong?

Ah, that one is under the queue lock, so irqs are already disabled.

-- 
Jens Axboe

Re: [PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[Ming Lei]

On Tue, Aug 03, 2021 at 07:02:28AM -0600, Jens Axboe wrote:
> On 8/3/21 1:06 AM, Ming Lei wrote:
> > blkcg->lock depends on q->queue_lock which may depend on another driver
> > lock required in irq context, one example is dm-thin:
> > 
> > 	Chain exists of:
> > 	  &pool->lock#3 --> &q->queue_lock --> &blkcg->lock
> > 
> > 	 Possible interrupt unsafe locking scenario:
> > 
> > 	       CPU0                    CPU1
> > 	       ----                    ----
> > 	  lock(&blkcg->lock);
> > 	                               local_irq_disable();
> > 	                               lock(&pool->lock#3);
> > 	                               lock(&q->queue_lock);
> > 	  <Interrupt>
> > 	    lock(&pool->lock#3);
> > 
> > Fix the issue by using spin_lock_irq(&blkcg->lock) in ioc_weight_write().
> 
> This looks fine to me for blk-iocost, but block/blk-cgroup.c:blkg_create()
> also looks like it gets the IRQ state of the same lock wrong?

blkg_create() is called with irq disabled in all three callers: 
blkg_lookup_create(), blkg_conf_prep() and blkcg_init_queue().

-- 
Ming

Re: [PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[Jens Axboe]

On 8/3/21 7:25 PM, Ming Lei wrote:
> On Tue, Aug 03, 2021 at 07:02:28AM -0600, Jens Axboe wrote:
>> On 8/3/21 1:06 AM, Ming Lei wrote:
>>> blkcg->lock depends on q->queue_lock which may depend on another driver
>>> lock required in irq context, one example is dm-thin:
>>>
>>> 	Chain exists of:
>>> 	  &pool->lock#3 --> &q->queue_lock --> &blkcg->lock
>>>
>>> 	 Possible interrupt unsafe locking scenario:
>>>
>>> 	       CPU0                    CPU1
>>> 	       ----                    ----
>>> 	  lock(&blkcg->lock);
>>> 	                               local_irq_disable();
>>> 	                               lock(&pool->lock#3);
>>> 	                               lock(&q->queue_lock);
>>> 	  <Interrupt>
>>> 	    lock(&pool->lock#3);
>>>
>>> Fix the issue by using spin_lock_irq(&blkcg->lock) in ioc_weight_write().
>>
>> This looks fine to me for blk-iocost, but block/blk-cgroup.c:blkg_create()
>> also looks like it gets the IRQ state of the same lock wrong?
> 
> blkg_create() is called with irq disabled in all three callers: 
> blkg_lookup_create(), blkg_conf_prep() and blkcg_init_queue().

Yes I know, see email sent 1 min after the one you're replying to.

-- 
Jens Axboe

Re: [PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[Tejun Heo]

On Tue, Aug 03, 2021 at 03:06:08PM +0800, Ming Lei wrote:
> blkcg->lock depends on q->queue_lock which may depend on another driver
> lock required in irq context, one example is dm-thin:
> 
> 	Chain exists of:
> 	  &pool->lock#3 --> &q->queue_lock --> &blkcg->lock
> 
> 	 Possible interrupt unsafe locking scenario:
> 
> 	       CPU0                    CPU1
> 	       ----                    ----
> 	  lock(&blkcg->lock);
> 	                               local_irq_disable();
> 	                               lock(&pool->lock#3);
> 	                               lock(&q->queue_lock);
> 	  <Interrupt>
> 	    lock(&pool->lock#3);
> 
> Fix the issue by using spin_lock_irq(&blkcg->lock) in ioc_weight_write().
> 
> Cc: Tejun Heo <tj@kernel.org>
> Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
> Link: https://lore.kernel.org/linux-block/CA+QYu4rzz6079ighEanS3Qq_Dmnczcf45ZoJoHKVLVATTo1e4Q@mail.gmail.com/T/#u
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] blk-iocost: fix lockdep warning on blkcg->lock

[Jens Axboe]

On 8/3/21 1:06 AM, Ming Lei wrote:
> blkcg->lock depends on q->queue_lock which may depend on another driver
> lock required in irq context, one example is dm-thin:
> 
> 	Chain exists of:
> 	  &pool->lock#3 --> &q->queue_lock --> &blkcg->lock
> 
> 	 Possible interrupt unsafe locking scenario:
> 
> 	       CPU0                    CPU1
> 	       ----                    ----
> 	  lock(&blkcg->lock);
> 	                               local_irq_disable();
> 	                               lock(&pool->lock#3);
> 	                               lock(&q->queue_lock);
> 	  <Interrupt>
> 	    lock(&pool->lock#3);
> 
> Fix the issue by using spin_lock_irq(&blkcg->lock) in ioc_weight_write().

Applied, thanks.

-- 
Jens Axboe