| Message ID | 20220809073542.3390-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] igb: Add lock to avoid data race | expand |
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Lin Ma > Sent: Tuesday, August 9, 2022 9:36 AM > To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > ast@kernel.org; daniel@iogearbox.net; hawk@kernel.org; > john.fastabend@gmail.com; intel-wired-lan@lists.osuosl.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > bpf@vger.kernel.org > Cc: Lin Ma <linma@zju.edu.cn> > Subject: [Intel-wired-lan] [PATCH v3] igb: Add lock to avoid data race > > The commit c23d92b80e0b ("igb: Teardown SR-IOV before > unregister_netdev()") places the unregister_netdev() call after the > igb_disable_sriov() call to avoid functionality issue. > > However, it introduces several race conditions when detaching a device. > For example, when .remove() is called, the below interleaving leads to use- > after-free. > > (FREE from device detaching) | (USE from netdev core) > igb_remove | igb_ndo_get_vf_config > igb_disable_sriov | vf >= adapter->vfs_allocated_count? > kfree(adapter->vf_data) | > adapter->vfs_allocated_count = 0 | > | memcpy(... adapter->vf_data[vf] > > Moreover, the igb_disable_sriov() also suffers from data race with the > requests from VF driver. > > (FREE from device detaching) | (USE from requests) > igb_remove | igb_msix_other > igb_disable_sriov | igb_msg_task > kfree(adapter->vf_data) | vf < adapter->vfs_allocated_count > adapter->vfs_allocated_count = 0 | > > To this end, this commit first eliminates the data races from netdev core by > using rtnl_lock (similar to commit 719479230893 ("dpaa2-eth: add MAC/PHY > support through phylink")). And then adds a spinlock to eliminate races from > driver requests. (similar to commit 1e53834ce541 > ("ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero") > > > Fixes: c23d92b80e0b ("igb: Teardown SR-IOV before unregister_netdev()") > Signed-off-by: Lin Ma <linma@zju.edu.cn> > --- > V2 -> V3: make the commit message much clear > V1 -> V2: fix typo in title idb -> igb > V0 -> V1: change title from "Add rtnl_lock" to "Add lock" > add additional spinlock as suggested by Jakub, according to > 1e53834ce541 ("ixgbe: Add locking to prevent panic when setting > sriov_numvfs to zero") > > drivers/net/ethernet/intel/igb/igb.h | 2 ++ > drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++++++++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igb/igb.h > b/drivers/net/ethernet/intel/igb/igb.h > index 2d3daf022651..015b78144114 100644 > --- a/drivers/net/ethernet/intel/igb/igb.h > +++ b/drivers/net/ethernet/intel/igb/igb.h > @@ -664,6 +664,8 @@ struct igb_adapter { Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h index 2d3daf022651..015b78144114 100644 --- a/drivers/net/ethernet/intel/igb/igb.h +++ b/drivers/net/ethernet/intel/igb/igb.h @@ -664,6 +664,8 @@ struct igb_adapter { struct igb_mac_addr *mac_table; struct vf_mac_filter vf_macs; struct vf_mac_filter *vf_mac_list; + /* lock for VF resources */ + spinlock_t vfs_lock; }; /* flags controlling PTP/1588 function */ diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index d8b836a85cc3..2796e81d2726 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -3637,6 +3637,7 @@ static int igb_disable_sriov(struct pci_dev *pdev) struct net_device *netdev = pci_get_drvdata(pdev); struct igb_adapter *adapter = netdev_priv(netdev); struct e1000_hw *hw = &adapter->hw; + unsigned long flags; /* reclaim resources allocated to VFs */ if (adapter->vf_data) { @@ -3649,12 +3650,13 @@ static int igb_disable_sriov(struct pci_dev *pdev) pci_disable_sriov(pdev); msleep(500); } - + spin_lock_irqsave(&adapter->vfs_lock, flags); kfree(adapter->vf_mac_list); adapter->vf_mac_list = NULL; kfree(adapter->vf_data); adapter->vf_data = NULL; adapter->vfs_allocated_count = 0; + spin_unlock_irqrestore(&adapter->vfs_lock, flags); wr32(E1000_IOVCTL, E1000_IOVCTL_REUSE_VFQ); wrfl(); msleep(100); @@ -3814,7 +3816,9 @@ static void igb_remove(struct pci_dev *pdev) igb_release_hw_control(adapter); #ifdef CONFIG_PCI_IOV + rtnl_lock(); igb_disable_sriov(pdev); + rtnl_unlock(); #endif unregister_netdev(netdev); @@ -3974,6 +3978,9 @@ static int igb_sw_init(struct igb_adapter *adapter) spin_lock_init(&adapter->nfc_lock); spin_lock_init(&adapter->stats64_lock); + + /* init spinlock to avoid concurrency of VF resources */ + spin_lock_init(&adapter->vfs_lock); #ifdef CONFIG_PCI_IOV switch (hw->mac.type) { case e1000_82576: @@ -7958,8 +7965,10 @@ static void igb_rcv_msg_from_vf(struct igb_adapter *adapter, u32 vf) static void igb_msg_task(struct igb_adapter *adapter) { struct e1000_hw *hw = &adapter->hw; + unsigned long flags; u32 vf; + spin_lock_irqsave(&adapter->vfs_lock, flags); for (vf = 0; vf < adapter->vfs_allocated_count; vf++) { /* process any reset requests */ if (!igb_check_for_rst(hw, vf)) @@ -7973,6 +7982,7 @@ static void igb_msg_task(struct igb_adapter *adapter) if (!igb_check_for_ack(hw, vf)) igb_rcv_ack_from_vf(adapter, vf); } + spin_unlock_irqrestore(&adapter->vfs_lock, flags); } /**
The commit c23d92b80e0b ("igb: Teardown SR-IOV before unregister_netdev()") places the unregister_netdev() call after the igb_disable_sriov() call to avoid functionality issue. However, it introduces several race conditions when detaching a device. For example, when .remove() is called, the below interleaving leads to use-after-free. (FREE from device detaching) | (USE from netdev core) igb_remove | igb_ndo_get_vf_config igb_disable_sriov | vf >= adapter->vfs_allocated_count? kfree(adapter->vf_data) | adapter->vfs_allocated_count = 0 | | memcpy(... adapter->vf_data[vf] Moreover, the igb_disable_sriov() also suffers from data race with the requests from VF driver. (FREE from device detaching) | (USE from requests) igb_remove | igb_msix_other igb_disable_sriov | igb_msg_task kfree(adapter->vf_data) | vf < adapter->vfs_allocated_count adapter->vfs_allocated_count = 0 | To this end, this commit first eliminates the data races from netdev core by using rtnl_lock (similar to commit 719479230893 ("dpaa2-eth: add MAC/PHY support through phylink")). And then adds a spinlock to eliminate races from driver requests. (similar to commit 1e53834ce541 ("ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero") Fixes: c23d92b80e0b ("igb: Teardown SR-IOV before unregister_netdev()") Signed-off-by: Lin Ma <linma@zju.edu.cn> --- V2 -> V3: make the commit message much clear V1 -> V2: fix typo in title idb -> igb V0 -> V1: change title from "Add rtnl_lock" to "Add lock" add additional spinlock as suggested by Jakub, according to 1e53834ce541 ("ixgbe: Add locking to prevent panic when setting sriov_numvfs to zero") drivers/net/ethernet/intel/igb/igb.h | 2 ++ drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-)