Re: [PATCH net-next v2 3/5] ice: migrate to netdev ops lock

[Kohei Enju <kohei@enjuk.jp>]

On 03/25 13:06, Tony Nguyen wrote:
> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> 
> Queue management ops unconditionally enable netdev locking. The same
> lock is taken by default by several NAPI configuration functions,
> such as napi_enable() and netif_napi_set_irq().
> Request ops locking in advance and make sure we use the _locked
> counterparts of those functions to avoid deadlocks, taking the lock
> manually where needed (suspend/resume, queue rebuild and resets).
> 

I've been testing this series and found that attaching XDP program
hangs:
    ip/1033 is trying to acquire lock:
    ffff888103d8ec30 (&dev->lock){+.+.}-{4:4}, at: xdp_features_set_redirect_target+0x1f/0x80
    
    but task is already holding lock:
    ffff888103d8ec30 (&dev->lock){+.+.}-{4:4}, at: do_setlink.isra.0+0x261/0x39a0
    
    other info that might help us debug this:
     Possible unsafe locking scenario:
    
           CPU0
           ----
      lock(&dev->lock);
      lock(&dev->lock);
    
     *** DEADLOCK ***
    
     May be due to missing lock nesting notation
    
    3 locks held by ip/1033:
     #0: ffffffff86d8b548 (rtnl_mutex){+.+.}-{4:4}, at: rtnl_newlink+0x6c4/0x22a0
     #1: ffff888103d8ec30 (&dev->lock){+.+.}-{4:4}, at: do_setlink.isra.0+0x261/0x39a0
     #2: ffff888104e1c5b8 (&vsi->xdp_state_lock){+.+.}-{4:4}, at: ice_xdp+0x96/0xef0
    
    stack backtrace:
    CPU: 1 UID: 0 PID: 1033 Comm: ip Not tainted 7.0.0-rc4-enjuk-tnguy-00940-gadae968a1b0a #316 PREEMPT(full)
    Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
    Call Trace:
     <TASK>
     dump_stack_lvl+0x6f/0xb0
     print_deadlock_bug.cold+0xc0/0xce
     __lock_acquire+0x123d/0x1b90
     lock_acquire+0x192/0x310
     __mutex_lock+0x193/0x2860
     xdp_features_set_redirect_target+0x1f/0x80
     ice_xdp+0x64d/0xef0
     dev_xdp_install+0x3c4/0x840
     dev_xdp_attach+0x50a/0x10a0
     dev_change_xdp_fd+0x175/0x210
     do_setlink.isra.0+0x1c1b/0x39a0
    [...]


I think we should use locked versions for both
xdp_features_{set,clear}_redirect_target(), and this fixed that for me.

---
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 3477c53316ba..0be225ab9372 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3015,9 +3015,9 @@ ice_xdp_setup_prog(struct ice_vsi *vsi, struct bpf_prog *prog,
                                goto resume_if;
                        }
                }
-               xdp_features_set_redirect_target(vsi->netdev, true);
+               xdp_features_set_redirect_target_locked(vsi->netdev, true);
        } else if (ice_is_xdp_ena_vsi(vsi) && !prog) {
-               xdp_features_clear_redirect_target(vsi->netdev);
+               xdp_features_clear_redirect_target_locked(vsi->netdev);
                xdp_ring_err = ice_destroy_xdp_rings(vsi, ICE_XDP_CFG_FULL);
                if (xdp_ring_err)
                        NL_SET_ERR_MSG_MOD(extack, "Freeing XDP Tx resources failed");