[PATCH 0/3] PCI: Fix race condition upon sysfs init

[PATCH 0/3] PCI: Fix race condition upon sysfs init

[Alexander Stein]

Hi everyone,

this series is a totally different approach for fixing the sysfs init race
condition. The initial problem is stated at [1]. Previous proposals were
rejected ([2] and [3]). Here is what's happening


        CPU 0                                  CPU 1

                                        imx6_pcie_probe()
                                        dw_pcie_host_init()
                                        pci_host_probe()
                                        pci_scan_root_bus_bridge()
                                          pci_scan_child_bus_extend()
                                          pci_scan_slot()
                                          pci_scan_single_device()
                                          pci_device_add()
pci_sysfs_init()                          device_add()
  sysfs_initialized = 1;                  bus_add_device()
  for_each_pci_dev()                          ...
    pci_create_sysfs_dev_files()                  
                                        pci_bus_add_devices()
                                        pci_bus_add_device()
                                        pci_create_sysfs_dev_files()

Eventually calling pci_create_sysfs_dev_files() twice on the same pci_dev.
It's a very tight window, deeper PCIe trees increase that window during
host probe. Asynchronous PCIe host probe is a necessity
(PROBE_PREFER_ASYNCHRONOUS).

The first two patches are preparations for the last one actually fixing
the race. As functions like pci_create_sysfs_dev_files() are called from
externtal and internal to pci-sysfs, an internal version without checking
for sysfs_initialized is required.
For the fix a wait queue is introduced where all callers from external
callsites (regarding pci-sysfs.c) are waiting until pci_sysfs_init
initcall has finished and woken up all waiters.

A subtlety is that within __pci_create_sysfs_dev_files the resource files
(created by pci_sysfs_init) need to be removed, so they can be created
again from pci_host_probe call.

Best regards,
Alexander

Links:
[1] https://bugzilla.kernel.org/show_bug.cgi?id=215515
[2] https://lore.kernel.org/linux-pci/20230316091540.494366-1-alexander.stein@ew.tq-group.com/
[3] https://lore.kernel.org/linux-pci/20230316103036.1837869-1-alexander.stein@ew.tq-group.com/

Alexander Stein (3):
  PCI/sysfs: sort headers alphabetically
  PCI/sysfs: create private functions for
    pci_create_legacy_files/pci_create_sysfs_dev_files
  PCI/sysfs: Fix sysfs init race condition

 drivers/pci/pci-sysfs.c | 87 +++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 34 deletions(-)

-- 
2.34.1

[PATCH 1/3] PCI/sysfs: sort headers alphabetically

[Alexander Stein]

The includes were not sorted alphabetically. Sorts them properly.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/pci/pci-sysfs.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index ab32a91f287b..289c1c17b41f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -13,22 +13,22 @@
  */
 
 
-#include <linux/kernel.h>
-#include <linux/sched.h>
-#include <linux/pci.h>
-#include <linux/stat.h>
+#include <linux/aperture.h>
+#include <linux/capability.h>
 #include <linux/export.h>
-#include <linux/topology.h>
-#include <linux/mm.h>
 #include <linux/fs.h>
-#include <linux/capability.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/msi.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/sched.h>
 #include <linux/security.h>
 #include <linux/slab.h>
+#include <linux/stat.h>
+#include <linux/topology.h>
 #include <linux/vgaarb.h>
-#include <linux/pm_runtime.h>
-#include <linux/msi.h>
-#include <linux/of.h>
-#include <linux/aperture.h>
 #include "pci.h"
 
 static int sysfs_initialized;	/* = 0 */
-- 
2.34.1

[PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files

[Alexander Stein]

The only difference is they don't have the check against sysfs_initialized.
This is a preparation for the sysfs init race condition fix.
No functional change intended.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/pci/pci-sysfs.c | 44 ++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 289c1c17b41f..7d4733773633 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -938,24 +938,10 @@ void __weak pci_adjust_legacy_attr(struct pci_bus *b,
 {
 }
 
-/**
- * pci_create_legacy_files - create legacy I/O port and memory files
- * @b: bus to create files under
- *
- * Some platforms allow access to legacy I/O port and ISA memory space on
- * a per-bus basis.  This routine creates the files and ties them into
- * their associated read, write and mmap files from pci-sysfs.c
- *
- * On error unwind, but don't propagate the error to the caller
- * as it is ok to set up the PCI bus without these files.
- */
-void pci_create_legacy_files(struct pci_bus *b)
+static void __pci_create_legacy_files(struct pci_bus *b)
 {
 	int error;
 
-	if (!sysfs_initialized)
-		return;
-
 	b->legacy_io = kcalloc(2, sizeof(struct bin_attribute),
 			       GFP_ATOMIC);
 	if (!b->legacy_io)
@@ -998,6 +984,25 @@ void pci_create_legacy_files(struct pci_bus *b)
 	dev_warn(&b->dev, "could not create legacy I/O port and ISA memory resources in sysfs\n");
 }
 
+/**
+ * pci_create_legacy_files - create legacy I/O port and memory files
+ * @b: bus to create files under
+ *
+ * Some platforms allow access to legacy I/O port and ISA memory space on
+ * a per-bus basis.  This routine creates the files and ties them into
+ * their associated read, write and mmap files from pci-sysfs.c
+ *
+ * On error unwind, but don't propagate the error to the caller
+ * as it is ok to set up the PCI bus without these files.
+ */
+void pci_create_legacy_files(struct pci_bus *b)
+{
+	if (!sysfs_initialized)
+		return;
+
+	__pci_create_legacy_files(b);
+}
+
 void pci_remove_legacy_files(struct pci_bus *b)
 {
 	if (b->legacy_io) {
@@ -1006,6 +1011,8 @@ void pci_remove_legacy_files(struct pci_bus *b)
 		kfree(b->legacy_io); /* both are allocated here */
 	}
 }
+#else
+static void __pci_create_legacy_files(struct pci_bus *b) {}
 #endif /* HAVE_PCI_LEGACY */
 
 #if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
@@ -1492,12 +1499,17 @@ static const struct attribute_group pci_dev_resource_resize_group = {
 	.is_visible = resource_resize_is_visible,
 };
 
+int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
+{
+	return pci_create_resource_files(pdev);
+}
+
 int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
 {
 	if (!sysfs_initialized)
 		return -EACCES;
 
-	return pci_create_resource_files(pdev);
+	return __pci_create_sysfs_dev_files(pdev);
 }
 
 /**
-- 
2.34.1

[PATCH 3/3] PCI/sysfs: Fix sysfs init race condition

[Alexander Stein]

sysfs attribute files for PCIe devices (pci_create_sysfs_dev_files) can be
created by two paths:
1. pci_sysfs_init()
2. pci_bus_add_device() (drivers/pci/bus.c)

There is a race during startup where an asynchronous PCIe host probe races
against the pci_sysfs_init() late_initcall. In this case the PCIe devices
are already added to the bus, for_each_pci_dev() will see them, but
pci_bus_add_device() has not yet finished, so both code paths try to add
the sysfs attributes.

Fix this by waiting on a workqueue until sysfs has been initialized.
pci_sysfs_init() needs the internal function without the check that
sysfs_initialized has been set to 1.
__pci_create_sysfs_dev_files still needs to remove resource files,
which might have been created during pci_sysfs_init initcall.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/pci/pci-sysfs.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 7d4733773633..3067d55f981c 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -29,9 +29,11 @@
 #include <linux/stat.h>
 #include <linux/topology.h>
 #include <linux/vgaarb.h>
+#include <linux/wait.h>
 #include "pci.h"
 
 static int sysfs_initialized;	/* = 0 */
+static DECLARE_WAIT_QUEUE_HEAD(sysfs_wq);
 
 /* show configuration fields */
 #define pci_config_attr(field, format_string)				\
@@ -997,8 +999,7 @@ static void __pci_create_legacy_files(struct pci_bus *b)
  */
 void pci_create_legacy_files(struct pci_bus *b)
 {
-	if (!sysfs_initialized)
-		return;
+	wait_event(sysfs_wq, sysfs_initialized);
 
 	__pci_create_legacy_files(b);
 }
@@ -1501,13 +1502,18 @@ static const struct attribute_group pci_dev_resource_resize_group = {
 
 int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
 {
+	/*
+	 * sysfs attributes might already be created by pci_sysfs_init(),
+	 * delete them here just in case
+	 */
+	pci_remove_resource_files(pdev);
 	return pci_create_resource_files(pdev);
 }
 
 int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
 {
-	if (!sysfs_initialized)
-		return -EACCES;
+	/* Wait until sysfs has been initialized */
+	wait_event(sysfs_wq, sysfs_initialized);
 
 	return __pci_create_sysfs_dev_files(pdev);
 }
@@ -1520,8 +1526,8 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
  */
 void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
 {
-	if (!sysfs_initialized)
-		return;
+	/* Wait until sysfs has been initialized */
+	wait_event(sysfs_wq, sysfs_initialized);
 
 	pci_remove_resource_files(pdev);
 }
@@ -1532,9 +1538,8 @@ static int __init pci_sysfs_init(void)
 	struct pci_bus *pbus = NULL;
 	int retval;
 
-	sysfs_initialized = 1;
 	for_each_pci_dev(pdev) {
-		retval = pci_create_sysfs_dev_files(pdev);
+		retval = __pci_create_sysfs_dev_files(pdev);
 		if (retval) {
 			pci_dev_put(pdev);
 			return retval;
@@ -1542,7 +1547,9 @@ static int __init pci_sysfs_init(void)
 	}
 
 	while ((pbus = pci_find_next_bus(pbus)))
-		pci_create_legacy_files(pbus);
+		__pci_create_legacy_files(pbus);
+	sysfs_initialized = 1;
+	wake_up_all(&sysfs_wq);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 0/3] PCI: Fix race condition upon sysfs init

[Bjorn Helgaas]

On Thu, Apr 27, 2023 at 04:28:58PM +0200, Alexander Stein wrote:
> Hi everyone,
> 
> this series is a totally different approach for fixing the sysfs init race
> condition. The initial problem is stated at [1]. Previous proposals were
> rejected ([2] and [3]). Here is what's happening
> 
> 
>         CPU 0                                  CPU 1
> 
>                                         imx6_pcie_probe()
>                                         dw_pcie_host_init()
>                                         pci_host_probe()
>                                         pci_scan_root_bus_bridge()
>                                           pci_scan_child_bus_extend()
>                                           pci_scan_slot()
>                                           pci_scan_single_device()
>                                           pci_device_add()
> pci_sysfs_init()                          device_add()
>   sysfs_initialized = 1;                  bus_add_device()
>   for_each_pci_dev()                          ...
>     pci_create_sysfs_dev_files()                  
>                                         pci_bus_add_devices()
>                                         pci_bus_add_device()
>                                         pci_create_sysfs_dev_files()
> 
> Eventually calling pci_create_sysfs_dev_files() twice on the same pci_dev.
> It's a very tight window, deeper PCIe trees increase that window during
> host probe. Asynchronous PCIe host probe is a necessity
> (PROBE_PREFER_ASYNCHRONOUS).
> 
> The first two patches are preparations for the last one actually fixing
> the race. As functions like pci_create_sysfs_dev_files() are called from
> externtal and internal to pci-sysfs, an internal version without checking
> for sysfs_initialized is required.
> For the fix a wait queue is introduced where all callers from external
> callsites (regarding pci-sysfs.c) are waiting until pci_sysfs_init
> initcall has finished and woken up all waiters.
> 
> A subtlety is that within __pci_create_sysfs_dev_files the resource files
> (created by pci_sysfs_init) need to be removed, so they can be created
> again from pci_host_probe call.

I'll look at this in more detail, but if there's any way at all that
we could get rid of pci_sysfs_init() completely and do this with
static attributes or some other existing sysfs infrastructure, I would
STRONGLY prefer it because that infrastructure has already solved this
problem.

Maybe that's impossible and we really need to make a one-off solution
just for PCI, but ... I haven't been convinced yet.

> Links:
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=215515
> [2] https://lore.kernel.org/linux-pci/20230316091540.494366-1-alexander.stein@ew.tq-group.com/
> [3] https://lore.kernel.org/linux-pci/20230316103036.1837869-1-alexander.stein@ew.tq-group.com/
> 
> Alexander Stein (3):
>   PCI/sysfs: sort headers alphabetically
>   PCI/sysfs: create private functions for
>     pci_create_legacy_files/pci_create_sysfs_dev_files
>   PCI/sysfs: Fix sysfs init race condition
> 
>  drivers/pci/pci-sysfs.c | 87 +++++++++++++++++++++++++----------------
>  1 file changed, 53 insertions(+), 34 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files

[kernel test robot]

Hi Alexander,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.3 next-20230427]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20230427142901.3570536-3-alexander.stein%40ew.tq-group.com
patch subject: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230428/202304280450.17v5JK13-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/cbc778730116b49890b5c41ffc9fc664b566e3c4
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
        git checkout cbc778730116b49890b5c41ffc9fc664b566e3c4
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/pci/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304280450.17v5JK13-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pci/pci-sysfs.c:1256:12: warning: no previous prototype for 'pci_create_resource_files' [-Wmissing-prototypes]
    1256 | int __weak pci_create_resource_files(struct pci_dev *dev) { return 0; }
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/pci-sysfs.c:1257:13: warning: no previous prototype for 'pci_remove_resource_files' [-Wmissing-prototypes]
    1257 | void __weak pci_remove_resource_files(struct pci_dev *dev) { return; }
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/pci-sysfs.c:1502:18: warning: no previous prototype for '__pci_create_sysfs_dev_files' [-Wmissing-prototypes]
    1502 | int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
         |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/pci-sysfs.c:1015:13: warning: '__pci_create_legacy_files' defined but not used [-Wunused-function]
    1015 | static void __pci_create_legacy_files(struct pci_bus *b) {}
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/__pci_create_sysfs_dev_files +1502 drivers/pci/pci-sysfs.c

  1501	
> 1502	int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
  1503	{
  1504		return pci_create_resource_files(pdev);
  1505	}
  1506	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files

[kernel test robot]

Hi Alexander,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.3 next-20230505]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20230427142901.3570536-3-alexander.stein%40ew.tq-group.com
patch subject: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files
config: mips-maltaup_defconfig (https://download.01.org/0day-ci/archive/20230507/202305071649.zjynmg3E-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mipsel-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/cbc778730116b49890b5c41ffc9fc664b566e3c4
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
        git checkout cbc778730116b49890b5c41ffc9fc664b566e3c4
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/pci/ mm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305071649.zjynmg3E-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pci/pci-sysfs.c:1502:18: warning: no previous prototype for function '__pci_create_sysfs_dev_files' [-Wmissing-prototypes]
   int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
                    ^
   drivers/pci/pci-sysfs.c:1502:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
   ^
   static 
   drivers/pci/pci-sysfs.c:1015:13: warning: unused function '__pci_create_legacy_files' [-Wunused-function]
   static void __pci_create_legacy_files(struct pci_bus *b) {}
               ^
   2 warnings generated.


vim +/__pci_create_sysfs_dev_files +1502 drivers/pci/pci-sysfs.c

  1501	
> 1502	int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
  1503	{
  1504		return pci_create_resource_files(pdev);
  1505	}
  1506	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests