Message-ID: <SJ0PR11MB586684C1B9995B605D83CF71E5452@SJ0PR11MB5866.namprd11.prod.outlook.com>
Date: Tue, 15 Oct 2024 09:45:45 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix "Error
 I40E_AQ_RC_ENOSPC adding RX filters on VF" issue



> -----Original Message-----
> From: Paul Menzel <pmenzel@...gen.mpg.de>
> Sent: Tuesday, October 15, 2024 10:24 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix "Error
> I40E_AQ_RC_ENOSPC adding RX filters on VF" issue
> 
> Dear Aleksandr,
> 
> 
> Thank you for your patch. For the summary I’d make it more about the
> action of the patch like “Add intermediate filter to …”.
> 
Good day Paul
Sorry, I don't get your point. This patch fixes bug that can stop vfs to receive any traffic making them useless.
The first and most visible effect of the bug is a lot of "Error I40E_AQ_RC_ENOSPC adding RX filters on VF XX,..." errors from F/W complaining to add MAC/VLAN filter. So I've mentioned it in the title to be easy found.
I don't add any filter in the driver, we have to add one more intermediate state of the filter to avoid the race condition.

> 
> Am 15.10.24 um 09:04 schrieb Aleksandr Loktionov:
> > Fix a race condition in the i40e driver that leads to MAC/VLAN
> filters
> > becoming corrupted and leaking. Address the issue that occurs under
> > heavy load when multiple threads are concurrently modifying MAC/VLAN
> > filters by setting mac and port VLAN.
> >
> > 1. Thread T0 allocates a filter in i40e_add_filter() within
> >          i40e_ndo_set_vf_port_vlan().
> > 2. Thread T1 concurrently frees the filter in __i40e_del_filter()
> within
> >          i40e_ndo_set_vf_mac().
> > 3. Subsequently, i40e_service_task() calls i40e_sync_vsi_filters(),
> which
> >          refers to the already freed filter memory, causing
> corruption.
> >
> > Reproduction steps:
> > 1. Spawn multiple VFs.
> > 2. Apply a concurrent heavy load by running parallel operations to
> change
> >          MAC addresses on the VFs and change port VLANs on the host.
> 
> It’d be great if you shared your commands.
Sorry, I'm pretty sure it's quite impossible to reproduce the issue with bash scripts /*I've tried really hard*/. Reproduction is related to user-space/kernel code which might be not open-sourced. So as I've explained in the commit title the race condition possibility that was introduced from the very beginning.

> 
> > 3. Observe errors in dmesg:
> > "Error I40E_AQ_RC_ENOSPC adding RX filters on VF XX,
> >   please set promiscuous on manually for VF XX".
> 
> I’d indent it by eight spaces and put it in one line.
Ok, I'll fix it in v2

> > The fix involves implementing a new intermediate filter state,
> > I40E_FILTER_NEW_SYNC, for the time when a filter is on a
> tmp_add_list.
> > These filters cannot be deleted from the hash list directly but
> > must be removed using the full process.
> 
> Please excuse my ignorance. Where is that done in the diff? For me it
> looks like you only replace `I40E_FILTER_NEW` by
> `I40E_FILTER_NEW_SYNC`
> in certain places, but no new condition for this case.
> 
Here are below the code which adds new I40E_FILTER_NEW_SYNC enum.
And additional conditions for this I40E_FILTER_NEW_SYNC state.
All other places in the driver just tract I40E_FILTER_NEW_SYNC as not just I40E_FILTER_NEW by default. 

> > Fixes: 278e7d0b9d68 ("i40e: store MAC/VLAN filters in a hash with
> the MAC Address as key")
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> > ---
> >   drivers/net/ethernet/intel/i40e/i40e.h         |  2 ++
> >   drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  2 ++
> >   drivers/net/ethernet/intel/i40e/i40e_main.c    | 12 ++++++++++--
> >   3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
> b/drivers/net/ethernet/intel/i40e/i40e.h
> > index 2089a0e..a1842dd 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e.h
> > +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> > @@ -755,6 +755,8 @@ enum i40e_filter_state {
> >   	I40E_FILTER_ACTIVE,		/* Added to switch by FW */
> >   	I40E_FILTER_FAILED,		/* Rejected by FW */
> >   	I40E_FILTER_REMOVE,		/* To be removed */
> > +	/* RESERVED */
> 
> Why the reserved comment? Please elaborate in the commit message.
This is for not breaking compatibility with different driver versions.
Between OOT, net-next and plain old net. Isn't it obvious from the comment, it's "RESERVERD"?
Can you provide me example commit message what I should follow?

> > +	I40E_FILTER_NEW_SYNC = 6,	/* New, not sent, in sync task */
> >   /* There is no 'removed' state; the filter struct is freed */
> >   };
> >   struct i40e_mac_filter {
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > index abf624d..1c439b1 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> > @@ -89,6 +89,8 @@ static char *i40e_filter_state_string[] = {
> >   	"ACTIVE",
> >   	"FAILED",
> >   	"REMOVE",
> > +	"<RESERVED>",
> > +	"NEW_SYNC",
> >   };
> >
> >   /**
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index 25295ae..55fb362 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -1255,6 +1255,7 @@ int i40e_count_filters(struct i40e_vsi *vsi)
> >
> >   	hash_for_each_safe(vsi->mac_filter_hash, bkt, h, f, hlist) {
> >   		if (f->state == I40E_FILTER_NEW ||
> > +		    f->state == I40E_FILTER_NEW_SYNC ||
> >   		    f->state == I40E_FILTER_ACTIVE)
> >   			++cnt;
> >   	}
> > @@ -1441,6 +1442,8 @@ static int
> i40e_correct_mac_vlan_filters(struct i40e_vsi *vsi,
> >
> >   			new->f = add_head;
> >   			new->state = add_head->state;
> > +			if (add_head->state == I40E_FILTER_NEW)
> > +				add_head->state = I40E_FILTER_NEW_SYNC;
> >
> >   			/* Add the new filter to the tmp list */
> >   			hlist_add_head(&new->hlist, tmp_add_list);
> > @@ -1550,6 +1553,8 @@ static int
> i40e_correct_vf_mac_vlan_filters(struct i40e_vsi *vsi,
> >   				return -ENOMEM;
> >   			new_mac->f = add_head;
> >   			new_mac->state = add_head->state;
> > +			if (add_head->state == I40E_FILTER_NEW)
> > +				add_head->state = I40E_FILTER_NEW_SYNC;
> >
> >   			/* Add the new filter to the tmp list */
> >   			hlist_add_head(&new_mac->hlist, tmp_add_list);
> > @@ -2437,7 +2442,8 @@ static int
> >   i40e_aqc_broadcast_filter(struct i40e_vsi *vsi, const char
> *vsi_name,
> >   			  struct i40e_mac_filter *f)
> >   {
> > -	bool enable = f->state == I40E_FILTER_NEW;
> > +	bool enable = f->state == I40E_FILTER_NEW ||
> > +		      f->state == I40E_FILTER_NEW_SYNC;
> >   	struct i40e_hw *hw = &vsi->back->hw;
> >   	int aq_ret;
> >
> > @@ -2611,6 +2617,7 @@ int i40e_sync_vsi_filters(struct i40e_vsi
> *vsi)
> >
> >   				/* Add it to the hash list */
> >   				hlist_add_head(&new->hlist, &tmp_add_list);
> > +				f->state = I40E_FILTER_NEW_SYNC;
> >   			}
> >
> >   			/* Count the number of active (current and new)
> VLAN
> > @@ -2762,7 +2769,8 @@ int i40e_sync_vsi_filters(struct i40e_vsi
> *vsi)
> >   		spin_lock_bh(&vsi->mac_filter_hash_lock);
> >   		hlist_for_each_entry_safe(new, h, &tmp_add_list, hlist) {
> >   			/* Only update the state if we're still NEW */
> > -			if (new->f->state == I40E_FILTER_NEW)
> > +			if (new->f->state == I40E_FILTER_NEW ||
> > +			    new->f->state == I40E_FILTER_NEW_SYNC)
> >   				new->f->state = new->state;
> >   			hlist_del(&new->hlist);
> >   			netdev_hw_addr_refcnt(new->f, vsi->netdev, -1);
> 
> 
> Kind nregards,
> 
> Paul

Powered by blists - more mailing lists