diff mbox series

[net,1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero

Message ID 20210812171856.1867667-1-anthony.l.nguyen@intel.com
State New
Headers show
Series [net,1/1] ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero | expand

Commit Message

Tony Nguyen Aug. 12, 2021, 5:18 p.m. UTC 
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(+)

Comments

Jakub Kicinski Aug. 14, 2021, 12:20 a.m. UTC | #1 
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.
Tony Nguyen Aug. 16, 2021, 5:52 p.m. UTC | #2 
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
Ken Cox Aug. 17, 2021, 10:23 a.m. UTC | #3 
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
diff mbox series

Patch

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)