Message-ID: <20201005205544.7ddl4xzxhfas6nya@skbuf>
Date:   Mon, 5 Oct 2020 23:55:44 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Christian Eggers <ceggers@...i.de>
Cc:     Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] net: dsa: microchip: fix race condition

Hi Christian,

On Mon, Oct 05, 2020 at 06:08:29PM +0200, Christian Eggers wrote:
> Between queuing the delayed work and finishing the setup of the dsa
> ports, the process may sleep in request_module() and the queued work may
> be executed prior the initialization of the DSA ports is finished. In
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              "prior to the switch net devices being registered", maybe?
> ksz_mib_read_work(), a NULL dereference will happen within
> netof_carrier_ok(dp->slave).
> 
> Not queuing the delayed work in ksz_init_mib_timer() make things even
                                                       ~~~~
                                                       makes
> worse because the work will now be queued for immediate execution
> (instead of 2000 ms) in ksz_mac_link_down() via
> dsa_port_link_register_of().
> 
> Solution:
> 1. Do not queue (only initialize) delayed work in ksz_init_mib_timer().
> 2. Only queue delayed work in ksz_mac_link_down() if init is completed.
> 3. Queue work once in ksz_switch_register(), after dsa_register_switch()
> has completed.
> 
> Signed-off-by: Christian Eggers <ceggers@...i.de>
> Cc: stable@...r.kernel.org

For patches sent to the networking tree you should:
git format-patch --subject-prefix=
(a) "PATCH net-next" if it's a new feature (not applicable now)
(b) "PATCH net" if it's a bug fix (such is the case here)

Plus you should not Cc the stable mailing list, since David Miller deals
with sending patches to stable himself as long as you make sure to send
to his "net" tree as opposed to "net-next".

Read this for more details
https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html

> ---
> Call tree:

Please include the call path in the commit message, it is relevant that
request_module() is being called by phy_device_create(), and something
which you did not say in the verbal commit description.

FYI, you haven't even addressed the root cause of the problem, which is
ksz_mib_read_work sticking its nose where it's not supposed to:

		/* Only read MIB counters when the port is told to do.
		 * If not, read only dropped counters when link is not up.
		 */
		if (!p->read) {
			const struct dsa_port *dp = dsa_to_port(dev->ds, i);

			if (!netif_carrier_ok(dp->slave))
				mib->cnt_ptr = dev->reg_mib_cnt;
		}

This is simply Not Ok.
Not only the dp->slave is on purpose registered outside of the driver's
control (as you came to find out yourself), but not even all ports are
user ports. For example, the CPU port doesn't have a valid struct
net_device *slave pointer. You are just lucky that it's defined like
this:

struct dsa_port {
	/* A CPU port is physically connected to a master device.
	 * A user port exposed to userspace has a slave device.
	 */
	union {
		struct net_device *master;
		struct net_device *slave;
	};

so the code is in fact checking the status of the master interface's link.
But DSA doesn't assume that the *master and *slave pointers are under a
union. That can change any day, and when it changes, the KSZ driver will
break.

My personal feeling is that this driver hides a landmine beneath every
line of code, and it isn't getting better.
Sure, you should absolutely add the call stack to the commit message,
but how many people are going to git blame so they can see it. The code
needs to be obviously correct.

Things like needing to check dev->mib_read_interval as an indication
whether the race between ksz_mac_link_down and ksz_switch_register is
over are exactly the type of things that make it not fun to follow.

If reading MIB counters for ports that are down is such a "waste of time"
as per commit 7c6ff470aa867f53b8522a3a5c84c36ac7a20090, then how about
scheduling the delayed work from .phylink_mac_link_up, and canceling it
from .phylink_mac_link_down? Either that, or set a boolean variable to
struct ksz_port p->link_up, to true or false respectively from the
phylink callbacks, and using that as an indication whether to read the
MIB counters or not, instead of accessing the potentially invalid
dp->slave pointer? Would that work?

Sorry for rambling. I realize that there aren't probably a lot of things
you can do better to fix this problem for stable, but maybe you could
take some time and clean it up a little bit?

Thanks,
-Vladimir

Powered by blists - more mailing lists