[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-04-20

[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-04-20

[Tony Nguyen]

This series contains updates to iavf and i40e drivers.

Michal adds a check for down states before changing ring parameters to
avoid possible corruption for iavf.

Xiaomeng Tong prevents possible use of incorrect 'ch' variable by
introducing an interim iterator variable for i40e.

The following are changes since commit 5e6242151d7f17b056a82ca7b860c4ec8eaa7589:
  selftests: mlxsw: vxlan_flooding_ipv6: Prevent flooding of unwanted packets
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

Michal Maloszewski (1):
  iavf: Fix error when changing ring parameters on ice PF

Xiaomeng Tong (1):
  i40e: i40e_main: fix a missing check on list iterator

 drivers/net/ethernet/intel/i40e/i40e_main.c   | 27 ++++++++++---------
 .../net/ethernet/intel/iavf/iavf_ethtool.c    |  5 ++++
 2 files changed, 19 insertions(+), 13 deletions(-)

-- 
2.31.1

[PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

[Tony Nguyen]

From: Michal Maloszewski <michal.maloszewski@intel.com>

Reset is triggered when ring parameters are being changed through
ethtool and queues are reconfigured for VF's VSI. If ring is changed
again immediately, then the next reset could be executed before
queues could be properly reinitialized on VF's VSI. It caused ice PF
to mess up the VSI resource tree.

Add a check in iavf_set_ringparam for adapter and VF's queue
state. If VF is currently resetting or queues are disabled for the VF
return with EAGAIN error.

Fixes: d732a1844507 ("i40evf: fix crash when changing ring sizes")
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Michal Maloszewski <michal.maloszewski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index 3bb56714beb0..08efbc50fbe9 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -631,6 +631,11 @@ static int iavf_set_ringparam(struct net_device *netdev,
 	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
 		return -EINVAL;
 
+	if (adapter->state == __IAVF_RESETTING ||
+	    (adapter->state == __IAVF_RUNNING &&
+	     (adapter->flags & IAVF_FLAG_QUEUES_DISABLED)))
+		return -EAGAIN;
+
 	if (ring->tx_pending > IAVF_MAX_TXD ||
 	    ring->tx_pending < IAVF_MIN_TXD ||
 	    ring->rx_pending > IAVF_MAX_RXD ||
-- 
2.31.1

[PATCH net 2/2] i40e: i40e_main: fix a missing check on list iterator

[Tony Nguyen]

From: Xiaomeng Tong <xiam0nd.tong@gmail.com>

The bug is here:
	ret = i40e_add_macvlan_filter(hw, ch->seid, vdev->dev_addr, &aq_err);

The list iterator 'ch' will point to a bogus position containing
HEAD if the list is empty or no element is found. This case must
be checked before any use of the iterator, otherwise it will
lead to a invalid memory access.

To fix this bug, use a new variable 'iter' as the list iterator,
while use the origin variable 'ch' as a dedicated pointer to
point to the found element.

Cc: stable@vger.kernel.org
Fixes: 1d8d80b4e4ff6 ("i40e: Add macvlan support on i40e")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 27 +++++++++++----------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 6778df2177a1..98871f014994 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -7549,42 +7549,43 @@ static void i40e_free_macvlan_channels(struct i40e_vsi *vsi)
 static int i40e_fwd_ring_up(struct i40e_vsi *vsi, struct net_device *vdev,
 			    struct i40e_fwd_adapter *fwd)
 {
+	struct i40e_channel *ch = NULL, *ch_tmp, *iter;
 	int ret = 0, num_tc = 1,  i, aq_err;
-	struct i40e_channel *ch, *ch_tmp;
 	struct i40e_pf *pf = vsi->back;
 	struct i40e_hw *hw = &pf->hw;
 
-	if (list_empty(&vsi->macvlan_list))
-		return -EINVAL;
-
 	/* Go through the list and find an available channel */
-	list_for_each_entry_safe(ch, ch_tmp, &vsi->macvlan_list, list) {
-		if (!i40e_is_channel_macvlan(ch)) {
-			ch->fwd = fwd;
+	list_for_each_entry_safe(iter, ch_tmp, &vsi->macvlan_list, list) {
+		if (!i40e_is_channel_macvlan(iter)) {
+			iter->fwd = fwd;
 			/* record configuration for macvlan interface in vdev */
 			for (i = 0; i < num_tc; i++)
 				netdev_bind_sb_channel_queue(vsi->netdev, vdev,
 							     i,
-							     ch->num_queue_pairs,
-							     ch->base_queue);
-			for (i = 0; i < ch->num_queue_pairs; i++) {
+							     iter->num_queue_pairs,
+							     iter->base_queue);
+			for (i = 0; i < iter->num_queue_pairs; i++) {
 				struct i40e_ring *tx_ring, *rx_ring;
 				u16 pf_q;
 
-				pf_q = ch->base_queue + i;
+				pf_q = iter->base_queue + i;
 
 				/* Get to TX ring ptr */
 				tx_ring = vsi->tx_rings[pf_q];
-				tx_ring->ch = ch;
+				tx_ring->ch = iter;
 
 				/* Get the RX ring ptr */
 				rx_ring = vsi->rx_rings[pf_q];
-				rx_ring->ch = ch;
+				rx_ring->ch = iter;
 			}
+			ch = iter;
 			break;
 		}
 	}
 
+	if (!ch)
+		return -EINVAL;
+
 	/* Guarantee all rings are updated before we update the
 	 * MAC address filter.
 	 */
-- 
2.31.1

Re: [PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

[Jakub Kicinski]

On Wed, 20 Apr 2022 10:26:23 -0700 Tony Nguyen wrote:
> From: Michal Maloszewski <michal.maloszewski@intel.com>
> 
> Reset is triggered when ring parameters are being changed through
> ethtool and queues are reconfigured for VF's VSI. If ring is changed
> again immediately, then the next reset could be executed before
> queues could be properly reinitialized on VF's VSI. It caused ice PF
> to mess up the VSI resource tree.
> 
> Add a check in iavf_set_ringparam for adapter and VF's queue
> state. If VF is currently resetting or queues are disabled for the VF
> return with EAGAIN error.

Can't we wait for the device to get into the right state?
Throwing EAGAIN back to user space is not very friendly.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 3bb56714beb0..08efbc50fbe9 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -631,6 +631,11 @@ static int iavf_set_ringparam(struct net_device *netdev,
>  	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
>  		return -EINVAL;
>  
> +	if (adapter->state == __IAVF_RESETTING ||
> +	    (adapter->state == __IAVF_RUNNING &&
> +	     (adapter->flags & IAVF_FLAG_QUEUES_DISABLED)))
> +		return -EAGAIN;

nit: why add this check in the middle of input validation 
(i.e. checking the ring params are supported)?

>  	if (ring->tx_pending > IAVF_MAX_TXD ||
>  	    ring->tx_pending < IAVF_MIN_TXD ||
>  	    ring->rx_pending > IAVF_MAX_RXD ||

RE: [PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

[Maloszewski, Michal]

Hi,

>Can't we wait for the device to get into the right state?
>Throwing EAGAIN back to user space is not very friendly.

When we have state which is running, it does not mean that we have queues configured on PF.  
So in order to configure queues on PF, the IAVF_FLAG_QUEUES has to be disabled.      
I use here EAGAIN, because as long as we are not configured with queues, it does not make any sense to trigger command and we are not sure when the configuration of queues will end - so that is why EAGAIN is used.
 
>nit: why add this check in the middle of input validation (i.e. checking the ring params are supported)?

You are right here, I changed the order of 'if' statements.

Thanks,
Michał Małoszewski

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org> 
Sent: Saturday, April 23, 2022 12:48 AM
To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
Cc: davem@davemloft.net; pabeni@redhat.com; Maloszewski, Michal <michal.maloszewski@intel.com>; netdev@vger.kernel.org; sassmann@redhat.com; Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>; Jankowski, Konrad0 <konrad0.jankowski@intel.com>
Subject: Re: [PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

On Wed, 20 Apr 2022 10:26:23 -0700 Tony Nguyen wrote:
> From: Michal Maloszewski <michal.maloszewski@intel.com>
> 
> Reset is triggered when ring parameters are being changed through 
> ethtool and queues are reconfigured for VF's VSI. If ring is changed 
> again immediately, then the next reset could be executed before queues 
> could be properly reinitialized on VF's VSI. It caused ice PF to mess 
> up the VSI resource tree.
> 
> Add a check in iavf_set_ringparam for adapter and VF's queue state. If 
> VF is currently resetting or queues are disabled for the VF return 
> with EAGAIN error.

Can't we wait for the device to get into the right state?
Throwing EAGAIN back to user space is not very friendly.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c 
> b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 3bb56714beb0..08efbc50fbe9 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -631,6 +631,11 @@ static int iavf_set_ringparam(struct net_device *netdev,
>  	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
>  		return -EINVAL;
>  
> +	if (adapter->state == __IAVF_RESETTING ||
> +	    (adapter->state == __IAVF_RUNNING &&
> +	     (adapter->flags & IAVF_FLAG_QUEUES_DISABLED)))
> +		return -EAGAIN;

nit: why add this check in the middle of input validation (i.e. checking the ring params are supported)?

>  	if (ring->tx_pending > IAVF_MAX_TXD ||
>  	    ring->tx_pending < IAVF_MIN_TXD ||
>  	    ring->rx_pending > IAVF_MAX_RXD ||

Re: [PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

[Jakub Kicinski]

On Thu, 28 Apr 2022 18:10:49 +0000 Maloszewski, Michal wrote:
>> Can't we wait for the device to get into the right state?
>> Throwing EAGAIN back to user space is not very friendly.  
> 
> When we have state which is running, it does not mean that we have
> queues configured on PF. So in order to configure queues on PF, the
> IAVF_FLAG_QUEUES has to be disabled. I use here EAGAIN, because as
> long as we are not configured with queues, it does not make any sense
> to trigger command and we are not sure when the configuration of
> queues will end - so that is why EAGAIN is used.

Let me rephrase the question. Does getting out of the state where error
is reported require user to change something, or is it something that
happens automatically behind the scenes (i.e. the state is transient).

RE: [PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

[Maloszewski, Michal]

>On Thu, 28 Apr 2022 18:10:49 +0000 Maloszewski, Michal wrote:
>>> Can't we wait for the device to get into the right state?
>>> Throwing EAGAIN back to user space is not very friendly.  
>> 
>> When we have state which is running, it does not mean that we have 
>> queues configured on PF. So in order to configure queues on PF, the 
>> IAVF_FLAG_QUEUES has to be disabled. I use here EAGAIN, because as 
>> long as we are not configured with queues, it does not make any sense 
>> to trigger command and we are not sure when the configuration of 
>> queues will end - so that is why EAGAIN is used.
>
>Let me rephrase the question. Does getting out of the state where error is
>reported require user to change something, or is it something that happens
>automatically behind the scenes (i.e. the state is transient).

It is something that happens automatically behind the scenes. 
It takes some time and there is no guarantee that it will be finished.

Re: [PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

[Jakub Kicinski]

On Fri, 29 Apr 2022 13:14:12 +0000 Maloszewski, Michal wrote:
> >> When we have state which is running, it does not mean that we have 
> >> queues configured on PF. So in order to configure queues on PF, the 
> >> IAVF_FLAG_QUEUES has to be disabled. I use here EAGAIN, because as 
> >> long as we are not configured with queues, it does not make any sense 
> >> to trigger command and we are not sure when the configuration of 
> >> queues will end - so that is why EAGAIN is used.  
> >
> >Let me rephrase the question. Does getting out of the state where error is
> >reported require user to change something, or is it something that happens
> >automatically behind the scenes (i.e. the state is transient).  
> 
> It is something that happens automatically behind the scenes. 
> It takes some time and there is no guarantee that it will be finished.

Then either wait for it to finish (wait queue or such) or if
possible record the desired config and return success. Apply
the new config when the reset is finished.

It really seems like you're making users pay the price for poor
design of the driver's internals.

RE: [PATCH net 1/2] iavf: Fix error when changing ring parameters on ice PF

[Maloszewski, Michal]

>On Fri, 29 Apr 2022 13:14:12 +0000 Maloszewski, Michal wrote:
>> >> When we have state which is running, it does not mean that we have 
>> >> queues configured on PF. So in order to configure queues on PF, the 
>> >> IAVF_FLAG_QUEUES has to be disabled. I use here EAGAIN, because as 
>> >> long as we are not configured with queues, it does not make any 
>> >> sense to trigger command and we are not sure when the configuration 
>> >> of queues will end - so that is why EAGAIN is used.
>> >
>> >Let me rephrase the question. Does getting out of the state where 
>> >error is reported require user to change something, or is it 
>> >something that happens automatically behind the scenes (i.e. the state is transient).
>> 
>> It is something that happens automatically behind the scenes. 
>> It takes some time and there is no guarantee that it will be finished.
>
>Then either wait for it to finish (wait queue or such) or if possible record the desired config and return success. Apply the new config when the reset is finished.
>
>It really seems like you're making users pay the price for poor design of the driver's internals.

It is good idea. We had doubts if we could do that. I will try to fix it asap.