Skip to content

Commit d1639a1

Browse files
azaki1anguy11
azaki1
authored and
anguy11
committed
iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies
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]>
1 parent d916d27 commit d1639a1

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
@@ -255,6 +255,7 @@ struct iavf_adapter {
255255
struct workqueue_struct *wq;
256256
struct work_struct reset_task;
257257
struct work_struct adminq_task;
258+
struct work_struct finish_config;
258259
struct delayed_work client_task;
259260
wait_queue_head_t down_waitqueue;
260261
wait_queue_head_t reset_waitqueue;
@@ -521,6 +522,7 @@ int iavf_process_config(struct iavf_adapter *adapter);
521522
int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter);
522523
void iavf_schedule_reset(struct iavf_adapter *adapter);
523524
void iavf_schedule_request_stats(struct iavf_adapter *adapter);
525+
void iavf_schedule_finish_config(struct iavf_adapter *adapter);
524526
void iavf_reset(struct iavf_adapter *adapter);
525527
void iavf_set_ethtool_ops(struct net_device *netdev);
526528
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
@@ -1690,10 +1690,10 @@ static int iavf_set_interrupt_capability(struct iavf_adapter *adapter)
16901690
adapter->msix_entries[vector].entry = vector;
16911691

16921692
err = iavf_acquire_msix_vectors(adapter, v_budget);
1693+
if (!err)
1694+
iavf_schedule_finish_config(adapter);
16931695

16941696
out:
1695-
netif_set_real_num_rx_queues(adapter->netdev, pairs);
1696-
netif_set_real_num_tx_queues(adapter->netdev, pairs);
16971697
return err;
16981698
}
16991699

@@ -1913,9 +1913,7 @@ static int iavf_init_interrupt_scheme(struct iavf_adapter *adapter)
19131913
goto err_alloc_queues;
19141914
}
19151915

1916-
rtnl_lock();
19171916
err = iavf_set_interrupt_capability(adapter);
1918-
rtnl_unlock();
19191917
if (err) {
19201918
dev_err(&adapter->pdev->dev,
19211919
"Unable to setup interrupt capabilities\n");
@@ -2001,6 +1999,78 @@ static int iavf_reinit_interrupt_scheme(struct iavf_adapter *adapter, bool runni
20011999
return err;
20022000
}
20032001

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

26392709
netif_carrier_off(netdev);
26402710
adapter->link_up = false;
2641-
2642-
/* set the semaphore to prevent any callbacks after device registration
2643-
* up to time when state of driver will be set to __IAVF_DOWN
2644-
*/
2645-
rtnl_lock();
2646-
if (!adapter->netdev_registered) {
2647-
err = register_netdevice(netdev);
2648-
if (err) {
2649-
rtnl_unlock();
2650-
goto err_register;
2651-
}
2652-
}
2653-
2654-
adapter->netdev_registered = true;
2655-
26562711
netif_tx_stop_all_queues(netdev);
2712+
26572713
if (CLIENT_ALLOWED(adapter)) {
26582714
err = iavf_lan_add_device(adapter);
26592715
if (err)
@@ -2666,7 +2722,6 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
26662722

26672723
iavf_change_state(adapter, __IAVF_DOWN);
26682724
set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
2669-
rtnl_unlock();
26702725

26712726
iavf_misc_irq_enable(adapter);
26722727
wake_up(&adapter->down_waitqueue);
@@ -2686,10 +2741,11 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
26862741
/* request initial VLAN offload settings */
26872742
iavf_set_vlan_offload_features(adapter, 0, netdev->features);
26882743

2744+
iavf_schedule_finish_config(adapter);
26892745
return;
2746+
26902747
err_mem:
26912748
iavf_free_rss(adapter);
2692-
err_register:
26932749
iavf_free_misc_irq(adapter);
26942750
err_sw_init:
26952751
iavf_reset_interrupt_capability(adapter);
@@ -2716,15 +2772,6 @@ static void iavf_watchdog_task(struct work_struct *work)
27162772
goto restart_watchdog;
27172773
}
27182774

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

@@ -4942,6 +4989,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
49424989

49434990
INIT_WORK(&adapter->reset_task, iavf_reset_task);
49444991
INIT_WORK(&adapter->adminq_task, iavf_adminq_task);
4992+
INIT_WORK(&adapter->finish_config, iavf_finish_config);
49454993
INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task);
49464994
INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task);
49474995
queue_delayed_work(adapter->wq, &adapter->watchdog_task,
@@ -5084,13 +5132,15 @@ static void iavf_remove(struct pci_dev *pdev)
50845132
usleep_range(500, 1000);
50855133
}
50865134
cancel_delayed_work_sync(&adapter->watchdog_task);
5135+
cancel_work_sync(&adapter->finish_config);
50875136

5137+
rtnl_lock();
50885138
if (adapter->netdev_registered) {
5089-
rtnl_lock();
50905139
unregister_netdevice(netdev);
50915140
adapter->netdev_registered = false;
5092-
rtnl_unlock();
50935141
}
5142+
rtnl_unlock();
5143+
50945144
if (CLIENT_ALLOWED(adapter)) {
50955145
err = iavf_lan_del_device(adapter);
50965146
if (err)

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

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

22382238
iavf_process_config(adapter);
22392239
adapter->flags |= IAVF_FLAG_SETUP_NETDEV_FEATURES;
2240+
iavf_schedule_finish_config(adapter);
22402241

22412242
iavf_set_queue_vlan_tag_loc(adapter);
22422243

0 commit comments

Comments
 (0)