Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 14 Aug 2020 10:08:24 +0200
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: PCI: Race condition in pci_create_sysfs_dev_files
Message-ID: <20200814080824.dg57kmbimyf3ushe@pali>
References: <20200716110423.xtfyb3n6tn5ixedh@pali>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200716110423.xtfyb3n6tn5ixedh@pali>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello! I would like to remind this issue which I reported month ago.

On Thursday 16 July 2020 13:04:23 Pali Rohár wrote:
> Hello Bjorn!
> 
> I see following error message in dmesg which looks like a race condition:
> 
> sysfs: cannot create duplicate filename '/devices/platform/soc/d0070000.pcie/pci0000:00/0000:00:00.0/config'
> 
> I looked at it deeper and found out that in PCI subsystem code is race
> condition between pci_bus_add_device() and pci_sysfs_init() calls. Both
> of these functions calls pci_create_sysfs_dev_files() and calling this
> function more times for same pci device throws above error message.
> 
> There can be two different race conditions:
> 
> 1. pci_bus_add_device() called pcibios_bus_add_device() or
> pci_fixup_device() but have not called pci_create_sysfs_dev_files() yet.
> Meanwhile pci_sysfs_init() is running and pci_create_sysfs_dev_files()
> was called for newly registered device. In this case function
> pci_create_sysfs_dev_files() is called two times, ones from
> pci_bus_add_device() and once from pci_sysfs_init().
> 
> 2. pci_sysfs_init() is called. It first sets sysfs_initialized to 1
> which unblock calling pci_create_sysfs_dev_files(). Then another bus
> registers new PCI device and calls pci_bus_add_device() which calls
> pci_create_sysfs_dev_files() and registers sysfs files. Function
> pci_sysfs_init() continues execution and calls function
> pci_create_sysfs_dev_files() also for this newly registered device. So
> pci_create_sysfs_dev_files() is again called two times.
> 
> 
> I workaround both race conditions I created following hack patch. After
> applying it I'm not getting that 'sysfs: cannot create duplicate filename'
> error message anymore.
> 
> Can you look at it how to fix both race conditions in proper way?

Is this workaround diff enough? Or are you going to prepare something better?

Please let me know if I should send this diff as regular patch.

> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 8e40b3e6da77..691be2258c4e 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -316,7 +316,7 @@ void pci_bus_add_device(struct pci_dev *dev)
>  	 */
>  	pcibios_bus_add_device(dev);
>  	pci_fixup_device(pci_fixup_final, dev);
> -	pci_create_sysfs_dev_files(dev);
> +	pci_create_sysfs_dev_files(dev, false);
>  	pci_proc_attach_device(dev);
>  	pci_bridge_d3_update(dev);
>  
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 6d78df981d41..b0c4852a51dd 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1328,13 +1328,13 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  	return retval;
>  }
>  
> -int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
> +int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev, bool sysfs_initializing)
>  {
>  	int retval;
>  	int rom_size;
>  	struct bin_attribute *attr;
>  
> -	if (!sysfs_initialized)
> +	if (!sysfs_initializing && !sysfs_initialized)
>  		return -EACCES;
>  
>  	if (pdev->cfg_size > PCI_CFG_SPACE_SIZE)
> @@ -1437,18 +1437,21 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
>  static int __init pci_sysfs_init(void)
>  {
>  	struct pci_dev *pdev = NULL;
> -	int retval;
> +	int retval = 0;
>  
> -	sysfs_initialized = 1;
>  	for_each_pci_dev(pdev) {
> -		retval = pci_create_sysfs_dev_files(pdev);
> +		if (!pci_dev_is_added(pdev))
> +			continue;
> +		retval = pci_create_sysfs_dev_files(pdev, true);
>  		if (retval) {
>  			pci_dev_put(pdev);
> -			return retval;
> +			goto out;
>  		}
>  	}
>  
> -	return 0;
> +out:
> +	sysfs_initialized = 1;
> +	return retval;
>  }
>  late_initcall(pci_sysfs_init);
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6d3f75867106..304294c7171e 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -19,7 +19,7 @@ bool pcie_cap_has_rtctl(const struct pci_dev *dev);
>  
>  /* Functions internal to the PCI core code */
>  
> -int pci_create_sysfs_dev_files(struct pci_dev *pdev);
> +int pci_create_sysfs_dev_files(struct pci_dev *pdev, bool sysfs_initializing);
>  void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
>  #if !defined(CONFIG_DMI) && !defined(CONFIG_ACPI)
>  static inline void pci_create_firmware_label_files(struct pci_dev *pdev)
>