Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Sean Anderson <sean.anderson@seco.com>
To:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org
Cc:     Paolo Abeni <pabeni@redhat.com>, Jakub Kicinski <kuba@kernel.org>,
        "David S . Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        Sean Anderson <sean.anderson@seco.com>
Subject: [PATCH net] net: phy: Warn if phy is attached when removing
Date:   Tue, 16 Aug 2022 12:37:01 -0400
Message-Id: <20220816163701.1578850-1-sean.anderson@seco.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?FymRWuW5RoJ9jttvNcDnhuzU4JWxXJ13o+ochrgIe6CTjeGo5B1Tf6BIte5J?=
 =?us-ascii?Q?GNt1s37T+40LC+aAm6/jy1WCuwTWmowzgql0X7WsJAuqIe6kVBJvxeL1/lBv?=
 =?us-ascii?Q?/+usCcwjJ8PO2Vm17KmIQzyUoJ9B43GB3F1qRxWJZySTBBKidwaFe1U6J+iB?=
 =?us-ascii?Q?AM8wKrQsvkg5DGfqwJVeoAPULnOAAvAHk2fvSO4XBVYe69AJVvK5EOEWvFMC?=
 =?us-ascii?Q?bjwKlGlK8wrZel+8Lbyqbr6iII6NQKClXu4gm7pM/DbEBfsuNxXLZbiGPb6S?=
 =?us-ascii?Q?Kmqf4GFGuKz2WOZiYELXXXyA+rJcLyFtS1hVY7g17o5EwhHR9sAECwpLSMBL?=
 =?us-ascii?Q?IglN0bW/OYGrEu2PxQcTxzpuTn+PcyHYM1NFZ2/7IV2q2aOCb3+gC8CXfyoL?=
 =?us-ascii?Q?pstJNcrf58gbx6axjXFKrs+FKtesQqOCL9k1/M+HnXS3TNc72iDa22apVks0?=
 =?us-ascii?Q?xQjWIA+cyK9OnQK9M14YTZHu7AieA/sqM0mez6RB3BD1+37LIuB566YDcTnj?=
 =?us-ascii?Q?hXVd27ugj6vQhyuPTzRYFKTkXj4AjVWbBf3vfXxV8N6OIhX4NGdf4MJ98dsv?=
 =?us-ascii?Q?xHFbKq0iAJZPHU+CkKMQsFaxq2Th8tSC58N1OjpCg8yf2N8RHAqPyHNFdBe4?=
 =?us-ascii?Q?gWKcGPnUKXtTH7r47nBx24TrMh3JB5Gq8WzkuRI9REso077W4EtxmTspJntP?=
 =?us-ascii?Q?Rlicno16hIQjP2x3+aXur01Whhd+ZjlognC78WXreX0rJEduB+ksef3zDaoU?=
 =?us-ascii?Q?becULEQyeu2LMsO2Y8h4py8FhABd40/C8FT4ZH8gwTxqXyyVgWu0Rc17VQun?=
 =?us-ascii?Q?GayDs7QFdyquCFiXhwwT0VTvX3gPReMXPC7UrOxCjSCH4e9Q/HJ2rZz14sYZ?=
 =?us-ascii?Q?LNKdOofOmtnc5k1fUzfygf3+D5mBcx1rhYY1+NB5gLm+z2nevB/77RyuSj8L?=
 =?us-ascii?Q?NoJ/T2zhPBpPVZXbhd+++B4+BHK0puRkUlk6tly/eK70Z4rsMb5ak9//a8pV?=
 =?us-ascii?Q?s1q0IrCNqu07DvdSOG0QMPVWjPbhTSt0x2FRD4xzsmT4CpBBQhWo9JUdpIhj?=
 =?us-ascii?Q?F1uZxTGgaiUHmbo01n/wTWd8UrM4Bm0x5/KpQwmHIM13E0yB08vtmynkeXDn?=
 =?us-ascii?Q?atF5L7hw4scb85+/6TnO8we52g1Xq+45FTrI2rtnWi3NYJCuXvtVFAy1n0Q8?=
 =?us-ascii?Q?xWFEd2qzG7Tfknxz/C8+808AfBafqaJOU2Wmmykc4gH6C3eE+MIG4q41mk1R?=
 =?us-ascii?Q?n8PNdLgnUvF1UukRHxRNGaCe2G4p83FMuMSoIGIZMklBNNG0y+pXINIjlIU8?=
 =?us-ascii?Q?U2KadPiUghiuTVrTug3e9YOZd7fDvlsb/g3u2E22kv0t1ZAJO98z1wDXMJag?=
 =?us-ascii?Q?aPPuqkC2hL9vPOiGPKNyML8OeeaaRVzfHqVla55v+IxhSgkcJV0r8knKEDRB?=
 =?us-ascii?Q?VuoVaHR9hCDboJ1Fx8PZkQ7bzzKIcK992ca0V4kMUUtlgLTfFFIcHi8rlRyo?=
 =?us-ascii?Q?gIxhV55aTRypU74bKnTgvv3sBRYgzQAxFTixyCxESc0CPIZVEhQt6wF88rfY?=
 =?us-ascii?Q?g7yztm14ciJVcqatb83lLXJG7siU3EcgMaFYX7UCXGus0IBcc/vpDKHZSIWO?=
 =?us-ascii?Q?/g=3D=3D?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e725b9ae-452a-4202-a585-08da7fa59327
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4972.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Aug 2022 16:37:14.6839
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rbbpYox9+uu8/VjkAAKlAf9uJoMERocNFvexG35boa8WETwF8sywG4gvLcGlNatwIrmxMXnDTiMxwq7j/pKX1w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR0302MB3288
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

netdevs using phylib can be oopsed from userspace in the following
manner:

$ ip link set $iface up
$ echo $(basename $(readlink /sys/class/net/$iface/phydev)) > \
      /sys/class/net/$iface/phydev/driver/unbind
$ ip link set $iface down

However, the traceback provided is a bit too late, since it does not
capture the root of the problem (unbinding the driver). It's also
possible that the memory has been reallocated if sufficient time passes
between when the phy is detached and when the netdev touches the phy
(which could result in silent memory corruption). Add a warning at the
source of the problem. A future patch could make this more robust by
calling dev_close.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This is a result of discussion around my attempt to make PCS devices
proper devices [1]. Russell pointed out [2] that someone could unbind
the PCS at any time. However, the same issue applies to ethernet phys,
as well as serdes phys. Both of these can be unbound at any time, yet
no precautions are taken to avoid dereferencing a stale pointer.

As I discussed [3], we have (in general) four ways to approach this

- Just warn and accept that we are going to oops later on
- Create devlinks between the consumer/supplier
- Create a composite device composed of the consumer and its suppliers
- Add a callback (such as dev_close) and call it when the consumer goes
  away

It is (of course) also possible to rewrite phylib so that devices are
not used (preventing the phy from being unbound). However, I suspect
that this is quite undesirable.

This patch implements the first option, which, while fixing nothing, at
least provides some better debug information.

[1] https://lore.kernel.org/netdev/20220711160519.741990-1-sean.anderson@seco.com/
[2] https://lore.kernel.org/netdev/YsyPGMOiIGktUlqD@shell.armlinux.org.uk/
[3] https://lore.kernel.org/netdev/9747f8ef-66b3-0870-cbc0-c1783896b30d@seco.com/

 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0c6efd792690..d75ca97f74d4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3119,6 +3119,8 @@ static int phy_remove(struct device *dev)
 
 	cancel_delayed_work_sync(&phydev->state_queue);
 
+	WARN_ON(phydev->attached_dev);
+
 	mutex_lock(&phydev->lock);
 	phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
-- 
2.35.1.1320.gc452695387.dirty