Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 23 Sep 2020 17:21:25 -0700 (PDT)
Message-Id: <20200923.172125.1341776337290371000.davem@davemloft.net>
To:     saeed@kernel.org
Cc:     hkallweit1@gmail.com, geert+renesas@glider.be,
        f.fainelli@gmail.com, andrew@lunn.ch, kuba@kernel.org,
        gaku.inami.xh@renesas.com, yoshihiro.shimoda.uh@renesas.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Revert "net: linkwatch: add check for netdevice being
 present to linkwatch_do_dev"
From:   David Miller <davem@davemloft.net>
In-Reply-To: <a7ff1afd2e1fc2232103ceb9aa763064daf90212.camel@kernel.org>
References: <20200923.131529.637266321442993059.davem@davemloft.net>
        <e6f50a85-aa25-5fb7-7fd2-158668d55378@gmail.com>
        <a7ff1afd2e1fc2232103ceb9aa763064daf90212.camel@kernel.org>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Precedence: bulk

From: Saeed Mahameed <saeed@kernel.org>
Date: Wed, 23 Sep 2020 15:42:17 -0700

> Maybe we need to clear IFF_UP before calling ops->ndo_stop(dev),
> instead of after on __dev_close_many(). Assuming no driver is checking
> IFF_UP state on its own ndo_stop(), other than this, the order
> shouldn't really matter, since clearing the flag and calling ndo_stop()
> should be considered as one atomic operation.

This is my biggest concern, that some ndo_stop, or some helper called
by ndo_stop, checks IFF_UP or similar.

There is also something else.  We have both synchronous and async code
that checks state like IFF_UP and 'present' and makes a decision based
upon that.

If an async code path tests 'present', gets true, and then the RTNL
holding synchronous code path puts the device into D3hot immediately
afterwards, the async code path will still continue and access the
chips registers and fault.

I'm saying all of this because the only way this bug makes sense
is if the ->ndo_stop() sequence that marks the device !present and
then clears IFF_UP runs with the RTNL mutex held, and the code path
that tests this state in the linkwatch bits in question do not.