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]
Date:   Fri, 10 Dec 2021 20:45:43 +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 v2 net-next 0/4] DSA master state tracking

On Fri, Dec 10, 2021 at 07:27:24PM +0000, Vladimir Oltean wrote:
> On Fri, Dec 10, 2021 at 08:10:21PM +0100, Ansuel Smith wrote:
> > > Ok I added more tracing and packet are received to the tagger right
> > > after the log from ipv6 "link becomes ready". That log just check if the
> > > interface is up and if it does have a valid sched.
> > > I notice after link becomes ready we have a CHANGE event for eth0. That
> > > should be the correct way to understand when the cpu port is actually
> > > usable.
> > > (just to make it clear before the link becomes ready no packet is
> > > received to the tagger and the completion timeouts)
> > > 
> > > -- 
> > > 	Ansuel
> > 
> > Sorry for the triple message spam... I have a solution. It seems packet
> > are processed as soon as dev_activate is called (so a qdisk is assigned)
> > By adding another bool like master_oper_ready and
> > 
> > void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
> >                                       struct net_device *master,
> >                                       bool up);
> > 
> > static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
> >                                         struct net_device *master)
> > {
> >        struct dsa_notifier_master_state_info info;
> >        struct dsa_port *cpu_dp = master->dsa_ptr;
> > 
> >        info.master = master;
> >        info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up && cpu_dp->master_oper_ready;
> > 
> >        dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
> > }
> > 
> > void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
> >                                       struct net_device *master,
> >                                       bool up)
> > {
> >        struct dsa_port *cpu_dp = master->dsa_ptr;
> >        bool notify = false;
> > 
> >        if ((cpu_dp->master_oper_ready && cpu_dp->master_oper_ready) !=
> >            (cpu_dp->master_oper_ready && up))
> >                notify = true;
> > 
> >        cpu_dp->master_oper_ready = up;
> > 
> >        if (notify)
> >                dsa_tree_master_state_change(dst, master);
> > }
> > 
> > In slave.c at the NETDEV_CHANGE event the additional
> > dsa_tree_master_oper_state_ready(dst, dev, dev_ingress_queue(dev));
> > we have no timeout function. I just tested this and it works right away.
> > 
> > Think we need this additional check to make sure the tagger can finally
> > accept packet from the switch.
> > 
> > With this added I think this is ready.
> 
> Why ingress_queue?
> I was looking at dev_activate() too, especially since net/ipv6/addrconf.c uses:
> 
> /* Check if link is ready: is it up and is a valid qdisc available */
> static inline bool addrconf_link_ready(const struct net_device *dev)
> {
> 	return netif_oper_up(dev) && !qdisc_tx_is_noop(dev);
> }
> 
> and you can see that qdisc_tx_is_noop() checks for the qdisc on TX
> queues, not ingress qdisc (which makes more sense anyway).
> 
> 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.

-- 
	Ansuel