linux-kernel - Re: Fix promiscous and multicast mode on iavf after reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aAu2zoNIuRk-nwWt@csclub.uwaterloo.ca>
Date: Fri, 25 Apr 2025 12:22:38 -0400
From: Lennart Sorensen <lsorense@...lub.uwaterloo.ca>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: Fix promiscous and multicast mode on iavf after reset

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