Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
@ 2021-08-12 17:18 Tony Nguyen
  2021-08-14  0:20 ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Tony Nguyen @ 2021-08-12 17:18 UTC (permalink / raw)
  To: davem, kuba
  Cc: Ken Cox, netdev, anthony.l.nguyen, Anirudh Venkataramanan, Marek Szlosek

From: Ken Cox <jkc@redhat.com>

It is possible to disable VFs while the PF driver is processing requests
from the VF driver.  This can result in a panic.

BUG: unable to handle kernel paging request at 000000000000106c
PGD 0 P4D 0
Oops: 0000 [#1] SMP NOPTI
CPU: 8 PID: 0 Comm: swapper/8 Kdump: loaded Tainted: G          I      --------- -  -
Hardware name: Dell Inc. PowerEdge R740/06WXJT, BIOS 2.8.2 08/27/2020
RIP: 0010:ixgbe_msg_task+0x4c8/0x1690 [ixgbe]
Code: 00 00 48 8d 04 40 48 c1 e0 05 89 7c 24 24 89 fd 48 89 44 24 10 83 ff 01 0f 84 b8 04 00 00 4c 8b 64 24 10 4d 03 a5 48 22 00 00 <41> 80 7c 24 4c 00 0f 84 8a 03 00 00 0f b7 c7 83 f8 08 0f 84 8f 0a
RSP: 0018:ffffb337869f8df8 EFLAGS: 00010002
RAX: 0000000000001020 RBX: 0000000000000000 RCX: 000000000000002b
RDX: 0000000000000002 RSI: 0000000000000008 RDI: 0000000000000006
RBP: 0000000000000006 R08: 0000000000000002 R09: 0000000000029780
R10: 00006957d8f42832 R11: 0000000000000000 R12: 0000000000001020
R13: ffff8a00e8978ac0 R14: 000000000000002b R15: ffff8a00e8979c80
FS:  0000000000000000(0000) GS:ffff8a07dfd00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000000106c CR3: 0000000063e10004 CR4: 00000000007726e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 <IRQ>
 ? ttwu_do_wakeup+0x19/0x140
 ? try_to_wake_up+0x1cd/0x550
 ? ixgbevf_update_xcast_mode+0x71/0xc0 [ixgbevf]
 ixgbe_msix_other+0x17e/0x310 [ixgbe]
 __handle_irq_event_percpu+0x40/0x180
 handle_irq_event_percpu+0x30/0x80
 handle_irq_event+0x36/0x53
 handle_edge_irq+0x82/0x190
 handle_irq+0x1c/0x30
 do_IRQ+0x49/0xd0
 common_interrupt+0xf/0xf

This can be eventually be reproduced with the following script:

while :
do
	echo 63 > /sys/class/net/ens3f0/device/sriov_numvfs
	sleep 1
	echo 0 > /sys/class/net/ens3f0/device/sriov_numvfs
        sleep 1
done

Fixes: da36b64736cf ("ixgbe: Implement PCI SR-IOV sysfs callback operation")
Signed-off-by: Ken Cox <jkc@redhat.com>
Acked-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Marek Szlosek <marek.szlosek@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h       | 1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index a604552fa634..696bb2a61ea7 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -807,6 +807,7 @@ enum ixgbe_state_t {
 	__IXGBE_PTP_RUNNING,
 	__IXGBE_PTP_TX_IN_PROGRESS,
 	__IXGBE_RESET_REQUESTED,
+	__IXGBE_DISABLING_VFS,
 };
 
 struct ixgbe_cb {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 214a38de3f41..0a1a8756f1fd 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -206,8 +206,12 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
 	unsigned int num_vfs = adapter->num_vfs, vf;
 	int rss;
 
+	while (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter->state))
+		usleep_range(1000, 2000);
+
 	/* set num VFs to 0 to prevent access to vfinfo */
 	adapter->num_vfs = 0;
+	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);
 
 	/* put the reference to all of the vf devices */
 	for (vf = 0; vf < num_vfs; ++vf) {
@@ -1307,6 +1311,9 @@ void ixgbe_msg_task(struct ixgbe_adapter *adapter)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 vf;
 
+	if (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter->state))
+		return;
+
 	for (vf = 0; vf < adapter->num_vfs; vf++) {
 		/* process any reset requests */
 		if (!ixgbe_check_for_rst(hw, vf))
@@ -1320,6 +1327,7 @@ void ixgbe_msg_task(struct ixgbe_adapter *adapter)
 		if (!ixgbe_check_for_ack(hw, vf))
 			ixgbe_rcv_ack_from_vf(adapter, vf);
 	}
+	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);
 }
 
 void ixgbe_disable_tx_rx(struct ixgbe_adapter *adapter)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
  2021-08-12 17:18 [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero Tony Nguyen
@ 2021-08-14  0:20 ` Jakub Kicinski
  2021-08-16 17:52   ` Nguyen, Anthony L
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-14  0:20 UTC (permalink / raw)
  To: Tony Nguyen; +Cc: davem, Ken Cox, netdev, Anirudh Venkataramanan, Marek Szlosek

On Thu, 12 Aug 2021 10:18:56 -0700 Tony Nguyen wrote:
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> index 214a38de3f41..0a1a8756f1fd 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> @@ -206,8 +206,12 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
>  	unsigned int num_vfs = adapter->num_vfs, vf;
>  	int rss;
>  
> +	while (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter->state))
> +		usleep_range(1000, 2000);
> +
>  	/* set num VFs to 0 to prevent access to vfinfo */
>  	adapter->num_vfs = 0;
> +	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);
>  
>  	/* put the reference to all of the vf devices */
>  	for (vf = 0; vf < num_vfs; ++vf) {
> @@ -1307,6 +1311,9 @@ void ixgbe_msg_task(struct ixgbe_adapter *adapter)
>  	struct ixgbe_hw *hw = &adapter->hw;
>  	u32 vf;
>  
> +	if (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter->state))
> +		return;
> +
>  	for (vf = 0; vf < adapter->num_vfs; vf++) {
>  		/* process any reset requests */
>  		if (!ixgbe_check_for_rst(hw, vf))
> @@ -1320,6 +1327,7 @@ void ixgbe_msg_task(struct ixgbe_adapter *adapter)
>  		if (!ixgbe_check_for_ack(hw, vf))
>  			ixgbe_rcv_ack_from_vf(adapter, vf);
>  	}
> +	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);

Like I've already said two or three times. No flag based locking.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
  2021-08-14  0:20 ` Jakub Kicinski
@ 2021-08-16 17:52   ` Nguyen, Anthony L
  2021-08-17 10:23     ` Ken Cox
  0 siblings, 1 reply; 9+ messages in thread
From: Nguyen, Anthony L @ 2021-08-16 17:52 UTC (permalink / raw)
  To: kuba
  Cc: jkc, Szlosek, Marek, Yang, Lihong, davem, netdev, Venkataramanan,
	Anirudh

On Fri, 2021-08-13 at 17:20 -0700, Jakub Kicinski wrote:
> On Thu, 12 Aug 2021 10:18:56 -0700 Tony Nguyen wrote:
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > index 214a38de3f41..0a1a8756f1fd 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
> > @@ -206,8 +206,12 @@ int ixgbe_disable_sriov(struct ixgbe_adapter
> > *adapter)
> >  	unsigned int num_vfs = adapter->num_vfs, vf;
> >  	int rss;
> >  
> > +	while (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter-
> > >state))
> > +		usleep_range(1000, 2000);
> > +
> >  	/* set num VFs to 0 to prevent access to vfinfo */
> >  	adapter->num_vfs = 0;
> > +	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);
> >  
> >  	/* put the reference to all of the vf devices */
> >  	for (vf = 0; vf < num_vfs; ++vf) {
> > @@ -1307,6 +1311,9 @@ void ixgbe_msg_task(struct ixgbe_adapter
> > *adapter)
> >  	struct ixgbe_hw *hw = &adapter->hw;
> >  	u32 vf;
> >  
> > +	if (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter->state))
> > +		return;
> > +
> >  	for (vf = 0; vf < adapter->num_vfs; vf++) {
> >  		/* process any reset requests */
> >  		if (!ixgbe_check_for_rst(hw, vf))
> > @@ -1320,6 +1327,7 @@ void ixgbe_msg_task(struct ixgbe_adapter
> > *adapter)
> >  		if (!ixgbe_check_for_ack(hw, vf))
> >  			ixgbe_rcv_ack_from_vf(adapter, vf);
> >  	}
> > +	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);
> 
> Like I've already said two or three times. No flag based locking.

Ken,

Did you want to make this change or did you want Intel to do it?

Thanks,
Tony



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
  2021-08-16 17:52   ` Nguyen, Anthony L
@ 2021-08-17 10:23     ` Ken Cox
  0 siblings, 0 replies; 9+ messages in thread
From: Ken Cox @ 2021-08-17 10:23 UTC (permalink / raw)
  To: Nguyen, Anthony L, kuba
  Cc: Szlosek, Marek, Yang, Lihong, davem, netdev, Venkataramanan, Anirudh



On 8/16/21 12:52 PM, Nguyen, Anthony L wrote:
> On Fri, 2021-08-13 at 17:20 -0700, Jakub Kicinski wrote:
>> On Thu, 12 Aug 2021 10:18:56 -0700 Tony Nguyen wrote:
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> index 214a38de3f41..0a1a8756f1fd 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
>>> @@ -206,8 +206,12 @@ int ixgbe_disable_sriov(struct ixgbe_adapter
>>> *adapter)
>>>   	unsigned int num_vfs = adapter->num_vfs, vf;
>>>   	int rss;
>>>   
>>> +	while (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter-
>>>> state))
>>> +		usleep_range(1000, 2000);
>>> +
>>>   	/* set num VFs to 0 to prevent access to vfinfo */
>>>   	adapter->num_vfs = 0;
>>> +	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);
>>>   
>>>   	/* put the reference to all of the vf devices */
>>>   	for (vf = 0; vf < num_vfs; ++vf) {
>>> @@ -1307,6 +1311,9 @@ void ixgbe_msg_task(struct ixgbe_adapter
>>> *adapter)
>>>   	struct ixgbe_hw *hw = &adapter->hw;
>>>   	u32 vf;
>>>   
>>> +	if (test_and_set_bit(__IXGBE_DISABLING_VFS, &adapter->state))
>>> +		return;
>>> +
>>>   	for (vf = 0; vf < adapter->num_vfs; vf++) {
>>>   		/* process any reset requests */
>>>   		if (!ixgbe_check_for_rst(hw, vf))
>>> @@ -1320,6 +1327,7 @@ void ixgbe_msg_task(struct ixgbe_adapter
>>> *adapter)
>>>   		if (!ixgbe_check_for_ack(hw, vf))
>>>   			ixgbe_rcv_ack_from_vf(adapter, vf);
>>>   	}
>>> +	clear_bit(__IXGBE_DISABLING_VFS, &adapter->state);
>>
>> Like I've already said two or three times. No flag based locking.
> 
> Ken,
> 
> Did you want to make this change or did you want Intel to do it?

Hi Tony,

It would be great if Intel could make the change for the locking.

Thanks,
Ken


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
  2022-06-30 15:11     ` Jakub Kicinski
@ 2022-07-01  8:31       ` Piotr Skajewski
  0 siblings, 0 replies; 9+ messages in thread
From: Piotr Skajewski @ 2022-07-01  8:31 UTC (permalink / raw)
  To: kuba
  Cc: anthony.l.nguyen, davem, edumazet, konrad0.jankowski, netdev,
	pabeni, piotrx.skajewski

On Thu, 30 Jun 2022 08:11:34 -0700 Jakub Kicinski wrote:
> On Thu, 30 Jun 2022 12:08:39 +0200 Piotr Skajewski wrote:
> > On Tue, 28 Jun 2022 10:27:07 -0700 Jakub Kicinski wrote:
> > > On Tue, 28 Jun 2022 09:53:46 -0700 Tony Nguyen wrote:  
> > > > +	spin_lock_irqsave(&adapter->vfs_lock, flags);
> > > > +
> > > >  	/* set num VFs to 0 to prevent access to vfinfo */
> > > >  	adapter->num_vfs = 0;
> > > >  
> > > > @@ -228,6 +231,8 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
> > > >  	kfree(adapter->mv_list);
> > > >  	adapter->mv_list = NULL;
> > > >  
> > > > +	spin_unlock_irqrestore(&adapter->vfs_lock, flags);  
> > >
> > > There's a pci_dev_put() in there, are you sure it won't sleep?  
> > 
> > Thank Jakub for your notice, during development we were aware about this
> > and tests we've made on this particular case, did not report any problems
> > that could be related to might_sleep in conjunction with spinlock.
> 
> To be on the safe side how about we protect adapter->num_vfs instead 
> of adapter->vfinfo ?
> 
> You can hold the lock just around setting adapter->num_vfs to zero,
> and then inside ixgbe_msg_task() you don't have to add the new if()
> because the loop bound is already adapter->num_vfs.
> 
> Smaller change, and safer.

Yes sounds good, I will test it and prepare the patch.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
  2022-06-30 10:08   ` Piotr Skajewski
@ 2022-06-30 15:11     ` Jakub Kicinski
  2022-07-01  8:31       ` Piotr Skajewski
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2022-06-30 15:11 UTC (permalink / raw)
  To: Piotr Skajewski
  Cc: anthony.l.nguyen, davem, edumazet, konrad0.jankowski, netdev, pabeni

On Thu, 30 Jun 2022 12:08:39 +0200 Piotr Skajewski wrote:
> On Tue, 28 Jun 2022 10:27:07 -0700 Jakub Kicinski wrote:
> > On Tue, 28 Jun 2022 09:53:46 -0700 Tony Nguyen wrote:  
> > > +	spin_lock_irqsave(&adapter->vfs_lock, flags);
> > > +
> > >  	/* set num VFs to 0 to prevent access to vfinfo */
> > >  	adapter->num_vfs = 0;
> > >  
> > > @@ -228,6 +231,8 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
> > >  	kfree(adapter->mv_list);
> > >  	adapter->mv_list = NULL;
> > >  
> > > +	spin_unlock_irqrestore(&adapter->vfs_lock, flags);  
> >
> > There's a pci_dev_put() in there, are you sure it won't sleep?  
> 
> Thank Jakub for your notice, during development we were aware about this
> and tests we've made on this particular case, did not report any problems
> that could be related to might_sleep in conjunction with spinlock.

To be on the safe side how about we protect adapter->num_vfs instead 
of adapter->vfinfo ?

You can hold the lock just around setting adapter->num_vfs to zero,
and then inside ixgbe_msg_task() you don't have to add the new if()
because the loop bound is already adapter->num_vfs.

Smaller change, and safer.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
  2022-06-28 17:27 ` Jakub Kicinski
@ 2022-06-30 10:08   ` Piotr Skajewski
  2022-06-30 15:11     ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Piotr Skajewski @ 2022-06-30 10:08 UTC (permalink / raw)
  To: kuba
  Cc: anthony.l.nguyen, davem, edumazet, konrad0.jankowski, netdev,
	pabeni, piotrx.skajewski

On Tue, 28 Jun 2022 10:27:07 -0700 Jakub Kicinski wrote:
> On Tue, 28 Jun 2022 09:53:46 -0700 Tony Nguyen wrote:
> > +	spin_lock_irqsave(&adapter->vfs_lock, flags);
> > +
> >  	/* set num VFs to 0 to prevent access to vfinfo */
> >  	adapter->num_vfs = 0;
> >  
> > @@ -228,6 +231,8 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
> >  	kfree(adapter->mv_list);
> >  	adapter->mv_list = NULL;
> >  
> > +	spin_unlock_irqrestore(&adapter->vfs_lock, flags);
>
> There's a pci_dev_put() in there, are you sure it won't sleep?

Thank Jakub for your notice, during development we were aware about this
and tests we've made on this particular case, did not report any problems
that could be related to might_sleep in conjunction with spinlock.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
  2022-06-28 16:53 Tony Nguyen
@ 2022-06-28 17:27 ` Jakub Kicinski
  2022-06-30 10:08   ` Piotr Skajewski
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2022-06-28 17:27 UTC (permalink / raw)
  To: Tony Nguyen
  Cc: davem, pabeni, edumazet, Piotr Skajewski, netdev, Konrad Jankowski

On Tue, 28 Jun 2022 09:53:46 -0700 Tony Nguyen wrote:
> +	spin_lock_irqsave(&adapter->vfs_lock, flags);
> +
>  	/* set num VFs to 0 to prevent access to vfinfo */
>  	adapter->num_vfs = 0;
>  
> @@ -228,6 +231,8 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
>  	kfree(adapter->mv_list);
>  	adapter->mv_list = NULL;
>  
> +	spin_unlock_irqrestore(&adapter->vfs_lock, flags);

There's a pci_dev_put() in there, are you sure it won't sleep?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero
@ 2022-06-28 16:53 Tony Nguyen
  2022-06-28 17:27 ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Tony Nguyen @ 2022-06-28 16:53 UTC (permalink / raw)
  To: davem, kuba, pabeni, edumazet
  Cc: Piotr Skajewski, netdev, anthony.l.nguyen, Konrad Jankowski

From: Piotr Skajewski <piotrx.skajewski@intel.com>

It is possible to disable VFs while the PF driver is processing requests
from the VF driver.  This can result in a panic.

BUG: unable to handle kernel paging request at 000000000000106c
PGD 0 P4D 0
Oops: 0000 [#1] SMP NOPTI
CPU: 8 PID: 0 Comm: swapper/8 Kdump: loaded Tainted: G I      --------- -
Hardware name: Dell Inc. PowerEdge R740/06WXJT, BIOS 2.8.2 08/27/2020
RIP: 0010:ixgbe_msg_task+0x4c8/0x1690 [ixgbe]
Code: 00 00 48 8d 04 40 48 c1 e0 05 89 7c 24 24 89 fd 48 89 44 24 10 83 ff
01 0f 84 b8 04 00 00 4c 8b 64 24 10 4d 03 a5 48 22 00 00 <41> 80 7c 24 4c
00 0f 84 8a 03 00 00 0f b7 c7 83 f8 08 0f 84 8f 0a
RSP: 0018:ffffb337869f8df8 EFLAGS: 00010002
RAX: 0000000000001020 RBX: 0000000000000000 RCX: 000000000000002b
RDX: 0000000000000002 RSI: 0000000000000008 RDI: 0000000000000006
RBP: 0000000000000006 R08: 0000000000000002 R09: 0000000000029780
R10: 00006957d8f42832 R11: 0000000000000000 R12: 0000000000001020
R13: ffff8a00e8978ac0 R14: 000000000000002b R15: ffff8a00e8979c80
FS:  0000000000000000(0000) GS:ffff8a07dfd00000(0000) knlGS:00000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000000106c CR3: 0000000063e10004 CR4: 00000000007726e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 <IRQ>
 ? ttwu_do_wakeup+0x19/0x140
 ? try_to_wake_up+0x1cd/0x550
 ? ixgbevf_update_xcast_mode+0x71/0xc0 [ixgbevf]
 ixgbe_msix_other+0x17e/0x310 [ixgbe]
 __handle_irq_event_percpu+0x40/0x180
 handle_irq_event_percpu+0x30/0x80
 handle_irq_event+0x36/0x53
 handle_edge_irq+0x82/0x190
 handle_irq+0x1c/0x30
 do_IRQ+0x49/0xd0
 common_interrupt+0xf/0xf

This can be eventually be reproduced with the following script:

while :
do
    echo 63 > /sys/class/net/<devname>/device/sriov_numvfs
    sleep 1
    echo 0 > /sys/class/net/<devname>/device/sriov_numvfs
    sleep 1
done

Add lock when disabling SR-IOV to prevent process VF mailbox communication.

Fixes: d773d1310625 ("ixgbe: Fix memory leak when SR-IOV VFs are direct assigned")
Signed-off-by: Piotr Skajewski <piotrx.skajewski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  3 ++
 .../net/ethernet/intel/ixgbe/ixgbe_sriov.c    | 30 ++++++++++++-------
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 921a4d977d65..8813b4dd6872 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -779,6 +779,7 @@ struct ixgbe_adapter {
 #ifdef CONFIG_IXGBE_IPSEC
 	struct ixgbe_ipsec *ipsec;
 #endif /* CONFIG_IXGBE_IPSEC */
+	spinlock_t vfs_lock;
 };
 
 static inline int ixgbe_determine_xdp_q_idx(int cpu)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 77c2e70b0860..55f91c9ff047 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6403,6 +6403,9 @@ static int ixgbe_sw_init(struct ixgbe_adapter *adapter,
 	/* n-tuple support exists, always init our spinlock */
 	spin_lock_init(&adapter->fdir_perfect_lock);
 
+	/* init spinlock to avoid concurrency of VF resources */
+	spin_lock_init(&adapter->vfs_lock);
+
 #ifdef CONFIG_IXGBE_DCB
 	ixgbe_init_dcb(adapter);
 #endif
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index d4e63f0644c3..7f132322e2e3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -205,8 +205,11 @@ void ixgbe_enable_sriov(struct ixgbe_adapter *adapter, unsigned int max_vfs)
 int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
 {
 	unsigned int num_vfs = adapter->num_vfs, vf;
+	unsigned long flags;
 	int rss;
 
+	spin_lock_irqsave(&adapter->vfs_lock, flags);
+
 	/* set num VFs to 0 to prevent access to vfinfo */
 	adapter->num_vfs = 0;
 
@@ -228,6 +231,8 @@ int ixgbe_disable_sriov(struct ixgbe_adapter *adapter)
 	kfree(adapter->mv_list);
 	adapter->mv_list = NULL;
 
+	spin_unlock_irqrestore(&adapter->vfs_lock, flags);
+
 	/* if SR-IOV is already disabled then there is nothing to do */
 	if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
 		return 0;
@@ -1355,21 +1360,26 @@ static void ixgbe_rcv_ack_from_vf(struct ixgbe_adapter *adapter, u32 vf)
 void ixgbe_msg_task(struct ixgbe_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
+	unsigned long flags;
 	u32 vf;
 
-	for (vf = 0; vf < adapter->num_vfs; vf++) {
-		/* process any reset requests */
-		if (!ixgbe_check_for_rst(hw, vf))
-			ixgbe_vf_reset_event(adapter, vf);
+	spin_lock_irqsave(&adapter->vfs_lock, flags);
+	if (adapter->vfinfo) {
+		for (vf = 0; vf < adapter->num_vfs; vf++) {
+			/* process any reset requests */
+			if (!ixgbe_check_for_rst(hw, vf))
+				ixgbe_vf_reset_event(adapter, vf);
 
-		/* process any messages pending */
-		if (!ixgbe_check_for_msg(hw, vf))
-			ixgbe_rcv_msg_from_vf(adapter, vf);
+			/* process any messages pending */
+			if (!ixgbe_check_for_msg(hw, vf))
+				ixgbe_rcv_msg_from_vf(adapter, vf);
 
-		/* process any acks */
-		if (!ixgbe_check_for_ack(hw, vf))
-			ixgbe_rcv_ack_from_vf(adapter, vf);
+			/* process any acks */
+			if (!ixgbe_check_for_ack(hw, vf))
+				ixgbe_rcv_ack_from_vf(adapter, vf);
+		}
 	}
+	spin_unlock_irqrestore(&adapter->vfs_lock, flags);
 }
 
 static inline void ixgbe_ping_vf(struct ixgbe_adapter *adapter, int vf)
-- 
2.35.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-07-01  8:36 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12 17:18 [PATCH net 1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero Tony Nguyen
2021-08-14  0:20 ` Jakub Kicinski
2021-08-16 17:52   ` Nguyen, Anthony L
2021-08-17 10:23     ` Ken Cox
2022-06-28 16:53 Tony Nguyen
2022-06-28 17:27 ` Jakub Kicinski
2022-06-30 10:08   ` Piotr Skajewski
2022-06-30 15:11     ` Jakub Kicinski
2022-07-01  8:31       ` Piotr Skajewski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).