[PATCH bisected regression in 4.14] PCI: fix race while enabling upstream bridges concurrently

[PATCH bisected regression in 4.14] PCI: fix race while enabling upstream bridges concurrently

[Konstantin Khlebnikov]

In pci_enable_bridge() pci_enable_device() is called before calling
pci_set_master(), thus check pci_is_enabled() becomes true in the
middle of this sequence. As a result in pci_enable_device_flags()
concurrent enable of device on same bridge could think that this
bridge is completely enabled, but actually it's not yet.

For me this race broke ethernet devices after booting kernel via
kexec, normal reboot was fine.

This patch removes racy fast-path: pci_enable_bridge() will take
pci_bridge_mutex and do nothing if bridge is already enabled.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Fixes: 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges")
---
 drivers/pci/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b0002daa50f3..ffbe11dbdd61 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
 		return 0;		/* already enabled */
 
 	bridge = pci_upstream_bridge(dev);
-	if (bridge && !pci_is_enabled(bridge))
+	if (bridge)
 		pci_enable_bridge(bridge);
 
 	/* only skip sriov related */

Re: [PATCH bisected regression in 4.14] PCI: fix race while enabling upstream bridges concurrently

[Srinath Mannam]

Hi Konstantin,

On Fri, Sep 15, 2017 at 6:18 PM, Konstantin Khlebnikov
<khlebnikov@yandex-team.ru> wrote:
> In pci_enable_bridge() pci_enable_device() is called before calling
> pci_set_master(), thus check pci_is_enabled() becomes true in the
> middle of this sequence. As a result in pci_enable_device_flags()
> concurrent enable of device on same bridge could think that this
> bridge is completely enabled, but actually it's not yet.
>
> For me this race broke ethernet devices after booting kernel via
> kexec, normal reboot was fine.
>
> This patch removes racy fast-path: pci_enable_bridge() will take
> pci_bridge_mutex and do nothing if bridge is already enabled.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Fixes: 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges")
> ---
>  drivers/pci/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b0002daa50f3..ffbe11dbdd61 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>                 return 0;               /* already enabled */
>
>         bridge = pci_upstream_bridge(dev);
> -       if (bridge && !pci_is_enabled(bridge))
> +       if (bridge)
This patch causes deadlock because of nexted mutex lock.
As per original code, Bridge enable function is called equal to number
of child bridges it has.
In the case endpoint is connected to RC through two bridges.
bridge 2 is enabled(both device and bus master) first.
While bridge1 enable, it calls device enable which calls device_enable_flags.
set device enable flag
check it has bridge (here yes because it has bridge2)
calls bridge enable for bridge2. which is already enabled.

So in my patch we introduced mutex to stop the race condition.
By taking this mutex, we see dead lock in the second call for bridge
enable (ex: bridge2)
Here we stopped second time calling of bridge enable using "if (bridge
&& !pci_is_enabled(bridge))"
In this case, there will not be such scenario where device enable and
bus master is missed in bridge enable function.
Because pci_is_enabled check in "if (bridge &&
!pci_is_enabled(bridge))" will check for its bridge not itself.
Stopping its bridge is not a problem because it is already enabled, as
I explained above.

Please explain your case where bus master could missed for bridge. It
helps me to understand more about how various bridges enabled.

>                 pci_enable_bridge(bridge);
>
>         /* only skip sriov related */
>
Regards,
Srinath.

Re: [PATCH bisected regression in 4.14] PCI: fix race while enabling upstream bridges concurrently

[Konstantin Khlebnikov]

On 15.09.2017 16:43, Srinath Mannam wrote:
> Hi Konstantin,
> 
> On Fri, Sep 15, 2017 at 6:18 PM, Konstantin Khlebnikov
> <khlebnikov@yandex-team.ru> wrote:
>> In pci_enable_bridge() pci_enable_device() is called before calling
>> pci_set_master(), thus check pci_is_enabled() becomes true in the
>> middle of this sequence. As a result in pci_enable_device_flags()
>> concurrent enable of device on same bridge could think that this
>> bridge is completely enabled, but actually it's not yet.
>>
>> For me this race broke ethernet devices after booting kernel via
>> kexec, normal reboot was fine.
>>
>> This patch removes racy fast-path: pci_enable_bridge() will take
>> pci_bridge_mutex and do nothing if bridge is already enabled.
>>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> Fixes: 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges")
>> ---
>>   drivers/pci/pci.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b0002daa50f3..ffbe11dbdd61 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>                  return 0;               /* already enabled */
>>
>>          bridge = pci_upstream_bridge(dev);
>> -       if (bridge && !pci_is_enabled(bridge))
>> +       if (bridge)

> This patch causes deadlock because of nexted mutex lock.

Oh, yes.

I suppose not ascending to upstream bridge for bridges from pci_enable_device_flags should fix this.

Something like this:


--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1393,9 +1393,11 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
         if (atomic_inc_return(&dev->enable_cnt) > 1)
                 return 0;               /* already enabled */

-       bridge = pci_upstream_bridge(dev);
-       if (bridge)
-               pci_enable_bridge(bridge);
+       if (!pci_is_bridge(dev)) {
+               bridge = pci_upstream_bridge(dev);
+               if (bridge)
+                       pci_enable_bridge(bridge);
+       }

         /* only skip sriov related */
         for (i = 0; i <= PCI_ROM_RESOURCE; i++)

> As per original code, Bridge enable function is called equal to number
> of child bridges it has.
> In the case endpoint is connected to RC through two bridges.
> bridge 2 is enabled(both device and bus master) first.
> While bridge1 enable, it calls device enable which calls device_enable_flags.
> set device enable flag
> check it has bridge (here yes because it has bridge2)
> calls bridge enable for bridge2. which is already enabled.
> 
> So in my patch we introduced mutex to stop the race condition.
> By taking this mutex, we see dead lock in the second call for bridge
> enable (ex: bridge2)
> Here we stopped second time calling of bridge enable using "if (bridge
> && !pci_is_enabled(bridge))"
> In this case, there will not be such scenario where device enable and
> bus master is missed in bridge enable function.
> Because pci_is_enabled check in "if (bridge &&
> !pci_is_enabled(bridge))" will check for its bridge not itself.
> Stopping its bridge is not a problem because it is already enabled, as
> I explained above.
> 
> Please explain your case where bus master could missed for bridge. It
> helps me to understand more about how various bridges enabled.

Pure race: seocnd deivice could call do_pci_enable_device() while
first still enabling bridge and haven't set bus master for it:


CPU1                                          CPU2

pci_enable_device_flags(dev1)                 pci_enable_device_flags(dev2)

pci_is_enabled(bridge)

atomic_read(&bridge->enable_cnt) -> 0

pci_upstream_bridge(bridge)

mutex_lock(&pci_bridge_mutex);

pci_enable_device(bridge)

atomic_inc_return(&bridge->enable_cnt) 0 -> 1

                                               pci_is_enabled(bridge)

                                               atomic_read(&bridge->enable_cnt) -> 1

                                               do_pci_enable_device(dev2) -> fail,

pci_set_master(bridge);

mutex_unlock(&pci_bridge_mutex);

do_pci_enable_device(dev1)


> 
>>                  pci_enable_bridge(bridge);
>>
>>          /* only skip sriov related */
>>
> Regards,
> Srinath.
>