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: <20211209175617.652rdiidc6pfgdwz@skbuf>
Date:   Thu, 9 Dec 2021 17:56:17 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Ansuel Smith <ansuelsmth@...il.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 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.