| Message ID | 20250301141114.97204-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 637399bf7e77797811adf340090b561a8f9d1213 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device | expand |
On Sat, 1 Mar 2025 15:11:13 +0100 Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > 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> Reviewed-by: Kory Maincent <kory.maincent@bootlin.com> Thank you!
On Sat, 1 Mar 2025 15:11:13 +0100 Maxime Chevallier wrote: > 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> Well, this alone doesn't look too bad.. :) Hopefully we can address adding more suitable handlers for phy ops in net-next. Didn't someone report this, tho? I vaguely remember seeing an email, unless they said they don't want to be credited maybe we should add a Reported-by tag? You can post it in reply, no need to repost the patch.
On Mon, 3 Mar 2025 16:10:24 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Sat, 1 Mar 2025 15:11:13 +0100 Maxime Chevallier wrote: > > 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> > > Well, this alone doesn't look too bad.. :) Hopefully we can address > adding more suitable handlers for phy ops in net-next. Yeah I'm cooking something to improve on that, and I have also dusted off a netdevsim patch I had written back when working on the phy_link_topology that adds very very basic PHY support so that we can start covering all these commands with proper tests. I'll hopefully send something in the coming week or so. > Didn't someone report this, tho? I vaguely remember seeing an email, > unless they said they don't want to be credited maybe we should add > a Reported-by tag? You can post it in reply, no need to repost > the patch. Parthiban reported this without CC: netdev, but I think it's fair to add : Reported-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com> I didn't include it in the first place because checkpatch complained about a reported-by tag without a "Closes:", which we don't have because of the private reporting :) Thanks Jakub, Maxime
Hi All, On 04/03/25 3:22 pm, Maxime Chevallier wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, 3 Mar 2025 16:10:24 -0800 > Jakub Kicinski <kuba@kernel.org> wrote: > >> On Sat, 1 Mar 2025 15:11:13 +0100 Maxime Chevallier wrote: >>> 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> >> >> Well, this alone doesn't look too bad.. :) Hopefully we can address >> adding more suitable handlers for phy ops in net-next. > > Yeah I'm cooking something to improve on that, and I have also dusted > off a netdevsim patch I had written back when working on the > phy_link_topology that adds very very basic PHY support so that we can > start covering all these commands with proper tests. I'll hopefully send > something in the coming week or so. > >> Didn't someone report this, tho? I vaguely remember seeing an email, >> unless they said they don't want to be credited maybe we should add >> a Reported-by tag? You can post it in reply, no need to repost >> the patch. > > Parthiban reported this without CC: netdev, but I think it's fair to > add : > > Reported-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com> Thank you for fixing the issue. Best regards, Parthiban V > > I didn't include it in the first place because checkpatch complained > about a reported-by tag without a "Closes:", which we don't have > because of the private reporting :) > > Thanks Jakub, > > Maxime
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Sat, 1 Mar 2025 15:11:13 +0100 you wrote: > 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 : > > [...] Here is the summary with links: - [net,v2] net: ethtool: netlink: Allow NULL nlattrs when getting a phy_device https://git.kernel.org/netdev/net/c/637399bf7e77 You are awesome, thank you!
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 */
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> --- V2: Send this patch as a standalone fix instead of including it in a series [1] containing patches that don't belong to net. No modifications were made besides a rebase on -net. [1]: https://lore.kernel.org/netdev/20250227182454.1998236-1-maxime.chevallier@bootlin.com/ 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(-)