netdev - Re: [PATCH net-next] iavf: check that state transitions happen under lock

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: <5911b8f9-590b-6e05-646a-c1bc597105d8@kpanic.de>
Date:   Mon, 7 Nov 2022 12:57:42 +0100
From:   Stefan Assmann <sassmann@...nic.de>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        anthony.l.nguyen@...el.com, patryk.piotrowski@...el.com
Subject: Re: [PATCH net-next] iavf: check that state transitions happen under
 lock

On 06.11.22 20:14, Leon Romanovsky wrote:

> On Fri, Oct 28, 2022 at 03:45:15PM +0200, Stefan Assmann wrote:

>> Add a check to make sure crit_lock is being held during every state

>> transition and print a warning if that's not the case. For convenience

>> a wrapper is added that helps pointing out where the locking is missing.

>>

>> Make an exception for iavf_probe() as that is too early in the init

>> process and generates a false positive report.

>>

>> Signed-off-by: Stefan Assmann <sassmann@...nic.de>

>> ---

>>  drivers/net/ethernet/intel/iavf/iavf.h      | 23 +++++++++++++++------

>>  drivers/net/ethernet/intel/iavf/iavf_main.c |  2 +-

>>  2 files changed, 18 insertions(+), 7 deletions(-)

>>

>> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h

>> index 3f6187c16424..28f41bbc9c86 100644

>> --- a/drivers/net/ethernet/intel/iavf/iavf.h

>> +++ b/drivers/net/ethernet/intel/iavf/iavf.h

>> @@ -498,19 +498,30 @@ static inline const char *iavf_state_str(enum iavf_state_t state)

>>  	}

>>  }

>>  

>> -static inline void iavf_change_state(struct iavf_adapter *adapter,

>> -				     enum iavf_state_t state)

>> +static inline void __iavf_change_state(struct iavf_adapter *adapter,

>> +				       enum iavf_state_t state,

>> +				       const char *func,

>> +				       int line)

>>  {

>>  	if (adapter->state != state) {

>>  		adapter->last_state = adapter->state;

>>  		adapter->state = state;

>>  	}

>> -	dev_dbg(&adapter->pdev->dev,

>> -		"state transition from:%s to:%s\n",

>> -		iavf_state_str(adapter->last_state),

>> -		iavf_state_str(adapter->state));

>> +	if (mutex_is_locked(&adapter->crit_lock))

> 

> Please use lockdep for that, and not reinvent it.

> In you case lockdep_assert_held(&adapter->crit_lock).



Lockdep is mostly enabled in debug kernel but I was hoping to get

warnings in production environments as well. As those transitions don't

happen too often it shouldn't hurt performance.



> In addition, mutex_is_locked() doesn't check that this specific function

> is locked. It checks that this lock is used now.



You are correct, this check only triggers if crit_lock is not locked at

all. It would be better to check the lock owner, but I couldn't find an

easy way to do that. Better than no check IMO but we can drop it if you

don't see a benefit in it.



Thanks for the comments!



  Stefan