netdev - Re: [PATCH net-next 3/3] mlxsw: pci: Lock configuration space of upstream bridge during reset

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 for Android: free password hash cracker in your pocket

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

Date: Tue, 2 Jul 2024 09:35:50 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Petr Machata <petrm@...dia.com>, Ido Schimmel <idosch@...dia.com>
CC: <mlxsw@...dia.com>, <linux-pci@...r.kernel.org>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 3/3] mlxsw: pci: Lock configuration space of
 upstream bridge during reset

On 7/1/24 18:41, Petr Machata wrote:
> From: Ido Schimmel <idosch@...dia.com>
> 
> The driver triggers a "Secondary Bus Reset" (SBR) by calling
> __pci_reset_function_locked() which asserts the SBR bit in the "Bridge
> Control Register" in the configuration space of the upstream bridge for
> 2ms. This is done without locking the configuration space of the
> upstream bridge port, allowing user space to access it concurrently.

This means your patch is a bugfix.

> 
> Linux 6.11 will start warning about such unlocked resets [1][2]:
> 
> pcieport 0000:00:01.0: unlocked secondary bus reset via: pci_reset_bus_function+0x51c/0x6a0
> 
> Avoid the warning by locking the configuration space of the upstream
> bridge prior to the reset and unlocking it afterwards.

You are not avoiding the warning but protecting concurrent access,
please add a Fixes tag.

> 
> [1] https://lore.kernel.org/all/171711746953.1628941.4692125082286867825.stgit@dwillia2-xfh.jf.intel.com/
> [2] https://lore.kernel.org/all/20240531213150.GA610983@bhelgaas/
> 
> Cc: linux-pci@...r.kernel.org
> Signed-off-by: Ido Schimmel <idosch@...dia.com>
> Signed-off-by: Petr Machata <petrm@...dia.com>
> ---
>   drivers/net/ethernet/mellanox/mlxsw/pci.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> index 0320dabd1380..060e5b939211 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> @@ -1784,6 +1784,7 @@ static int mlxsw_pci_reset_at_pci_disable(struct mlxsw_pci *mlxsw_pci,
>   {
>   	struct pci_dev *pdev = mlxsw_pci->pdev;
>   	char mrsr_pl[MLXSW_REG_MRSR_LEN];
> +	struct pci_dev *bridge;
>   	int err;
>   
>   	if (!pci_reset_sbr_supported) {
> @@ -1800,6 +1801,9 @@ static int mlxsw_pci_reset_at_pci_disable(struct mlxsw_pci *mlxsw_pci,
>   sbr:
>   	device_lock_assert(&pdev->dev);
>   
> +	bridge = pci_upstream_bridge(pdev);
> +	if (bridge)
> +		pci_cfg_access_lock(bridge);
>   	pci_cfg_access_lock(pdev);
>   	pci_save_state(pdev);
>   
> @@ -1809,6 +1813,8 @@ static int mlxsw_pci_reset_at_pci_disable(struct mlxsw_pci *mlxsw_pci,
>   
>   	pci_restore_state(pdev);
>   	pci_cfg_access_unlock(pdev);
> +	if (bridge)
> +		pci_cfg_access_unlock(bridge);
>   
>   	return err;
>   }