[Intel-wired-lan] [PATCH] ice: double lock on adev_mutex

[Intel-wired-lan] [PATCH] ice: double lock on adev_mutex

[Michal Swiatkowski]

Call trace:
ice_set_channel() --> mutex_lock(adev_mutex)
ice_vsi_recfg_qs()
ice_pf_dcb_recfg()
ice_send_event_to_aux() --> another mutex_lock(adev_mutex)

This call trace is reached when user try to change queues amount using
ethtool:
$ ethtool -L enp24s0f0np0 tx 64 rx 32

Avoid double lock by unlocking after checking if RDMA is active.

Fixes: a49a2713f00e ("ice: Prevent set_channel from changing queues while RDMA active")
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 263d59929602..54fc2ca823f5 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3729,22 +3729,21 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 	mutex_lock(&pf->adev_mutex);
 	if (pf->adev && pf->adev->dev.driver) {
 		netdev_err(dev, "Cannot change channels when RDMA is active\n");
-		ret = -EINVAL;
-		goto adev_unlock;
+		mutex_unlock(&pf->adev_mutex);
+		return -EINVAL;
 	}
+	mutex_unlock(&pf->adev_mutex);
 
 	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
 
 	if (!netif_is_rxfh_configured(dev)) {
 		ret = ice_vsi_set_dflt_rss_lut(vsi, new_rx);
-		goto adev_unlock;
+		return ret;
 	}
 
 	/* Update rss_size due to change in Rx queues */
 	vsi->rss_size = ice_get_valid_rss_size(&pf->hw, new_rx);
 
-adev_unlock:
-	mutex_unlock(&pf->adev_mutex);
 	return ret;
 }
 
-- 
2.36.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH] ice: double lock on adev_mutex

[Jesse Brandeburg]

On 1/11/2023 3:01 AM, Michal Swiatkowski wrote:
 > Re: [Intel-wired-lan] [PATCH] ice: double lock on adev_mutex

Looks good but title should always contain PATCH net-next or PATCH net. 
Also doesn't hurt especially for net fixes to use --base net/master 
(assuming your remote for netdev net tree is 'net')


> Call trace:
> ice_set_channel() --> mutex_lock(adev_mutex)
> ice_vsi_recfg_qs()
> ice_pf_dcb_recfg()
> ice_send_event_to_aux() --> another mutex_lock(adev_mutex)

Ouch, didn't lockdep identify this? Let's hope it would find it if we 
turned it on.

> 
> This call trace is reached when user try to change queues amount using
> ethtool:
> $ ethtool -L enp24s0f0np0 tx 64 rx 32
> 
> Avoid double lock by unlocking after checking if RDMA is active.
> 
> Fixes: a49a2713f00e ("ice: Prevent set_channel from changing queues while RDMA active")
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

thanks for fixing this!

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH] ice: double lock on adev_mutex

[Michal Swiatkowski]

On Thu, Jan 12, 2023 at 05:11:04PM -0800, Jesse Brandeburg wrote:
> On 1/11/2023 3:01 AM, Michal Swiatkowski wrote:
> > Re: [Intel-wired-lan] [PATCH] ice: double lock on adev_mutex
> 
> Looks good but title should always contain PATCH net-next or PATCH net. Also
> doesn't hurt especially for net fixes to use --base net/master (assuming
> your remote for netdev net tree is 'net')
> 
> 

Sure, sorry, I forgot to add net tag, it is in v2. Will remember,
thanks.

> > Call trace:
> > ice_set_channel() --> mutex_lock(adev_mutex)
> > ice_vsi_recfg_qs()
> > ice_pf_dcb_recfg()
> > ice_send_event_to_aux() --> another mutex_lock(adev_mutex)
> 
> Ouch, didn't lockdep identify this? Let's hope it would find it if we turned
> it on.
> 

I haven't got lockdep compiled into my kernel, probably I should start
adding it.

> > 
> > This call trace is reached when user try to change queues amount using
> > ethtool:
> > $ ethtool -L enp24s0f0np0 tx 64 rx 32
> > 
> > Avoid double lock by unlocking after checking if RDMA is active.
> > 
> > Fixes: a49a2713f00e ("ice: Prevent set_channel from changing queues while RDMA active")
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> 
> thanks for fixing this!
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH] ice: double lock on adev_mutex

[G, GurucharanX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Michal Swiatkowski
> Sent: Wednesday, January 11, 2023 4:31 PM
> To: intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH] ice: double lock on adev_mutex
> 
> Call trace:
> ice_set_channel() --> mutex_lock(adev_mutex)
> ice_vsi_recfg_qs()
> ice_pf_dcb_recfg()
> ice_send_event_to_aux() --> another mutex_lock(adev_mutex)
> 
> This call trace is reached when user try to change queues amount using
> ethtool:
> $ ethtool -L enp24s0f0np0 tx 64 rx 32
> 
> Avoid double lock by unlocking after checking if RDMA is active.
> 
> Fixes: a49a2713f00e ("ice: Prevent set_channel from changing queues while
> RDMA active")
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 

Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan