> -----Original Message-----
> From: Paul Menzel <[email protected]>
> Sent: Tuesday, October 15, 2024 12:33 PM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: [email protected]; Nguyen, Anthony L
> <[email protected]>; [email protected]
> 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 reply. Just a note at the beginning, that your
> mailer
> seems to wrap lines without preserving the right citation level. It’d
> be
> great if you used a mailer following standards. (I know, it’s hard in
> a
> corporate environment, but other Intel developers seem to have found
> solutions for this.)
>
>
> Am 15.10.24 um 11:45 schrieb Loktionov, Aleksandr:
>
> >> -----Original Message-----
> >> From: Paul Menzel <[email protected]>
> >> Sent: Tuesday, October 15, 2024 10:24 AM
>
> >> Thank you for your patch. For the summary I’d make it more about
> the
> >> action of the patch like “Add intermediate filter to …”.
> >
> > 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.
>
> In my opinion, having the log in the body is good enough for search
> engines and the summary should be optimized for `git log --oneline`
> consumption. I am sorry about the confusion with my example. Maybe:
>
> Add intermediate sync state to fix race condition
>
I just wonder why do you insist on "ADD" which usually means implementing a new
feature.
When this patch FIXes the bug? If I'd want to add a new feature I'd rather send
my patch to net-next, isn't it?
For me 'fix race condition by adding filter's intermediate sync state '
Can you explain your strong argument for the Add?
> >> 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.
>
> Could you please ask to get clearance to publish it. My naive view
> wonders, why legal(?) should oppose publication.
Simply the defect can come from development tools or from external customer.
And being tract as a commercial secret, which even doesn't belong to Intel
sometimes.
>
> >>> 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.
> Thank you. For me it’s not so obvious from the diff, and indeed, it’s
> done in `i40e_sync_vsi_filters()`. Thank you again.
>
> >>> Fixes: 278e7d0b9d68 ("i40e: store MAC/VLAN filters in a hash with
> the MAC Address as key")
> >>> Signed-off-by: Aleksandr Loktionov <[email protected]>
> >>> ---
> >>> 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"?
>
> Apparently not, otherwise I wouldn’t ask. ;-)
>
> > Can you provide me example commit message what I should follow?
>
> There are people reading the code not familiar with the ecosystem,
> that
> there is an out of tree driver fore example. So the code or the commit
> message should have an explanation why `I40E_FILTER_NEW_SYNC = 6` and
> what the reserved is used for.
>
> >>> + I40E_FILTER_NEW_SYNC = 6, /* New, not sent, in sync task */
>
> Also mention the hash list in the comment?
>
> >>> /* 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);
> Thank you again for your work and explanations.
>
>
> Kind regards,
>
> Paul
