[PATCH net 0/2] net: ethtool: netlink: Fix notifications for

[PATCH net 0/2] net: ethtool: netlink: Fix notifications for

[Maxime Chevallier]

It has been found (thanks to Parthiban) that the PLCA ethtool commands
were failing since 6.12, due to the phy_link_topology work. This was
traced back to the ethnl notifications mechanism, in which calls to
ethnl_req_get_phydev() crashed in the notification path following a
->set request.

The typical callsite for ethnl_req_get_phydev() looks like :

    phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_XXX_HEADER],
				  info->extack);

as 'tb' is NULL in the notification path for the ->prepare_data ethnl
ops, this causes crashes. The solution for that is to change the
prototype of ethnl_req_get_phydev() to perform checks inside the helper
(patch 1).

While investigating that, I realised that the notification path for PHYs
is not correct anyways. As we don't have a netlink request to parse, we
can't know for sure which PHY the notification event targets in the case
of a notification following a ->set request.

Patch 2 introduces a context structure that is used between ->set
requests and the followup notification, to keep track of the PHY that
the original request targeted for the notification.

Thanks Parthiban for the report (not on netdev@ though).

Maxime

Maxime Chevallier (2):
  net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device
  net: ethtool: netlink: Pass a context for default ethnl notifications

 net/ethtool/cabletest.c |  8 ++++----
 net/ethtool/linkstate.c |  2 +-
 net/ethtool/netlink.c   | 21 +++++++++++++++++----
 net/ethtool/netlink.h   |  5 +++--
 net/ethtool/phy.c       |  2 +-
 net/ethtool/plca.c      |  6 +++---
 net/ethtool/pse-pd.c    |  4 ++--
 net/ethtool/stats.c     |  2 +-
 net/ethtool/strset.c    |  2 +-
 9 files changed, 33 insertions(+), 19 deletions(-)

-- 
2.48.1

[PATCH net 1/2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device

[Maxime Chevallier]

ethnl_req_get_phydev() is used to lookup a phy_device, in the case an
ethtool netlink command targets a specific phydev within a netdev's
topology.

It takes as a parameter a const struct nlattr *header that's used for
error handling :

       if (!phydev) {
               NL_SET_ERR_MSG_ATTR(extack, header,
                                   "no phy matching phyindex");
               return ERR_PTR(-ENODEV);
       }

In the notify path after a ->set operation however, there's no request
attributes available.

The typical callsite for the above function looks like:

	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_XXX_HEADER],
				      info->extack);

So, when tb is NULL (such as in the ethnl notify path), we have a nice
crash.

It turns out that there's only the PLCA command that is in that case, as
the other phydev-specific commands don't have a notification.

This commit fixes the crash by passing the cmd index and the nlattr
array separately, allowing NULL-checking it directly inside the helper.

Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some commands")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---

I didn't include a Reported-by tag as the report was done outside of the
mailing list, and checkpatch wants a "Closes:" if I add Reported-by...

 net/ethtool/cabletest.c | 8 ++++----
 net/ethtool/linkstate.c | 2 +-
 net/ethtool/netlink.c   | 6 +++---
 net/ethtool/netlink.h   | 5 +++--
 net/ethtool/phy.c       | 2 +-
 net/ethtool/plca.c      | 6 +++---
 net/ethtool/pse-pd.c    | 4 ++--
 net/ethtool/stats.c     | 2 +-
 net/ethtool/strset.c    | 2 +-
 9 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index f22051f33868..84096f6b0236 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -72,8 +72,8 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	dev = req_info.dev;
 
 	rtnl_lock();
-	phydev = ethnl_req_get_phydev(&req_info,
-				      tb[ETHTOOL_A_CABLE_TEST_HEADER],
+	phydev = ethnl_req_get_phydev(&req_info, tb,
+				      ETHTOOL_A_CABLE_TEST_HEADER,
 				      info->extack);
 	if (IS_ERR_OR_NULL(phydev)) {
 		ret = -EOPNOTSUPP;
@@ -339,8 +339,8 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		goto out_dev_put;
 
 	rtnl_lock();
-	phydev = ethnl_req_get_phydev(&req_info,
-				      tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
+	phydev = ethnl_req_get_phydev(&req_info, tb,
+				      ETHTOOL_A_CABLE_TEST_TDR_HEADER,
 				      info->extack);
 	if (IS_ERR_OR_NULL(phydev)) {
 		ret = -EOPNOTSUPP;
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index af19e1bed303..05a5f72c99fa 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -103,7 +103,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_LINKSTATE_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_LINKSTATE_HEADER,
 				      info->extack);
 	if (IS_ERR(phydev)) {
 		ret = PTR_ERR(phydev);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index b4c45207fa32..734849a57369 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -211,7 +211,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 }
 
 struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
-					const struct nlattr *header,
+					struct nlattr **tb, unsigned int header,
 					struct netlink_ext_ack *extack)
 {
 	struct phy_device *phydev;
@@ -225,8 +225,8 @@ struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
 		return req_info->dev->phydev;
 
 	phydev = phy_link_topo_get_phy(req_info->dev, req_info->phy_index);
-	if (!phydev) {
-		NL_SET_ERR_MSG_ATTR(extack, header,
+	if (!phydev && tb) {
+		NL_SET_ERR_MSG_ATTR(extack, tb[header],
 				    "no phy matching phyindex");
 		return ERR_PTR(-ENODEV);
 	}
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index ff69ca0715de..ec6ab5443a6f 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -275,7 +275,8 @@ static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
  * ethnl_req_get_phydev() - Gets the phy_device targeted by this request,
  *			    if any. Must be called under rntl_lock().
  * @req_info:	The ethnl request to get the phy from.
- * @header:	The netlink header, used for error reporting.
+ * @tb:		The netlink attributes array, for error reporting.
+ * @header:	The netlink header index, used for error reporting.
  * @extack:	The netlink extended ACK, for error reporting.
  *
  * The caller must hold RTNL, until it's done interacting with the returned
@@ -289,7 +290,7 @@ static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
  *	   is returned.
  */
 struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
-					const struct nlattr *header,
+					struct nlattr **tb, unsigned int header,
 					struct netlink_ext_ack *extack);
 
 /**
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index ed8f690f6bac..e067cc234419 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -125,7 +125,7 @@ static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
 	struct phy_req_info *req_info = PHY_REQINFO(req_base);
 	struct phy_device *phydev;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PHY_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PHY_HEADER,
 				      extack);
 	if (!phydev)
 		return 0;
diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
index d95d92f173a6..e1f7820a6158 100644
--- a/net/ethtool/plca.c
+++ b/net/ethtool/plca.c
@@ -62,7 +62,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PLCA_HEADER,
 				      info->extack);
 	// check that the PHY device is available and connected
 	if (IS_ERR_OR_NULL(phydev)) {
@@ -152,7 +152,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 	bool mod = false;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PLCA_HEADER],
+	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PLCA_HEADER,
 				      info->extack);
 	// check that the PHY device is available and connected
 	if (IS_ERR_OR_NULL(phydev))
@@ -211,7 +211,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PLCA_HEADER,
 				      info->extack);
 	// check that the PHY device is available and connected
 	if (IS_ERR_OR_NULL(phydev)) {
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 2819e2ba6be2..4f6b99eab2a6 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -64,7 +64,7 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PSE_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
 	if (IS_ERR(phydev))
 		return -ENODEV;
@@ -261,7 +261,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
+	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
 	ret = ethnl_set_pse_validate(phydev, info);
 	if (ret)
diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 038a2558f052..3ca8eb2a3b31 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -138,7 +138,7 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_STATS_HEADER],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_STATS_HEADER,
 				      info->extack);
 	if (IS_ERR(phydev))
 		return PTR_ERR(phydev);
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 6b76c05caba4..f6a67109beda 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -309,7 +309,7 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 		return 0;
 	}
 
-	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_HEADER_FLAGS],
+	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_HEADER_FLAGS,
 				      info->extack);
 
 	/* phydev can be NULL, check for errors only */
-- 
2.48.1

[PATCH net 2/2] net: ethtool: netlink: Pass a context for default ethnl notifications

[Maxime Chevallier]

In some situations, it's useful to get some context passed to ethnl
notifications, especially when we perform a ->set followed by
ethtool_notify().

One such case is when the ->set targets a specific PHY device. The
phy_index of the PHY may be coming from the request header, and we want
the followup notification to be specific to the phydev we just
accessed.

This commit leverages the const void *data pointer that's passed to
ethtool netlink notifications.

In our case, and only for default ethnl ops, lets use that void* pointer
to pass a context. The context is filled in the ->set request path, and
used in ethnl_default_notify() to populate the req_info with context
information.

For now, the only thing we pass in the context is the phy_index of the
->set request.

The only relevant user for now is PLCA, and it very likely that we never
ended-up in a situation where the follow-up notif wasn't targeting the
correct PHY as :

 - This was broken due to the tb[] array being NULL for notifs
 - There's no upstream-supported scenario (as of today) where we have 2
   PHYs that can do PLCA (a BaseT1 feature) on the same netdev.

Fixes: c15e065b46dc ("net: ethtool: Allow passing a phy index for some commands")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/netlink.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 734849a57369..6691a8f73bfd 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -663,8 +663,17 @@ static int ethnl_default_done(struct netlink_callback *cb)
 	return 0;
 }
 
+/* Structure to store context information between a ->set request and the
+ * follow-up notification. Used only for the ethnl_default ops.
+ * @phy_index: If the original ->set request had a PHY index, store it in ctx.
+ */
+struct ethnl_default_notify_ctx {
+	u32 phy_index;
+};
+
 static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 {
+	struct ethnl_default_notify_ctx ctx = {0};
 	const struct ethnl_request_ops *ops;
 	struct ethnl_req_info req_info = {};
 	const u8 cmd = info->genlhdr->cmd;
@@ -691,6 +700,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	dev = req_info.dev;
+	ctx.phy_index = req_info.phy_index;
 
 	rtnl_lock();
 	dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
@@ -711,7 +721,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	swap(dev->cfg, dev->cfg_pending);
 	if (!ret)
 		goto out_ops;
-	ethtool_notify(dev, ops->set_ntf_cmd, NULL);
+	ethtool_notify(dev, ops->set_ntf_cmd, &ctx);
 
 	ret = 0;
 out_ops:
@@ -749,6 +759,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 				 const void *data)
 {
+	const struct ethnl_default_notify_ctx *ctx = data;
 	struct ethnl_reply_data *reply_data;
 	const struct ethnl_request_ops *ops;
 	struct ethnl_req_info *req_info;
@@ -776,6 +787,8 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 
 	req_info->dev = dev;
 	req_info->flags |= ETHTOOL_FLAG_COMPACT_BITSETS;
+	if (ctx)
+		req_info->phy_index = ctx->phy_index;
 
 	ethnl_init_reply_data(reply_data, ops, dev);
 	ret = ops->prepare_data(req_info, reply_data, &info);
-- 
2.48.1

Re: [PATCH net 2/2] net: ethtool: netlink: Pass a context for default ethnl notifications

[Jakub Kicinski]

On Thu, 27 Feb 2025 19:24:52 +0100 Maxime Chevallier wrote:
> The only relevant user for now is PLCA, and it very likely that we never
> ended-up in a situation where the follow-up notif wasn't targeting the
> correct PHY as :

PLCA uses the ethnl_default_* handlers but it seems to operate on PHYs
now. How does the dump work? Shoehorning the PHY support into
the ethnl_default_* handlers is starting to look pretty messy.

Re: [PATCH net 2/2] net: ethtool: netlink: Pass a context for default ethnl notifications

[Maxime Chevallier]

On Fri, 28 Feb 2025 18:24:40 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 27 Feb 2025 19:24:52 +0100 Maxime Chevallier wrote:
> > The only relevant user for now is PLCA, and it very likely that we never
> > ended-up in a situation where the follow-up notif wasn't targeting the
> > correct PHY as :  
> 
> PLCA uses the ethnl_default_* handlers but it seems to operate on PHYs
> now. How does the dump work? Shoehorning the PHY support into
> the ethnl_default_* handlers is starting to look pretty messy.

I agree, that's the less ugly quick solution I could think of :(

So maybe we need some generic PHY dump support for all PHY commands ?

I should probably re-send patch 1 only to fix the crash, and rework the
dump/notify for PHY commands separately if this is OK for you ?

Maxime

Re: [PATCH net 2/2] net: ethtool: netlink: Pass a context for default ethnl notifications

[Maxime Chevallier]

On Fri, 28 Feb 2025 18:24:40 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 27 Feb 2025 19:24:52 +0100 Maxime Chevallier wrote:
> > The only relevant user for now is PLCA, and it very likely that we never
> > ended-up in a situation where the follow-up notif wasn't targeting the
> > correct PHY as :  
> 
> PLCA uses the ethnl_default_* handlers but it seems to operate on PHYs
> now. How does the dump work? Shoehorning the PHY support into
> the ethnl_default_* handlers is starting to look pretty messy.

Thinking more about this, this patch shouldn't have targetted -net in
the first place, as no usecase regressed yet (we don't have any
multi-PHY PLCA setup that exists). The DUMP keeps working as it did
before for that command, in that we dump the PLCA info for
netdev->phydev.

That DUMP / notify situation needs to be addressed as tome point thouh,
before the multi-PHY support through muxes lands (haven't sent it yet,
I'm still on the preliminary series for phy_ports).

So I think that indeed, I'll resend patch 1, and find a more graceful
solution for the phy-targetting commands in a separate series.

Maxime

Re: [PATCH net 0/2] net: ethtool: netlink: Fix notifications for

[Maxime Chevallier]

On Thu, 27 Feb 2025 19:24:50 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

Oops subject got truncated, should be :

net: ethtool: netlink: Fix notifications for PHY commands 

Maxime :(

Re: [PATCH net 0/2] net: ethtool: netlink: Fix notifications for

[Maxime Chevallier]

On Fri, 28 Feb 2025 06:19:18 +0000
<Parthiban.Veerasooran@microchip.com> wrote:

> Hi Maxime,
> 
> I did a quick test with your patches and it seems working fine without 
> kernel crash.
> 
> Best regards,
> Parthiban V

Thanks for testing, good to hear that this solves the issue :)

Maxime