netdev - Re: [RFC PATCH v2 net-next 0/4] 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: <20211210195441.6drqtckl2m6rbmk6@skbuf>
Date:   Fri, 10 Dec 2021 19:54:42 +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 v2 net-next 0/4] DSA master state tracking

On Fri, Dec 10, 2021 at 08:45:43PM +0100, Ansuel Smith wrote:
> > Anyway the reason why I didn't say anything about this is because I
> > don't yet understand how it is supposed to work. Specifically:
> > 
> > rtnl_lock
> > 
> > dev_open()
> > -> __dev_open()
> >    -> dev->flags |= IFF_UP;
> >    -> dev_activate()
> >       -> transition_one_qdisc()
> > -> call_netdevice_notifiers(NETDEV_UP, dev);
> > 
> > rtnl_unlock
> > 
> > so the qdisc should have already transitioned by the time NETDEV_UP is
> > emitted.
> > 
> > and since we already require a NETDEV_UP to have occurred, or dev->flags
> > to contain IFF_UP, I simply don't understand the following
> > (a) why would the qdisc be noop when we catch NETDEV_UP
> > (b) who calls netdev_state_change() (or __dev_notify_flags ?!) after the
> >     qdisc changes on a TX queue? If no one, then I'm not sure how we can
> >     reliably check for the state of the qdisc if we aren't notified
> >     about changes to it.
> 
> The ipv6 check is just a hint. The real clue was the second
> NETDEV_CHANGE called by linkwatch_do_dev in link_watch.c
> That is the one that calls the CHANGE event before the ready stuff.
> 
> I had problem tracking this as the change logic is "emit CHANGE when flags
> change" but netdev_state_change is also called for other reason and one
> example is dev_activate/dev_deactivate from linkwatch_do_dev.
> It seems a bit confusing that a generic state change is called even when
> flags are not changed and because of this is a bit problematic track why
> the CHANGE event was called.
> 
> Wonder if linkwatch_do_dev should be changed and introduce a flag? But
> that seems problematic if for whatever reason a driver use the CHANGE
> event to track exactly dev_activate/deactivate.

Yes, I had my own "aha" moment just minutes before you sent this email
about linkwatch_do_dev. So indeed that's the source of both the
dev_activate(), as well as the netdev_state_change() notifier.

As to my previous question (why would the qdisc be noop when we catch
NETDEV_UP): the answer is of course in the code as well:

dev_activate() has:
	if (!netif_carrier_ok(dev))
		/* Delay activation until next carrier-on event */
		return;

which is then actually picked up from linkwatch_do_dev().

Let's not change linkwatch_do_dev(), I just wanted to understand why it
works. Please confirm that it also works for you to make master_admin_up
depend on qdisc_tx_is_noop() instead of the current ingress_queue check,
then add a comment stating the mechanism through which we are tracking
the dev_activate() calls, and then this should be good to go.
I'd like you to pick up the patches and post them together with your
driver changes. I can't post the patches on my own since I don't have
any use for them. I'll leave a few more "review" comments on them in a
minute.