[PATCH] igb: Use a sperate mutex insead of rtnl_lock()

[PATCH] igb: Use a sperate mutex insead of rtnl_lock()

[Kai-Heng Feng]

Commit 9474933caf21 ("igb: close/suspend race in netif_device_detach")
fixed race condition between close and power management ops by using
rtnl_lock().

This fix is a preparation for next patch, to prevent a dead lock under
rtnl_lock() when calling runtime resume routine.

However, we can't use device_lock() in igb_close() because when module
is getting removed, the lock is already held for igb_remove(), and
igb_close() gets called during unregistering the netdev, hence causing a
deadlock. So let's introduce a new mutex so we don't cause a deadlock
with driver core or netdev core.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b46bff8fe056..dc7ed5dd216b 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -288,6 +288,8 @@ static const struct igb_reg_info igb_reg_info_tbl[] = {
 	{}
 };
 
+static DEFINE_MUTEX(igb_mutex);
+
 /* igb_regdump - register printout routine */
 static void igb_regdump(struct e1000_hw *hw, struct igb_reg_info *reginfo)
 {
@@ -4026,9 +4028,14 @@ static int __igb_close(struct net_device *netdev, bool suspending)
 
 int igb_close(struct net_device *netdev)
 {
+	int err = 0;
+
+	mutex_lock(&igb_mutex);
 	if (netif_device_present(netdev) || netdev->dismantle)
-		return __igb_close(netdev, false);
-	return 0;
+		err = __igb_close(netdev, false);
+	mutex_unlock(&igb_mutex);
+
+	return err;
 }
 
 /**
@@ -8760,7 +8767,7 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
 	u32 wufc = runtime ? E1000_WUFC_LNKC : adapter->wol;
 	bool wake;
 
-	rtnl_lock();
+	mutex_lock(&igb_mutex);
 	netif_device_detach(netdev);
 
 	if (netif_running(netdev))
@@ -8769,7 +8776,7 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
 	igb_ptp_suspend(adapter);
 
 	igb_clear_interrupt_scheme(adapter);
-	rtnl_unlock();
+	mutex_unlock(&igb_mutex);
 
 	status = rd32(E1000_STATUS);
 	if (status & E1000_STATUS_LU)
@@ -8897,13 +8904,13 @@ static int __maybe_unused igb_resume(struct device *dev)
 
 	wr32(E1000_WUS, ~0);
 
-	rtnl_lock();
+	mutex_lock(&igb_mutex);
 	if (!err && netif_running(netdev))
 		err = __igb_open(netdev, true);
 
 	if (!err)
 		netif_device_attach(netdev);
-	rtnl_unlock();
+	mutex_unlock(&igb_mutex);
 
 	return err;
 }
-- 
2.17.1

Re: [PATCH] igb: Use a sperate mutex insead of rtnl_lock()

[Kai-Heng Feng]

Hi,

> On Mar 26, 2020, at 18:39, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> Commit 9474933caf21 ("igb: close/suspend race in netif_device_detach")
> fixed race condition between close and power management ops by using
> rtnl_lock().
> 
> This fix is a preparation for next patch, to prevent a dead lock under
> rtnl_lock() when calling runtime resume routine.
> 
> However, we can't use device_lock() in igb_close() because when module
> is getting removed, the lock is already held for igb_remove(), and
> igb_close() gets called during unregistering the netdev, hence causing a
> deadlock. So let's introduce a new mutex so we don't cause a deadlock
> with driver core or netdev core.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Please drop "igb: Use device_lock() insead of rtnl_lock()" and use this one instead.
Thanks!

Kai-Heng

> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index b46bff8fe056..dc7ed5dd216b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -288,6 +288,8 @@ static const struct igb_reg_info igb_reg_info_tbl[] = {
> 	{}
> };
> 
> +static DEFINE_MUTEX(igb_mutex);
> +
> /* igb_regdump - register printout routine */
> static void igb_regdump(struct e1000_hw *hw, struct igb_reg_info *reginfo)
> {
> @@ -4026,9 +4028,14 @@ static int __igb_close(struct net_device *netdev, bool suspending)
> 
> int igb_close(struct net_device *netdev)
> {
> +	int err = 0;
> +
> +	mutex_lock(&igb_mutex);
> 	if (netif_device_present(netdev) || netdev->dismantle)
> -		return __igb_close(netdev, false);
> -	return 0;
> +		err = __igb_close(netdev, false);
> +	mutex_unlock(&igb_mutex);
> +
> +	return err;
> }
> 
> /**
> @@ -8760,7 +8767,7 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
> 	u32 wufc = runtime ? E1000_WUFC_LNKC : adapter->wol;
> 	bool wake;
> 
> -	rtnl_lock();
> +	mutex_lock(&igb_mutex);
> 	netif_device_detach(netdev);
> 
> 	if (netif_running(netdev))
> @@ -8769,7 +8776,7 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
> 	igb_ptp_suspend(adapter);
> 
> 	igb_clear_interrupt_scheme(adapter);
> -	rtnl_unlock();
> +	mutex_unlock(&igb_mutex);
> 
> 	status = rd32(E1000_STATUS);
> 	if (status & E1000_STATUS_LU)
> @@ -8897,13 +8904,13 @@ static int __maybe_unused igb_resume(struct device *dev)
> 
> 	wr32(E1000_WUS, ~0);
> 
> -	rtnl_lock();
> +	mutex_lock(&igb_mutex);
> 	if (!err && netif_running(netdev))
> 		err = __igb_open(netdev, true);
> 
> 	if (!err)
> 		netif_device_attach(netdev);
> -	rtnl_unlock();
> +	mutex_unlock(&igb_mutex);
> 
> 	return err;
> }
> -- 
> 2.17.1
> 

Re: [Intel-wired-lan] [PATCH] igb: Use a sperate mutex insead of rtnl_lock()

[Paul Menzel]

Dear Kai-Heng,


Thank you.

There is a small typo in the commit summary: s*epa*rate.

Am 26.03.20 um 11:39 schrieb Kai-Heng Feng:
> Commit 9474933caf21 ("igb: close/suspend race in netif_device_detach")
> fixed race condition between close and power management ops by using
> rtnl_lock().
> 
> This fix is a preparation for next patch, to prevent a dead lock under
> rtnl_lock() when calling runtime resume routine.

Do you refer with *this fix* to the referenced commit? Or do you mean 
the patch you just sent?

How can the issue be reproduced?

> However, we can't use device_lock() in igb_close() because when module
> is getting removed, the lock is already held for igb_remove(), and
> igb_close() gets called during unregistering the netdev, hence causing a
> deadlock. So let's introduce a new mutex so we don't cause a deadlock
> with driver core or netdev core.

Is there a bug report with more details?

If this fixes a regression, please add the appropriate `Fixes:` tag.

> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index b46bff8fe056..dc7ed5dd216b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -288,6 +288,8 @@ static const struct igb_reg_info igb_reg_info_tbl[] = {
>   	{}
>   };
>   
> +static DEFINE_MUTEX(igb_mutex);
> +
>   /* igb_regdump - register printout routine */
>   static void igb_regdump(struct e1000_hw *hw, struct igb_reg_info *reginfo)
>   {
> @@ -4026,9 +4028,14 @@ static int __igb_close(struct net_device *netdev, bool suspending)
>   
>   int igb_close(struct net_device *netdev)
>   {
> +	int err = 0;
> +
> +	mutex_lock(&igb_mutex);
>   	if (netif_device_present(netdev) || netdev->dismantle)
> -		return __igb_close(netdev, false);
> -	return 0;
> +		err = __igb_close(netdev, false);
> +	mutex_unlock(&igb_mutex);
> +
> +	return err;
>   }
>   
>   /**
> @@ -8760,7 +8767,7 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
>   	u32 wufc = runtime ? E1000_WUFC_LNKC : adapter->wol;
>   	bool wake;
>   
> -	rtnl_lock();
> +	mutex_lock(&igb_mutex);
>   	netif_device_detach(netdev);
>   
>   	if (netif_running(netdev))
> @@ -8769,7 +8776,7 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
>   	igb_ptp_suspend(adapter);
>   
>   	igb_clear_interrupt_scheme(adapter);
> -	rtnl_unlock();
> +	mutex_unlock(&igb_mutex);
>   
>   	status = rd32(E1000_STATUS);
>   	if (status & E1000_STATUS_LU)
> @@ -8897,13 +8904,13 @@ static int __maybe_unused igb_resume(struct device *dev)
>   
>   	wr32(E1000_WUS, ~0);
>   
> -	rtnl_lock();
> +	mutex_lock(&igb_mutex);
>   	if (!err && netif_running(netdev))
>   		err = __igb_open(netdev, true);
>   
>   	if (!err)
>   		netif_device_attach(netdev);
> -	rtnl_unlock();
> +	mutex_unlock(&igb_mutex);
>   
>   	return err;
>   }

The rest looks fine.


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH] igb: Use a sperate mutex insead of rtnl_lock()

[Alexander Duyck]

On Thu, Mar 26, 2020 at 3:39 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> Commit 9474933caf21 ("igb: close/suspend race in netif_device_detach")
> fixed race condition between close and power management ops by using
> rtnl_lock().
>
> This fix is a preparation for next patch, to prevent a dead lock under
> rtnl_lock() when calling runtime resume routine.
>
> However, we can't use device_lock() in igb_close() because when module
> is getting removed, the lock is already held for igb_remove(), and
> igb_close() gets called during unregistering the netdev, hence causing a
> deadlock. So let's introduce a new mutex so we don't cause a deadlock
> with driver core or netdev core.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

So this description doesn't make much sense to me. You describe the
use of the device_lock() in igb_close() but it isn't used there. In
addition it seems like you are arbitrarily moving code that was
wrapped in the rtnl_lock out of it. I'm not entirely sure that is safe
since there are calls within many of these functions that assume the
rtnl_lock is held and changing that is likely going to introduce more
issues.



> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index b46bff8fe056..dc7ed5dd216b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -288,6 +288,8 @@ static const struct igb_reg_info igb_reg_info_tbl[] = {
>         {}
>  };
>
> +static DEFINE_MUTEX(igb_mutex);
> +
>  /* igb_regdump - register printout routine */
>  static void igb_regdump(struct e1000_hw *hw, struct igb_reg_info *reginfo)
>  {
> @@ -4026,9 +4028,14 @@ static int __igb_close(struct net_device *netdev, bool suspending)
>
>  int igb_close(struct net_device *netdev)
>  {
> +       int err = 0;
> +
> +       mutex_lock(&igb_mutex);
>         if (netif_device_present(netdev) || netdev->dismantle)
> -               return __igb_close(netdev, false);
> -       return 0;
> +               err = __igb_close(netdev, false);
> +       mutex_unlock(&igb_mutex);
> +
> +       return err;
>  }
>

Okay, so I am guessing the problem has something to do with the
addition of the netdev->dismantle test here and the fact that it is
bypassing the present check for the hotplug remove case?

So it looks like nobody ever really reviewed commit 888f22931478
("igb: Free IRQs when device is hotplugged"). What I would recommend
is reverting it and instead we fix the remaining pieces that need to
be addressed in igb so it more closely matches what we have in e1000e
after commit a7023819404a ("e1000e: Use rtnl_lock to prevent race
conditions between net and pci/pm"). From what I can tell the only
pieces that are really missing is to update igb_io_error_detected so
that in addition to igb_down it will call igb_free_irq, and then in
addition we should be wrapping most of the code in that function with
an rtnl_lock since it is detaching a device and making modifications
to it.

Re: [Intel-wired-lan] [PATCH] igb: Use a sperate mutex insead of rtnl_lock()

[Kai-Heng Feng]

Hi Alexander,

> On Mar 27, 2020, at 00:27, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> 
> On Thu, Mar 26, 2020 at 3:39 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com <mailto:kai.heng.feng@canonical.com>> wrote:
>> 
>> Commit 9474933caf21 ("igb: close/suspend race in netif_device_detach")
>> fixed race condition between close and power management ops by using
>> rtnl_lock().
>> 
>> This fix is a preparation for next patch, to prevent a dead lock under
>> rtnl_lock() when calling runtime resume routine.
>> 
>> However, we can't use device_lock() in igb_close() because when module
>> is getting removed, the lock is already held for igb_remove(), and
>> igb_close() gets called during unregistering the netdev, hence causing a
>> deadlock. So let's introduce a new mutex so we don't cause a deadlock
>> with driver core or netdev core.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> So this description doesn't make much sense to me. You describe the
> use of the device_lock() in igb_close() but it isn't used there.

Sorry I forgot to add a revision number.
It was used by previous version and Aaron found a regression when device_lock() is used.

> In addition it seems like you are arbitrarily moving code that was
> wrapped in the rtnl_lock out of it. I'm not entirely sure that is safe
> since there are calls within many of these functions that assume the
> rtnl_lock is held and changing that is likely going to introduce more
> issues.

The reason why rtnl lock needs to be removed is because of the following patch:
https://lore.kernel.org/lkml/20200207101005.4454-2-kai.heng.feng@canonical.com/

Ethtools helpers already held rtnl_lock, so to prevent a deadlock, my idea is to use another lock to solve what "igb: close/suspend race in netif_device_detach" originally tried to fix.

> 
> 
> 
>> ---
>> drivers/net/ethernet/intel/igb/igb_main.c | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index b46bff8fe056..dc7ed5dd216b 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -288,6 +288,8 @@ static const struct igb_reg_info igb_reg_info_tbl[] = {
>>        {}
>> };
>> 
>> +static DEFINE_MUTEX(igb_mutex);
>> +
>> /* igb_regdump - register printout routine */
>> static void igb_regdump(struct e1000_hw *hw, struct igb_reg_info *reginfo)
>> {
>> @@ -4026,9 +4028,14 @@ static int __igb_close(struct net_device *netdev, bool suspending)
>> 
>> int igb_close(struct net_device *netdev)
>> {
>> +       int err = 0;
>> +
>> +       mutex_lock(&igb_mutex);
>>        if (netif_device_present(netdev) || netdev->dismantle)
>> -               return __igb_close(netdev, false);
>> -       return 0;
>> +               err = __igb_close(netdev, false);
>> +       mutex_unlock(&igb_mutex);
>> +
>> +       return err;
>> }
>> 
> 
> Okay, so I am guessing the problem has something to do with the
> addition of the netdev->dismantle test here and the fact that it is
> bypassing the present check for the hotplug remove case?

Please see the rationale above.

> 
> So it looks like nobody ever really reviewed commit 888f22931478
> ("igb: Free IRQs when device is hotplugged"). What I would recommend
> is reverting it and instead we fix the remaining pieces that need to
> be addressed in igb so it more closely matches what we have in e1000e
> after commit a7023819404a ("e1000e: Use rtnl_lock to prevent race
> conditions between net and pci/pm"). From what I can tell the only
> pieces that are really missing is to update igb_io_error_detected so
> that in addition to igb_down it will call igb_free_irq, and then in
> addition we should be wrapping most of the code in that function with
> an rtnl_lock since it is detaching a device and making modifications
> to it.

In addition to that, igb_shutdown() indirectly calls igb_close() when netdev unregistering the device.

My "only scratch the surface" approach is because I don't have a reproducer for commit "igb: close/suspend race in netif_device_detach", and I am afraid of breaking it.

Kai-Heng

Re: [Intel-wired-lan] [PATCH] igb: Use a sperate mutex insead of rtnl_lock()

[Alexander Duyck]

On Thu, Mar 26, 2020 at 10:16 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> Hi Alexander,
>
> > On Mar 27, 2020, at 00:27, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> >
> > On Thu, Mar 26, 2020 at 3:39 AM Kai-Heng Feng
> > <kai.heng.feng@canonical.com <mailto:kai.heng.feng@canonical.com>> wrote:
> >>
> >> Commit 9474933caf21 ("igb: close/suspend race in netif_device_detach")
> >> fixed race condition between close and power management ops by using
> >> rtnl_lock().
> >>
> >> This fix is a preparation for next patch, to prevent a dead lock under
> >> rtnl_lock() when calling runtime resume routine.
> >>
> >> However, we can't use device_lock() in igb_close() because when module
> >> is getting removed, the lock is already held for igb_remove(), and
> >> igb_close() gets called during unregistering the netdev, hence causing a
> >> deadlock. So let's introduce a new mutex so we don't cause a deadlock
> >> with driver core or netdev core.
> >>
> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >
> > So this description doesn't make much sense to me. You describe the
> > use of the device_lock() in igb_close() but it isn't used there.
>
> Sorry I forgot to add a revision number.
> It was used by previous version and Aaron found a regression when device_lock() is used.
>
> > In addition it seems like you are arbitrarily moving code that was
> > wrapped in the rtnl_lock out of it. I'm not entirely sure that is safe
> > since there are calls within many of these functions that assume the
> > rtnl_lock is held and changing that is likely going to introduce more
> > issues.
>
> The reason why rtnl lock needs to be removed is because of the following patch:
> https://lore.kernel.org/lkml/20200207101005.4454-2-kai.heng.feng@canonical.com/
>
> Ethtools helpers already held rtnl_lock, so to prevent a deadlock, my idea is to use another lock to solve what "igb: close/suspend race in netif_device_detach" originally tried to fix.

No offense, but that is a horrible reason to be removing the
rtnl_lock. It basically makes things worse since it is guaranteeing
the device can be in flux while you are trying to record the state of
the device.

Wouldn't it make more sense to check for pm_runtime_suspended and if
the interface is down and simply report SPEED_UNKNOWN rather than
trying to instrument the driver so that you can force it out of the
power management state to report statistics for an interface that we
know is already down?

> >
> > So it looks like nobody ever really reviewed commit 888f22931478
> > ("igb: Free IRQs when device is hotplugged"). What I would recommend
> > is reverting it and instead we fix the remaining pieces that need to
> > be addressed in igb so it more closely matches what we have in e1000e
> > after commit a7023819404a ("e1000e: Use rtnl_lock to prevent race
> > conditions between net and pci/pm"). From what I can tell the only
> > pieces that are really missing is to update igb_io_error_detected so
> > that in addition to igb_down it will call igb_free_irq, and then in
> > addition we should be wrapping most of the code in that function with
> > an rtnl_lock since it is detaching a device and making modifications
> > to it.
>
> In addition to that, igb_shutdown() indirectly calls igb_close() when netdev unregistering the device.

Yes, that is how it is supposed to be. We are modifying core state of
the netdevice and should only do so while holding the rtnl_lock.

> My "only scratch the surface" approach is because I don't have a reproducer for commit "igb: close/suspend race in netif_device_detach", and I am afraid of breaking it.
>
> Kai-Heng

This is taking things in the wrong direction. My advice is if you
cannot get reliable link information when the device is in a
pm_runtime_suspended state would be to simply test for that and report
the value out. Again, you can take a look at e1000e since it already
appears to be doing all this in e1000e_get_link_ksettings. We don't
need to have the drivers diverge from each other in solutions for
this. It is much easier to maintain if they can all be making use of
the same approach.

Thanks.

- Alex