Fix promiscous and multicast mode on iavf after reset

Fix promiscous and multicast mode on iavf after reset

[Lennart Sorensen]

I discovered that anything that causes a reset in iavf makes breaks
promiscous mode and multicast.  This is because the host side ice
driver clears the VF from filters when it is reset.  iavf then correctly
calls iavf_configure, but since the current_netdev_promisc_flags already
match the netdev promisc settings, nothing is done, so the promisc and
multicast settings are not sent to the ice host driver after the reset.
As a result the iavf side shows promisc enabled but it isn't working.
Disabling and re-enabling promisc on the iavf side fixes it of course.
Simple test case to show this is to enable promisc, check that packets
are being seen, then change the mtu size (which does a reset) and check
packets received again, and promisc is no longer active.  Disabling
promisc and enabling it again restores receiving the packets.

The following seems to work for me, but I am not sure it is the correct
place to clear the saved flags.

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 6d7ba4d67a19..4018a08d63c1 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3233,6 +3233,14 @@ static void iavf_reset_task(struct work_struct *work)
 		iavf_shutdown_adminq(hw);
 		iavf_init_adminq(hw);
 		iavf_request_reset(adapter);
+
+		/* Clear remembered promisc and multicast flags since
+		 * reset clears them on the host so they will get force
+		 * applied again through iavf_configure() down below.
+		 */
+		spin_lock_bh(&adapter->current_netdev_promisc_flags_lock);
+		adapter->current_netdev_promisc_flags &= ~(IFF_PROMISC | IFF_ALLMULTI);
+		spin_unlock_bh(&adapter->current_netdev_promisc_flags_lock);
 	}
 	adapter->flags |= IAVF_FLAG_RESET_PENDING;
 

-- 
Len Sorensen

Re: Fix promiscous and multicast mode on iavf after reset

[Jacob Keller]

On 4/23/2025 10:12 AM, Lennart Sorensen wrote:
> I discovered that anything that causes a reset in iavf makes breaks
> promiscous mode and multicast.  This is because the host side ice
> driver clears the VF from filters when it is reset.  iavf then correctly
> calls iavf_configure, but since the current_netdev_promisc_flags already
> match the netdev promisc settings, nothing is done, so the promisc and
> multicast settings are not sent to the ice host driver after the reset.
> As a result the iavf side shows promisc enabled but it isn't working.
> Disabling and re-enabling promisc on the iavf side fixes it of course.
> Simple test case to show this is to enable promisc, check that packets
> are being seen, then change the mtu size (which does a reset) and check
> packets received again, and promisc is no longer active.  Disabling
> promisc and enabling it again restores receiving the packets.
> 
> The following seems to work for me, but I am not sure it is the correct
> place to clear the saved flags.
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 6d7ba4d67a19..4018a08d63c1 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3233,6 +3233,14 @@ static void iavf_reset_task(struct work_struct *work)
>  		iavf_shutdown_adminq(hw);
>  		iavf_init_adminq(hw);
>  		iavf_request_reset(adapter);
> +
> +		/* Clear remembered promisc and multicast flags since
> +		 * reset clears them on the host so they will get force
> +		 * applied again through iavf_configure() down below.
> +		 */
> +		spin_lock_bh(&adapter->current_netdev_promisc_flags_lock);
> +		adapter->current_netdev_promisc_flags &= ~(IFF_PROMISC | IFF_ALLMULTI);
> +		spin_unlock_bh(&adapter->current_netdev_promisc_flags_lock);
>  	}
>  	adapter->flags |= IAVF_FLAG_RESET_PENDING;
>  
> 

We probably need to do something similar in the flow where we get an
unexpected reset (such as if PF resets us by changing trusted flag or
other state).

I don't think there's a better solution. Arguably the PF shouldn't be
losing data, but I think its a bit late to go that route at this point..
Its pretty baked into the virtchnl API :(

Re: Fix promiscous and multicast mode on iavf after reset

[Lennart Sorensen]

On Thu, Apr 24, 2025 at 02:59:38PM -0700, Jacob Keller wrote:
> 
> 
> On 4/23/2025 10:12 AM, Lennart Sorensen wrote:
> > I discovered that anything that causes a reset in iavf makes breaks
> > promiscous mode and multicast.  This is because the host side ice
> > driver clears the VF from filters when it is reset.  iavf then correctly
> > calls iavf_configure, but since the current_netdev_promisc_flags already
> > match the netdev promisc settings, nothing is done, so the promisc and
> > multicast settings are not sent to the ice host driver after the reset.
> > As a result the iavf side shows promisc enabled but it isn't working.
> > Disabling and re-enabling promisc on the iavf side fixes it of course.
> > Simple test case to show this is to enable promisc, check that packets
> > are being seen, then change the mtu size (which does a reset) and check
> > packets received again, and promisc is no longer active.  Disabling
> > promisc and enabling it again restores receiving the packets.
> > 
> > The following seems to work for me, but I am not sure it is the correct
> > place to clear the saved flags.
> > 
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index 6d7ba4d67a19..4018a08d63c1 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -3233,6 +3233,14 @@ static void iavf_reset_task(struct work_struct *work)
> >  		iavf_shutdown_adminq(hw);
> >  		iavf_init_adminq(hw);
> >  		iavf_request_reset(adapter);
> > +
> > +		/* Clear remembered promisc and multicast flags since
> > +		 * reset clears them on the host so they will get force
> > +		 * applied again through iavf_configure() down below.
> > +		 */
> > +		spin_lock_bh(&adapter->current_netdev_promisc_flags_lock);
> > +		adapter->current_netdev_promisc_flags &= ~(IFF_PROMISC | IFF_ALLMULTI);
> > +		spin_unlock_bh(&adapter->current_netdev_promisc_flags_lock);
> >  	}
> >  	adapter->flags |= IAVF_FLAG_RESET_PENDING;
> >  
> > 
> 
> We probably need to do something similar in the flow where we get an
> unexpected reset (such as if PF resets us by changing trusted flag or
> other state).
> 
> I don't think there's a better solution. Arguably the PF shouldn't be
> losing data, but I think its a bit late to go that route at this point..
> Its pretty baked into the virtchnl API :(

Yeah I can see arguments that calling reset should put everything in a
known state so the VF driver can configure things as it wants, but since
reset is also used when tx hang happens or mtu changes and various other
things, it is definitely inconvinient.  Changing behaviour with an API
version change seems ugly too and you would still have to support the
old API anyhow.  I suppose having a reset fully to defaults and a soft
reset to change settings but keep other values could have been nice,
but a bit late now.  Some VF drivers may even be depending on the reset
putting everything to defaults.

If someone that knows the driver better can make a complete fix that
would be great.  So far this small change appears to be working but I
could certainly have missed some cases.  It took quite a bit of debugging
to discover why promiscous mode on the VF side was so unreliable and
unpredicable.  Due to the somewhat asynchrounous message handling,
sometimes the reset would not happen until after the promisc setting
had been applied, and then it was silently lost, while other times it
would do the reset quicker and then the promisc setting would work.
Very confusing to debug.

-- 
Len Sorensen

Re: Fix promiscous and multicast mode on iavf after reset

[Jacob Keller]

On 4/25/2025 9:22 AM, Lennart Sorensen wrote:
> On Thu, Apr 24, 2025 at 02:59:38PM -0700, Jacob Keller wrote:
>>
>>
>> On 4/23/2025 10:12 AM, Lennart Sorensen wrote:
>>> I discovered that anything that causes a reset in iavf makes breaks
>>> promiscous mode and multicast.  This is because the host side ice
>>> driver clears the VF from filters when it is reset.  iavf then correctly
>>> calls iavf_configure, but since the current_netdev_promisc_flags already
>>> match the netdev promisc settings, nothing is done, so the promisc and
>>> multicast settings are not sent to the ice host driver after the reset.
>>> As a result the iavf side shows promisc enabled but it isn't working.
>>> Disabling and re-enabling promisc on the iavf side fixes it of course.
>>> Simple test case to show this is to enable promisc, check that packets
>>> are being seen, then change the mtu size (which does a reset) and check
>>> packets received again, and promisc is no longer active.  Disabling
>>> promisc and enabling it again restores receiving the packets.
>>>
>>> The following seems to work for me, but I am not sure it is the correct
>>> place to clear the saved flags.
>>>
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> index 6d7ba4d67a19..4018a08d63c1 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> @@ -3233,6 +3233,14 @@ static void iavf_reset_task(struct work_struct *work)
>>>  		iavf_shutdown_adminq(hw);
>>>  		iavf_init_adminq(hw);
>>>  		iavf_request_reset(adapter);
>>> +
>>> +		/* Clear remembered promisc and multicast flags since
>>> +		 * reset clears them on the host so they will get force
>>> +		 * applied again through iavf_configure() down below.
>>> +		 */
>>> +		spin_lock_bh(&adapter->current_netdev_promisc_flags_lock);
>>> +		adapter->current_netdev_promisc_flags &= ~(IFF_PROMISC | IFF_ALLMULTI);
>>> +		spin_unlock_bh(&adapter->current_netdev_promisc_flags_lock);
>>>  	}
>>>  	adapter->flags |= IAVF_FLAG_RESET_PENDING;
>>>  
>>>
>>
>> We probably need to do something similar in the flow where we get an
>> unexpected reset (such as if PF resets us by changing trusted flag or
>> other state).
>>
>> I don't think there's a better solution. Arguably the PF shouldn't be
>> losing data, but I think its a bit late to go that route at this point..
>> Its pretty baked into the virtchnl API :(
> 
> Yeah I can see arguments that calling reset should put everything in a
> known state so the VF driver can configure things as it wants, but since
> reset is also used when tx hang happens or mtu changes and various other
> things, it is definitely inconvinient.  Changing behaviour with an API
> version change seems ugly too and you would still have to support the
> old API anyhow.  I suppose having a reset fully to defaults and a soft
> reset to change settings but keep other values could have been nice,
> but a bit late now.  Some VF drivers may even be depending on the reset
> putting everything to defaults.
> 

Yes. That's the trouble with the current approach. The VF interface has
to work well when the VF driver is running different operating systems
or versions, and if we change the behavior with a new opcode or similar
that would be difficult.

The reset logic is likely a haphazard mess of different "solutions" to
various issues we've had. It grew more or less organically out of i40evf
code from years ago.

> If someone that knows the driver better can make a complete fix that
> would be great.  So far this small change appears to be working but I
> could certainly have missed some cases.  It took quite a bit of debugging
> to discover why promiscous mode on the VF side was so unreliable and
> unpredicable.  Due to the somewhat asynchrounous message handling,
> sometimes the reset would not happen until after the promisc setting
> had been applied, and then it was silently lost, while other times it
> would do the reset quicker and then the promisc setting would work.
> Very confusing to debug.
> 

Agreed. Obviously, our own testing never caught this. :(

We might be able to get away with improving the PF to stop losing as
much data, but I worry that could lead to a similar sort of race
condition as this but in reverse, where VF thinks that it was cleared. I
guess the VF would send a new config and that would either be a no-op or
just restore config.

That makes me think this fix to the VF is required regardless of what or
how we modify the PF.

Re: Fix promiscous and multicast mode on iavf after reset

[Lennart Sorensen]

On Tue, Apr 29, 2025 at 11:44:55AM -0700, Jacob Keller wrote:
> Yes. That's the trouble with the current approach. The VF interface has
> to work well when the VF driver is running different operating systems
> or versions, and if we change the behavior with a new opcode or similar
> that would be difficult.
> 
> The reset logic is likely a haphazard mess of different "solutions" to
> various issues we've had. It grew more or less organically out of i40evf
> code from years ago.
> 
> Agreed. Obviously, our own testing never caught this. :(

Yes you need to actually run with promisc on, not just using tcpdump
once in a while.  So someone using the interface connected to a virtual
bridge that would want promisc to allow all traffic to be received that
then hits a tx hang would see it, but probably that is about the only
time you would have hit it.  tx hangs don't seem to be nearly as common
as they were back in the igbe and ixgbe days fortunately.

In my particular case it was enabling promisc mode, then changing the
mtu that resulted in very often loosing promisc mode.

> We might be able to get away with improving the PF to stop losing as
> much data, but I worry that could lead to a similar sort of race
> condition as this but in reverse, where VF thinks that it was cleared. I
> guess the VF would send a new config and that would either be a no-op or
> just restore config.
> 
> That makes me think this fix to the VF is required regardless of what or
> how we modify the PF.

It seems better to make the VF driver handle it since you don't know
what kernel version the host is running and hence what it is going to
do when you do reset (unless you up the API version of course, which
seems excessive just for this, and you would still have to handle the
case when the host is older).

Of course it seems that if the driver wasn't caching the current settings
for promisc and multicast and simply sent the config everytime any config
changed, it would be working, but it would also be wasteful.  I don't
remember when the cache was introduced, but I think it was done as part
of not sending a message for promisc and a separate one for multicast
since it sometimes resulted in the wrong setting in the end.  But the
caching thing has not been around for the entire life of the iavf/i40evf
driver so it may in fact have worked in the past and was accidentally
broken as part of fixing the other issue.

-- 
Len Sorensen