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: <Y2gHqj18Tz66k4ZN@unreal>
Date:   Sun, 6 Nov 2022 21:14:50 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Stefan Assmann <sassmann@...nic.de>
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 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).

In addition, mutex_is_locked() doesn't check that this specific function
is locked. It checks that this lock is used now.

> +		dev_dbg(&adapter->pdev->dev, "%s:%d state transition %s to %s\n",
> +			func, line,
> +			iavf_state_str(adapter->last_state),
> +			iavf_state_str(adapter->state));
> +	else
> +		dev_warn(&adapter->pdev->dev, "%s:%d state transition %s to %s without locking!\n",
> +			 func, line,
> +			 iavf_state_str(adapter->last_state),
> +			 iavf_state_str(adapter->state));
>  }
>  
> +#define iavf_change_state(adapter, state) \
> +	__iavf_change_state(adapter, state, __func__, __LINE__)
> +
>  int iavf_up(struct iavf_adapter *adapter);
>  void iavf_down(struct iavf_adapter *adapter);
>  int iavf_process_config(struct iavf_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 3fc572341781..bbc0c9f347a7 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -4892,7 +4892,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	hw->back = adapter;
>  
>  	adapter->msg_enable = BIT(DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
> -	iavf_change_state(adapter, __IAVF_STARTUP);
> +	adapter->state = __IAVF_STARTUP;
>  
>  	/* Call save state here because it relies on the adapter struct. */
>  	pci_save_state(pdev);
> -- 
> 2.37.3
>