netdev - Re: [RFC PATCH net-next 0/7] DSA master state tracking

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <61b24816.1c69fb81.c3a53.17cc@mx.google.com>
Date:   Thu, 9 Dec 2021 19:16:46 +0100
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [RFC PATCH net-next 0/7] DSA master state tracking

On Thu, Dec 09, 2021 at 05:56:17PM +0000, Vladimir Oltean wrote:
> On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
> > > I think the problem is that we also need to track the operstate of the
> > > master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
> > > You can see that this is exactly the line after which the timeouts disappear:
> > > 
> > > [    7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> > > 
> > > I didn't really want to go there, because now I'm not sure how to
> > > synthesize the information for the switch drivers to consume it.
> > > Anyway I've prepared a v2 patchset and I'll send it out very soon.
> > 
> > Wonder if we should leave the driver decide when it's ready by parsing
> > the different state? (And change
> > the up ops to something like a generic change?)
> 
> There isn't just one state to track, which is precisely the problem that
> I had to deal with for v2. The master is operational during the time
> frame between NETDEV_UP and NETDEV_GOING_DOWN, intersected with the
> interval during which netif_oper_up(master) is true. So in the simple
> state propagation approach, DSA would need to provide at least two ops
> to switches, one for admin state and the other for oper state. And the
> switch driver would need to AND the two and keep state by itself.
> Letting the driver make the decision would have been acceptable to me if
> we could have 3 ops and a common implementation, something like this:
> 
> static void qca8k_master_state_change(struct dsa_switch *ds,
> 				      const struct dsa_master *master)
> {
> 	bool operational = (master->flags & IFF_UP) && netif_oper_up(master);
> }
> 
> 	.master_admin_state_change	= qca8k_master_state_change,
> 	.master_oper_state_change	= qca8k_master_state_change,
> 
> but the problem is that during NETDEV_GOING_DOWN, master->flags & IFF_UP
> is still true, so this wouldn't work. And replacing the NETDEV_GOING_DOWN
> notifier with the NETDEV_DOWN one would solve that problem, but it would
> no longer guarantee that the switch can disable this feature without
> timeouts before the master is down - because now it _is_ down.

Ok will have to test v2 and check if this is also fixed.

-- 
	Ansuel