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: <20211210192723.noa3hb2vso6t7zju@skbuf>
Date:   Fri, 10 Dec 2021 19:27:24 +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: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.