linux-kernel - Re: [PATCH v2] blk_iocost: remove some duplicate irq disable/enables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <df1cc7cb-bac6-4ec2-b148-0260654cc59a@redhat.com>
Date: Wed, 2 Oct 2024 14:40:52 -0400
From: Waiman Long <longman@...hat.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Yu Kuai <yukuai3@...wei.com>, Tejun Heo <tj@...nel.org>,
 Josef Bacik <josef@...icpanda.com>, Jens Axboe <axboe@...nel.dk>,
 cgroups@...r.kernel.org, linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
 Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v2] blk_iocost: remove some duplicate irq disable/enables


On 10/2/24 14:10, Dan Carpenter wrote:
> On Wed, Oct 02, 2024 at 01:49:48PM -0400, Waiman Long wrote:
>>> -	spin_unlock_irq(&ioc->lock);
>>> +	spin_unlock(&ioc->lock);
>>>    	return 0;
>>>    }
>> I would suggest adding a "lockdep_assert_irqs_disabled()" call before
>> spin_lock() to confirm that irq is indeed disabled just in case the callers
>> are changed in the future.
> It's really hard to predict future bugs.  I doubt we'll add new callers.
> Outputting this information to a struct seq_file *sf is pretty specific.
>
> If there were a bug related to this, then wouldn't it be caught by lockdep?
>
> The other idea is that we could catch bugs like this using static analysis.
> Like every time we take the &ioc->lock, either IRQs should already be disabled
> or we disable it ourselves.  I could write a Smatch check like this.
>
> KTODO: add Smatch check to ensure IRQs are disabled for &ioc->lock

This is just a suggestion and it is fine if you don't think it is 
necessary. The call can also serve as a comment that irq should have 
been disabled at this point.

Cheers,
Longman