Skip to content

Commit 80496f3

Browse files
PlaidCatPlaidCat
committed
iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies
jira LE-1907 Rebuild_History Non-Buildable kernel-5.14.0-284.30.1.el9_2 commit-author Ahmed Zaki <[email protected]> commit d1639a1 A driver's lock (crit_lock) is used to serialize all the driver's tasks. Lockdep, however, shows a circular dependency between rtnl and crit_lock. This happens when an ndo that already holds the rtnl requests the driver to reset, since the reset task (in some paths) tries to grab rtnl to either change real number of queues of update netdev features. [566.241851] ====================================================== [566.241893] WARNING: possible circular locking dependency detected [566.241936] 6.2.14-100.fc36.x86_64+debug #1 Tainted: G OE [566.241984] ------------------------------------------------------ [566.242025] repro.sh/2604 is trying to acquire lock: [566.242061] ffff9280fc5ceee8 (&adapter->crit_lock){+.+.}-{3:3}, at: iavf_close+0x3c/0x240 [iavf] [566.242167] but task is already holding lock: [566.242209] ffffffff9976d350 (rtnl_mutex){+.+.}-{3:3}, at: iavf_remove+0x6b5/0x730 [iavf] [566.242300] which lock already depends on the new lock. [566.242353] the existing dependency chain (in reverse order) is: [566.242401] -> #1 (rtnl_mutex){+.+.}-{3:3}: [566.242451] __mutex_lock+0xc1/0xbb0 [566.242489] iavf_init_interrupt_scheme+0x179/0x440 [iavf] [566.242560] iavf_watchdog_task+0x80b/0x1400 [iavf] [566.242627] process_one_work+0x2b3/0x560 [566.242663] worker_thread+0x4f/0x3a0 [566.242696] kthread+0xf2/0x120 [566.242730] ret_from_fork+0x29/0x50 [566.242763] -> #0 (&adapter->crit_lock){+.+.}-{3:3}: [566.242815] __lock_acquire+0x15ff/0x22b0 [566.242869] lock_acquire+0xd2/0x2c0 [566.242901] __mutex_lock+0xc1/0xbb0 [566.242934] iavf_close+0x3c/0x240 [iavf] [566.242997] __dev_close_many+0xac/0x120 [566.243036] dev_close_many+0x8b/0x140 [566.243071] unregister_netdevice_many_notify+0x165/0x7c0 [566.243116] unregister_netdevice_queue+0xd3/0x110 [566.243157] iavf_remove+0x6c1/0x730 [iavf] [566.243217] pci_device_remove+0x33/0xa0 [566.243257] device_release_driver_internal+0x1bc/0x240 [566.243299] pci_stop_bus_device+0x6c/0x90 [566.243338] pci_stop_and_remove_bus_device+0xe/0x20 [566.243380] pci_iov_remove_virtfn+0xd1/0x130 [566.243417] sriov_disable+0x34/0xe0 [566.243448] ice_free_vfs+0x2da/0x330 [ice] [566.244383] ice_sriov_configure+0x88/0xad0 [ice] [566.245353] sriov_numvfs_store+0xde/0x1d0 [566.246156] kernfs_fop_write_iter+0x15e/0x210 [566.246921] vfs_write+0x288/0x530 [566.247671] ksys_write+0x74/0xf0 [566.248408] do_syscall_64+0x58/0x80 [566.249145] entry_SYSCALL_64_after_hwframe+0x72/0xdc [566.249886] other info that might help us debug this: [566.252014] Possible unsafe locking scenario: [566.253432] CPU0 CPU1 [566.254118] ---- ---- [566.254800] lock(rtnl_mutex); [566.255514] lock(&adapter->crit_lock); [566.256233] lock(rtnl_mutex); [566.256897] lock(&adapter->crit_lock); [566.257388] *** DEADLOCK *** The deadlock can be triggered by a script that is continuously resetting the VF adapter while doing other operations requiring RTNL, e.g: while :; do ip link set $VF up ethtool --set-channels $VF combined 2 ip link set $VF down ip link set $VF up ethtool --set-channels $VF combined 4 ip link set $VF down done Any operation that triggers a reset can substitute "ethtool --set-channles" As a fix, add a new task "finish_config" that do all the work which needs rtnl lock. With the exception of iavf_remove(), all work that require rtnl should be called from this task. As for iavf_remove(), at the point where we need to call unregister_netdevice() (and grab rtnl_lock), we make sure the finish_config task is not running (cancel_work_sync()) to safely grab rtnl. Subsequent finish_config work cannot restart after that since the task is guarded by the __IAVF_IN_REMOVE_TASK bit in iavf_schedule_finish_config(). Fixes: 5ac49f3 ("iavf: use mutexes for locking of critical sections") Signed-off-by: Ahmed Zaki <[email protected]> Signed-off-by: Mateusz Palczewski <[email protected]> Tested-by: Rafal Romanowski <[email protected]> Signed-off-by: Tony Nguyen <[email protected]> (cherry picked from commit d1639a1) Signed-off-by: Jonathan Maple <[email protected]>
1 parent df03610 commit 80496f3

File tree

3 files changed

+85
-32
lines changed

3 files changed

+85
-32
lines changed

‎drivers/net/ethernet/intel/iavf/iavf.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,7 @@ struct iavf_adapter {
252252
struct workqueue_struct *wq;
253253
struct work_struct reset_task;
254254
struct work_struct adminq_task;
255+
struct work_struct finish_config;
255256
struct delayed_work client_task;
256257
wait_queue_head_t down_waitqueue;
257258
wait_queue_head_t reset_waitqueue;
@@ -517,6 +518,7 @@ int iavf_process_config(struct iavf_adapter *adapter);
517518
int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter);
518519
void iavf_schedule_reset(struct iavf_adapter *adapter);
519520
void iavf_schedule_request_stats(struct iavf_adapter *adapter);
521+
void iavf_schedule_finish_config(struct iavf_adapter *adapter);
520522
void iavf_reset(struct iavf_adapter *adapter);
521523
void iavf_set_ethtool_ops(struct net_device *netdev);
522524
void iavf_update_stats(struct iavf_adapter *adapter);

‎drivers/net/ethernet/intel/iavf/iavf_main.c

Lines changed: 82 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1688,10 +1688,10 @@ static int iavf_set_interrupt_capability(struct iavf_adapter *adapter)
16881688
adapter->msix_entries[vector].entry = vector;
16891689

16901690
err = iavf_acquire_msix_vectors(adapter, v_budget);
1691+
if (!err)
1692+
iavf_schedule_finish_config(adapter);
16911693

16921694
out:
1693-
netif_set_real_num_rx_queues(adapter->netdev, pairs);
1694-
netif_set_real_num_tx_queues(adapter->netdev, pairs);
16951695
return err;
16961696
}
16971697

@@ -1911,9 +1911,7 @@ static int iavf_init_interrupt_scheme(struct iavf_adapter *adapter)
19111911
goto err_alloc_queues;
19121912
}
19131913

1914-
rtnl_lock();
19151914
err = iavf_set_interrupt_capability(adapter);
1916-
rtnl_unlock();
19171915
if (err) {
19181916
dev_err(&adapter->pdev->dev,
19191917
"Unable to setup interrupt capabilities\n");
@@ -1999,6 +1997,78 @@ static int iavf_reinit_interrupt_scheme(struct iavf_adapter *adapter, bool runni
19991997
return err;
20001998
}
20011999

2000+
/**
2001+
* iavf_finish_config - do all netdev work that needs RTNL
2002+
* @work: our work_struct
2003+
*
2004+
* Do work that needs both RTNL and crit_lock.
2005+
**/
2006+
static void iavf_finish_config(struct work_struct *work)
2007+
{
2008+
struct iavf_adapter *adapter;
2009+
int pairs, err;
2010+
2011+
adapter = container_of(work, struct iavf_adapter, finish_config);
2012+
2013+
/* Always take RTNL first to prevent circular lock dependency */
2014+
rtnl_lock();
2015+
mutex_lock(&adapter->crit_lock);
2016+
2017+
if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES) &&
2018+
adapter->netdev_registered &&
2019+
!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
2020+
netdev_update_features(adapter->netdev);
2021+
adapter->flags &= ~IAVF_FLAG_SETUP_NETDEV_FEATURES;
2022+
}
2023+
2024+
switch (adapter->state) {
2025+
case __IAVF_DOWN:
2026+
if (!adapter->netdev_registered) {
2027+
err = register_netdevice(adapter->netdev);
2028+
if (err) {
2029+
dev_err(&adapter->pdev->dev, "Unable to register netdev (%d)\n",
2030+
err);
2031+
2032+
/* go back and try again.*/
2033+
iavf_free_rss(adapter);
2034+
iavf_free_misc_irq(adapter);
2035+
iavf_reset_interrupt_capability(adapter);
2036+
iavf_change_state(adapter,
2037+
__IAVF_INIT_CONFIG_ADAPTER);
2038+
goto out;
2039+
}
2040+
adapter->netdev_registered = true;
2041+
}
2042+
2043+
/* Set the real number of queues when reset occurs while
2044+
* state == __IAVF_DOWN
2045+
*/
2046+
fallthrough;
2047+
case __IAVF_RUNNING:
2048+
pairs = adapter->num_active_queues;
2049+
netif_set_real_num_rx_queues(adapter->netdev, pairs);
2050+
netif_set_real_num_tx_queues(adapter->netdev, pairs);
2051+
break;
2052+
2053+
default:
2054+
break;
2055+
}
2056+
2057+
out:
2058+
mutex_unlock(&adapter->crit_lock);
2059+
rtnl_unlock();
2060+
}
2061+
2062+
/**
2063+
* iavf_schedule_finish_config - Set the flags and schedule a reset event
2064+
* @adapter: board private structure
2065+
**/
2066+
void iavf_schedule_finish_config(struct iavf_adapter *adapter)
2067+
{
2068+
if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
2069+
queue_work(adapter->wq, &adapter->finish_config);
2070+
}
2071+
20022072
/**
20032073
* iavf_process_aq_command - process aq_required flags
20042074
* and sends aq command
@@ -2636,22 +2706,8 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
26362706

26372707
netif_carrier_off(netdev);
26382708
adapter->link_up = false;
2639-
2640-
/* set the semaphore to prevent any callbacks after device registration
2641-
* up to time when state of driver will be set to __IAVF_DOWN
2642-
*/
2643-
rtnl_lock();
2644-
if (!adapter->netdev_registered) {
2645-
err = register_netdevice(netdev);
2646-
if (err) {
2647-
rtnl_unlock();
2648-
goto err_register;
2649-
}
2650-
}
2651-
2652-
adapter->netdev_registered = true;
2653-
26542709
netif_tx_stop_all_queues(netdev);
2710+
26552711
if (CLIENT_ALLOWED(adapter)) {
26562712
err = iavf_lan_add_device(adapter);
26572713
if (err)
@@ -2664,7 +2720,6 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
26642720

26652721
iavf_change_state(adapter, __IAVF_DOWN);
26662722
set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
2667-
rtnl_unlock();
26682723

26692724
iavf_misc_irq_enable(adapter);
26702725
wake_up(&adapter->down_waitqueue);
@@ -2684,10 +2739,11 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
26842739
/* request initial VLAN offload settings */
26852740
iavf_set_vlan_offload_features(adapter, 0, netdev->features);
26862741

2742+
iavf_schedule_finish_config(adapter);
26872743
return;
2744+
26882745
err_mem:
26892746
iavf_free_rss(adapter);
2690-
err_register:
26912747
iavf_free_misc_irq(adapter);
26922748
err_sw_init:
26932749
iavf_reset_interrupt_capability(adapter);
@@ -2714,15 +2770,6 @@ static void iavf_watchdog_task(struct work_struct *work)
27142770
goto restart_watchdog;
27152771
}
27162772

2717-
if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES) &&
2718-
adapter->netdev_registered &&
2719-
!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section) &&
2720-
rtnl_trylock()) {
2721-
netdev_update_features(adapter->netdev);
2722-
rtnl_unlock();
2723-
adapter->flags &= ~IAVF_FLAG_SETUP_NETDEV_FEATURES;
2724-
}
2725-
27262773
if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
27272774
iavf_change_state(adapter, __IAVF_COMM_FAILED);
27282775

@@ -4944,6 +4991,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
49444991

49454992
INIT_WORK(&adapter->reset_task, iavf_reset_task);
49464993
INIT_WORK(&adapter->adminq_task, iavf_adminq_task);
4994+
INIT_WORK(&adapter->finish_config, iavf_finish_config);
49474995
INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task);
49484996
INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task);
49494997
queue_delayed_work(adapter->wq, &adapter->watchdog_task,
@@ -5087,13 +5135,15 @@ static void iavf_remove(struct pci_dev *pdev)
50875135
usleep_range(500, 1000);
50885136
}
50895137
cancel_delayed_work_sync(&adapter->watchdog_task);
5138+
cancel_work_sync(&adapter->finish_config);
50905139

5140+
rtnl_lock();
50915141
if (adapter->netdev_registered) {
5092-
rtnl_lock();
50935142
unregister_netdevice(netdev);
50945143
adapter->netdev_registered = false;
5095-
rtnl_unlock();
50965144
}
5145+
rtnl_unlock();
5146+
50975147
if (CLIENT_ALLOWED(adapter)) {
50985148
err = iavf_lan_del_device(adapter);
50995149
if (err)

‎drivers/net/ethernet/intel/iavf/iavf_virtchnl.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2226,6 +2226,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
22262226

22272227
iavf_process_config(adapter);
22282228
adapter->flags |= IAVF_FLAG_SETUP_NETDEV_FEATURES;
2229+
iavf_schedule_finish_config(adapter);
22292230

22302231
iavf_set_queue_vlan_tag_loc(adapter);
22312232

0 commit comments

Comments
 (0)