[PATCH net 00/10] ethtool: more bug fixes

[PATCH net 00/10] ethtool: more bug fixes

[Jakub Kicinski]

Last week I sent two patch sets - one fixing bugs in RSS handling,
and one fixing CMIS / module handling. This set contains the remaining
fixes. There's a concentration of fixes around PHY and timestamp config
handling but not enough to break those out as separate sets.

Jakub Kicinski (10):
  ethtool: coalesce: cap profile updates at NET_DIM_PARAMS_NUM_PROFILES
  ethtool: tsconfig: fix reply error handling
  ethtool: linkstate: fix unbalanced ethnl_ops_complete() on PHY lookup
    error
  ethtool: pse-pd: fix missing ethnl_ops_complete()
  ethtool: tsconfig: fix missing ethnl_ops_complete()
  ethtool: tsinfo: fix uninitialized stats on the by-PHC path
  ethtool: tsinfo: don't pass ERR_PTR to genlmsg_cancel on prepare
    failure
  ethtool: strset: fix header attribute index in ethnl_req_get_phydev()
  ethtool: eeprom: add missing ethnl_ops_begin() / _complete() during
    fallback
  ethtool: eeprom: add more safeties to EEPROM Netlink fallback

 net/ethtool/coalesce.c  |  6 ++++++
 net/ethtool/eeprom.c    | 10 ++++++----
 net/ethtool/linkstate.c |  6 ++----
 net/ethtool/pse-pd.c    | 10 +++++-----
 net/ethtool/strset.c    |  2 +-
 net/ethtool/tsconfig.c  | 15 +++++++++++----
 net/ethtool/tsinfo.c    | 19 +++++++++++++++----
 7 files changed, 46 insertions(+), 22 deletions(-)

-- 
2.54.0

[PATCH net 01/10] ethtool: coalesce: cap profile updates at NET_DIM_PARAMS_NUM_PROFILES

[Jakub Kicinski]

ethnl_update_profile() walks the ETHTOOL_A_PROFILE_IRQ_MODERATION
nest list with an index 'i' and writes new_profile[i++] without
bounding i. The destination is kmemdup()'d at NET_DIM_PARAMS_NUM_PROFILES
entries (5), but the Netlink nest count is entirely user-controlled.
Netlink policies do not have support for constraining the number
of nested entries (or number of multi-attr entries).

Fixes: f750dfe825b9 ("ethtool: provide customized dim profile management")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
CC: haiyangz@microsoft.com
CC: hengqi@linux.alibaba.com
---
 net/ethtool/coalesce.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
index 1e2c5c7048a8..e73fc3e5a02b 100644
--- a/net/ethtool/coalesce.c
+++ b/net/ethtool/coalesce.c
@@ -472,6 +472,12 @@ static int ethnl_update_profile(struct net_device *dev,
 
 	nla_for_each_nested_type(nest, ETHTOOL_A_PROFILE_IRQ_MODERATION,
 				 nests, rem) {
+		if (i >= NET_DIM_PARAMS_NUM_PROFILES) {
+			NL_SET_BAD_ATTR(extack, nest);
+			ret = -E2BIG;
+			goto err_out;
+		}
+
 		ret = nla_parse_nested(tb, len_irq_moder - 1, nest,
 				       coalesce_irq_moderation_policy,
 				       extack);
-- 
2.54.0

Re: [PATCH net 01/10] ethtool: coalesce: cap profile updates at NET_DIM_PARAMS_NUM_PROFILES

[Maxime Chevallier]

Hi,

On 5/26/26 17:35, Jakub Kicinski wrote:
> ethnl_update_profile() walks the ETHTOOL_A_PROFILE_IRQ_MODERATION
> nest list with an index 'i' and writes new_profile[i++] without
> bounding i. The destination is kmemdup()'d at NET_DIM_PARAMS_NUM_PROFILES
> entries (5), but the Netlink nest count is entirely user-controlled.
> Netlink policies do not have support for constraining the number
> of nested entries (or number of multi-attr entries).
> 
> Fixes: f750dfe825b9 ("ethtool: provide customized dim profile management")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: haiyangz@microsoft.com
> CC: hengqi@linux.alibaba.com
> ---
>   net/ethtool/coalesce.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/net/ethtool/coalesce.c b/net/ethtool/coalesce.c
> index 1e2c5c7048a8..e73fc3e5a02b 100644
> --- a/net/ethtool/coalesce.c
> +++ b/net/ethtool/coalesce.c
> @@ -472,6 +472,12 @@ static int ethnl_update_profile(struct net_device *dev,
>   
>   	nla_for_each_nested_type(nest, ETHTOOL_A_PROFILE_IRQ_MODERATION,
>   				 nests, rem) {
> +		if (i >= NET_DIM_PARAMS_NUM_PROFILES) {
> +			NL_SET_BAD_ATTR(extack, nest);
> +			ret = -E2BIG;
> +			goto err_out;
> +		}
> +
>   		ret = nla_parse_nested(tb, len_irq_moder - 1, nest,
>   				       coalesce_irq_moderation_policy,
>   				       extack);

[PATCH net 02/10] ethtool: tsconfig: fix reply error handling

[Jakub Kicinski]

A couple of trivial bugs in error handling in tsconfig_send_reply().
If we failed to allocate rskb we need to set the error.
If we did allocate it but failed to send it - we need to remember
to free it.

Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to get/set hwtstamp config")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: vadim.fedorenko@linux.dev
CC: kory.maincent@bootlin.com
---
 net/ethtool/tsconfig.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
index e4f518e49d4c..e9db4ee2299d 100644
--- a/net/ethtool/tsconfig.c
+++ b/net/ethtool/tsconfig.c
@@ -224,16 +224,21 @@ static int tsconfig_send_reply(struct net_device *dev, struct genl_info *info)
 	reply_len = ret + ethnl_reply_header_size();
 	rskb = ethnl_reply_init(reply_len, dev, ETHTOOL_MSG_TSCONFIG_SET_REPLY,
 				ETHTOOL_A_TSCONFIG_HEADER, info, &reply_payload);
-	if (!rskb)
+	if (!rskb) {
+		ret = -ENOMEM;
 		goto err_cleanup;
+	}
 
 	ret = tsconfig_fill_reply(rskb, &req_info->base, &reply_data->base);
 	if (ret < 0)
-		goto err_cleanup;
+		goto err_free_msg;
 
 	genlmsg_end(rskb, reply_payload);
 	ret = genlmsg_reply(rskb, info);
+	rskb = NULL;
 
+err_free_msg:
+	nlmsg_free(rskb);
 err_cleanup:
 	kfree(reply_data);
 	kfree(req_info);
-- 
2.54.0

Re: [PATCH net 02/10] ethtool: tsconfig: fix reply error handling

[Vadim Fedorenko]

On 26/05/2026 16:35, Jakub Kicinski wrote:
> A couple of trivial bugs in error handling in tsconfig_send_reply().
> If we failed to allocate rskb we need to set the error.
> If we did allocate it but failed to send it - we need to remember
> to free it.
> 
> Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to get/set hwtstamp config")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: andrew@lunn.ch
> CC: vadim.fedorenko@linux.dev
> CC: kory.maincent@bootlin.com
> ---
>   net/ethtool/tsconfig.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
> index e4f518e49d4c..e9db4ee2299d 100644
> --- a/net/ethtool/tsconfig.c
> +++ b/net/ethtool/tsconfig.c
> @@ -224,16 +224,21 @@ static int tsconfig_send_reply(struct net_device *dev, struct genl_info *info)
>   	reply_len = ret + ethnl_reply_header_size();
>   	rskb = ethnl_reply_init(reply_len, dev, ETHTOOL_MSG_TSCONFIG_SET_REPLY,
>   				ETHTOOL_A_TSCONFIG_HEADER, info, &reply_payload);
> -	if (!rskb)
> +	if (!rskb) {
> +		ret = -ENOMEM;
>   		goto err_cleanup;
> +	}
>   
>   	ret = tsconfig_fill_reply(rskb, &req_info->base, &reply_data->base);
>   	if (ret < 0)
> -		goto err_cleanup;
> +		goto err_free_msg;
>   
>   	genlmsg_end(rskb, reply_payload);
>   	ret = genlmsg_reply(rskb, info);
> +	rskb = NULL;
>   
> +err_free_msg:
> +	nlmsg_free(rskb);
>   err_cleanup:
>   	kfree(reply_data);
>   	kfree(req_info);

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[PATCH net 03/10] ethtool: linkstate: fix unbalanced ethnl_ops_complete() on PHY lookup error

[Jakub Kicinski]

linkstate_prepare_data() calls ethnl_req_get_phydev() before
ethnl_ops_begin(), but routes its error path through "goto out"
which calls ethnl_ops_complete().

Fixes: fe55b1d401c6 ("ethtool: linkstate: migrate linkstate functions to support multi-PHY setups")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
CC: o.rempel@pengutronix.de
---
 net/ethtool/linkstate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 8a5985fd7712..24569e92942c 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -106,10 +106,8 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 
 	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_LINKSTATE_HEADER,
 				      info->extack);
-	if (IS_ERR(phydev)) {
-		ret = PTR_ERR(phydev);
-		goto out;
-	}
+	if (IS_ERR(phydev))
+		return PTR_ERR(phydev);
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
-- 
2.54.0

Re: [PATCH net 03/10] ethtool: linkstate: fix unbalanced ethnl_ops_complete() on PHY lookup error

[Maxime Chevallier]

Hi Jakub,

On 5/26/26 17:35, Jakub Kicinski wrote:
> linkstate_prepare_data() calls ethnl_req_get_phydev() before
> ethnl_ops_begin(), but routes its error path through "goto out"
> which calls ethnl_ops_complete().
> 
> Fixes: fe55b1d401c6 ("ethtool: linkstate: migrate linkstate functions to support multi-PHY setups")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks for fixing this,

Maxime

> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: o.rempel@pengutronix.de
> ---
>   net/ethtool/linkstate.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
> index 8a5985fd7712..24569e92942c 100644
> --- a/net/ethtool/linkstate.c
> +++ b/net/ethtool/linkstate.c
> @@ -106,10 +106,8 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>   
>   	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_LINKSTATE_HEADER,
>   				      info->extack);
> -	if (IS_ERR(phydev)) {
> -		ret = PTR_ERR(phydev);
> -		goto out;
> -	}
> +	if (IS_ERR(phydev))
> +		return PTR_ERR(phydev);
>   
>   	ret = ethnl_ops_begin(dev);
>   	if (ret < 0)

[PATCH net 04/10] ethtool: pse-pd: fix missing ethnl_ops_complete()

[Jakub Kicinski]

pse_prepare_data() is missing ethnl_ops_complete() if
ethnl_req_get_phydev() returned an error. Move getting
phydev up so that we don't have to worry about this
(similar order to linkstate_prepare_data()).

Note that phydev may still be NULL (this is checked in
pse_get_pse_attributes()), the goal isn't really to avoid
the _begin() / _complete() calls, only to simplify the error
handling.

While at it propagate the original error. Why this code
overrides the error with -ENODEV but !phydev generates
-EOPNOTSUPP is unclear to me...

Fixes: 31748765bed3 ("net: ethtool: pse-pd: Target the command to the requested PHY")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: o.rempel@pengutronix.de
CC: kory.maincent@bootlin.com
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
CC: chleroy@kernel.org
---
 net/ethtool/pse-pd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 2eb9bdc2dcb9..757c9e0cc856 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -62,14 +62,14 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	struct phy_device *phydev;
 	int ret;
 
-	ret = ethnl_ops_begin(dev);
-	if (ret < 0)
-		return ret;
-
 	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
 	if (IS_ERR(phydev))
-		return -ENODEV;
+		return PTR_ERR(phydev);
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
 
 	ret = pse_get_pse_attributes(phydev, info->extack, data);
 
-- 
2.54.0

Re: [PATCH net 04/10] ethtool: pse-pd: fix missing ethnl_ops_complete()

[Maxime Chevallier]

Hi,

On 5/26/26 17:35, Jakub Kicinski wrote:
> pse_prepare_data() is missing ethnl_ops_complete() if
> ethnl_req_get_phydev() returned an error. Move getting
> phydev up so that we don't have to worry about this
> (similar order to linkstate_prepare_data()).
> 
> Note that phydev may still be NULL (this is checked in
> pse_get_pse_attributes()), the goal isn't really to avoid
> the _begin() / _complete() calls, only to simplify the error
> handling.
> 
> While at it propagate the original error. Why this code
> overrides the error with -ENODEV but !phydev generates
> -EOPNOTSUPP is unclear to me...

The logic is that ethnl_req_get_phydev() may return -ENODEV,
meaning "you passed a phy index for a PHY that doesn't
exist", which is different than "there's no PHY, setting PSE-PD
params isn't supported then."

But there's no real sense overriding the phydev ERR_PTR with the same
value :) Your fix should be fine :) Thanks for this !

> 
> Fixes: 31748765bed3 ("net: ethtool: pse-pd: Target the command to the requested PHY")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> CC: o.rempel@pengutronix.de
> CC: kory.maincent@bootlin.com
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: chleroy@kernel.org
> ---
>   net/ethtool/pse-pd.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
> index 2eb9bdc2dcb9..757c9e0cc856 100644
> --- a/net/ethtool/pse-pd.c
> +++ b/net/ethtool/pse-pd.c
> @@ -62,14 +62,14 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
>   	struct phy_device *phydev;
>   	int ret;
>   
> -	ret = ethnl_ops_begin(dev);
> -	if (ret < 0)
> -		return ret;
> -
>   	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PSE_HEADER,
>   				      info->extack);
>   	if (IS_ERR(phydev))
> -		return -ENODEV;
> +		return PTR_ERR(phydev);
> +
> +	ret = ethnl_ops_begin(dev);
> +	if (ret < 0)
> +		return ret;
>   
>   	ret = pse_get_pse_attributes(phydev, info->extack, data);
>   

[PATCH net 05/10] ethtool: tsconfig: fix missing ethnl_ops_complete()

[Jakub Kicinski]

tsconfig_prepare_data() calls ethnl_ops_begin(), we need to call
ethnl_ops_complete() before returning the error.

Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to get/set hwtstamp config")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: vadim.fedorenko@linux.dev
CC: kory.maincent@bootlin.com
---
 net/ethtool/tsconfig.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
index e9db4ee2299d..fc4f93cfa459 100644
--- a/net/ethtool/tsconfig.c
+++ b/net/ethtool/tsconfig.c
@@ -69,8 +69,10 @@ static int tsconfig_prepare_data(const struct ethnl_req_info *req_base,
 		if (ret)
 			goto out;
 
-		if (ts_info.phc_index == -1)
-			return -ENODEV;
+		if (ts_info.phc_index == -1) {
+			ret = -ENODEV;
+			goto out;
+		}
 
 		data->hwprov_desc.index = ts_info.phc_index;
 		data->hwprov_desc.qualifier = ts_info.phc_qualifier;
-- 
2.54.0

Re: [PATCH net 05/10] ethtool: tsconfig: fix missing ethnl_ops_complete()

[Vadim Fedorenko]

On 26/05/2026 16:35, Jakub Kicinski wrote:
> tsconfig_prepare_data() calls ethnl_ops_begin(), we need to call
> ethnl_ops_complete() before returning the error.
> 
> Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to get/set hwtstamp config")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: andrew@lunn.ch
> CC: vadim.fedorenko@linux.dev
> CC: kory.maincent@bootlin.com
> ---
>   net/ethtool/tsconfig.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
> index e9db4ee2299d..fc4f93cfa459 100644
> --- a/net/ethtool/tsconfig.c
> +++ b/net/ethtool/tsconfig.c
> @@ -69,8 +69,10 @@ static int tsconfig_prepare_data(const struct ethnl_req_info *req_base,
>   		if (ret)
>   			goto out;
>   
> -		if (ts_info.phc_index == -1)
> -			return -ENODEV;
> +		if (ts_info.phc_index == -1) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
>   
>   		data->hwprov_desc.index = ts_info.phc_index;
>   		data->hwprov_desc.qualifier = ts_info.phc_qualifier;

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[PATCH net 06/10] ethtool: tsinfo: fix uninitialized stats on the by-PHC path

[Jakub Kicinski]

tsinfo_prepare_data() has two code paths: a "by-PHC" path for
user-specified hardware timestamping providers, and the old path.
Commit 89e281ebff72 ("ethtool: init tsinfo stats if requested") added
ethtool_stats_init() to mark stat slots as ETHTOOL_STAT_NOT_SET before
the driver callback populates them, but placed the call inside the
old-path block.

When commit b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to
support several hwtstamp by net topology") added the by-PHC early
return, it landed above the stats initialization. On that path
the stats array retains the zero-fill from ethnl_init_reply_data()'s
zalloc. This leads to the reply including a stats nest with four
zero-valued attributes that should have been absent.

Reject GET requests for stats with HWTSTAMP_PROVIDER or dump.

Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
CC: kory.maincent@bootlin.com
CC: kees@kernel.org
---
 net/ethtool/tsinfo.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index a865f0fdd26b..f54fe6b662b2 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -83,6 +83,11 @@ tsinfo_parse_request(struct ethnl_req_info *req_base,
 	if (!tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER])
 		return 0;
 
+	if (req_base->flags & ETHTOOL_FLAG_STATS) {
+		NL_SET_ERR_MSG(extack, "can't query statistics for a provider");
+		return -EOPNOTSUPP;
+	}
+
 	return ts_parse_hwtst_provider(tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER],
 				       &req->hwprov_desc, extack, &mod);
 }
@@ -523,6 +528,12 @@ int ethnl_tsinfo_start(struct netlink_callback *cb)
 	if (ret < 0)
 		goto free_reply_data;
 
+	if (req_info->base.flags & ETHTOOL_FLAG_STATS) {
+		NL_SET_ERR_MSG(cb->extack, "stats not supported in dump");
+		ret = -EOPNOTSUPP;
+		goto err_dev_put;
+	}
+
 	ctx->req_info = req_info;
 	ctx->reply_data = reply_data;
 	ctx->pos_ifindex = 0;
@@ -532,6 +543,8 @@ int ethnl_tsinfo_start(struct netlink_callback *cb)
 
 	return 0;
 
+err_dev_put:
+	ethnl_parse_header_dev_put(&req_info->base);
 free_reply_data:
 	kfree(reply_data);
 free_req_info:
-- 
2.54.0

Re: [PATCH net 06/10] ethtool: tsinfo: fix uninitialized stats on the by-PHC path

[Maxime Chevallier]

On 5/26/26 17:35, Jakub Kicinski wrote:
> tsinfo_prepare_data() has two code paths: a "by-PHC" path for
> user-specified hardware timestamping providers, and the old path.
> Commit 89e281ebff72 ("ethtool: init tsinfo stats if requested") added
> ethtool_stats_init() to mark stat slots as ETHTOOL_STAT_NOT_SET before
> the driver callback populates them, but placed the call inside the
> old-path block.
> 
> When commit b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to
> support several hwtstamp by net topology") added the by-PHC early
> return, it landed above the stats initialization. On that path
> the stats array retains the zero-fill from ethnl_init_reply_data()'s
> zalloc. This leads to the reply including a stats nest with four
> zero-valued attributes that should have been absent.
> 
> Reject GET requests for stats with HWTSTAMP_PROVIDER or dump.
> 
> Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

We may have per-provider stats one day, but for now I agree
with this patch.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime


> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: kory.maincent@bootlin.com
> CC: kees@kernel.org
> ---
>   net/ethtool/tsinfo.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
> index a865f0fdd26b..f54fe6b662b2 100644
> --- a/net/ethtool/tsinfo.c
> +++ b/net/ethtool/tsinfo.c
> @@ -83,6 +83,11 @@ tsinfo_parse_request(struct ethnl_req_info *req_base,
>   	if (!tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER])
>   		return 0;
>   
> +	if (req_base->flags & ETHTOOL_FLAG_STATS) {
> +		NL_SET_ERR_MSG(extack, "can't query statistics for a provider");
> +		return -EOPNOTSUPP;
> +	}
> +
>   	return ts_parse_hwtst_provider(tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER],
>   				       &req->hwprov_desc, extack, &mod);
>   }
> @@ -523,6 +528,12 @@ int ethnl_tsinfo_start(struct netlink_callback *cb)
>   	if (ret < 0)
>   		goto free_reply_data;
>   
> +	if (req_info->base.flags & ETHTOOL_FLAG_STATS) {
> +		NL_SET_ERR_MSG(cb->extack, "stats not supported in dump");
> +		ret = -EOPNOTSUPP;
> +		goto err_dev_put;
> +	}
> +
>   	ctx->req_info = req_info;
>   	ctx->reply_data = reply_data;
>   	ctx->pos_ifindex = 0;
> @@ -532,6 +543,8 @@ int ethnl_tsinfo_start(struct netlink_callback *cb)
>   
>   	return 0;
>   
> +err_dev_put:
> +	ethnl_parse_header_dev_put(&req_info->base);
>   free_reply_data:
>   	kfree(reply_data);
>   free_req_info:

[PATCH net 07/10] ethtool: tsinfo: don't pass ERR_PTR to genlmsg_cancel on prepare failure

[Jakub Kicinski]

The goto err label leads to:

	genlmsg_cancel(skb, ehdr);
	return ret;

If ethnl_tsinfo_prepare_dump() failed, it has not started a genlmsg.
There's nothing to cancel, and passing an error pointer to
genlmsg_cancel() would cause a crash.

Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
CC: kory.maincent@bootlin.com
---
 net/ethtool/tsinfo.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index f54fe6b662b2..14bf01e3b55c 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -407,10 +407,8 @@ static int ethnl_tsinfo_dump_one_netdev(struct sk_buff *skb,
 			continue;
 
 		ehdr = ethnl_tsinfo_prepare_dump(skb, dev, reply_data, cb);
-		if (IS_ERR(ehdr)) {
-			ret = PTR_ERR(ehdr);
-			goto err;
-		}
+		if (IS_ERR(ehdr))
+			return PTR_ERR(ehdr);
 
 		reply_data->ts_info.phc_qualifier = ctx->pos_phcqualifier;
 		ret = ops->get_ts_info(dev, &reply_data->ts_info);
-- 
2.54.0

Re: [PATCH net 07/10] ethtool: tsinfo: don't pass ERR_PTR to genlmsg_cancel on prepare failure

[Maxime Chevallier]

Hi,

On 5/26/26 17:35, Jakub Kicinski wrote:
> The goto err label leads to:
> 
> 	genlmsg_cancel(skb, ehdr);
> 	return ret;
> 
> If ethnl_tsinfo_prepare_dump() failed, it has not started a genlmsg.
> There's nothing to cancel, and passing an error pointer to
> genlmsg_cancel() would cause a crash.
> 
> Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: kory.maincent@bootlin.com
> ---
>   net/ethtool/tsinfo.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
> index f54fe6b662b2..14bf01e3b55c 100644
> --- a/net/ethtool/tsinfo.c
> +++ b/net/ethtool/tsinfo.c
> @@ -407,10 +407,8 @@ static int ethnl_tsinfo_dump_one_netdev(struct sk_buff *skb,
>   			continue;
>   
>   		ehdr = ethnl_tsinfo_prepare_dump(skb, dev, reply_data, cb);
> -		if (IS_ERR(ehdr)) {
> -			ret = PTR_ERR(ehdr);
> -			goto err;
> -		}
> +		if (IS_ERR(ehdr))
> +			return PTR_ERR(ehdr);
>   
>   		reply_data->ts_info.phc_qualifier = ctx->pos_phcqualifier;
>   		ret = ops->get_ts_info(dev, &reply_data->ts_info);

[PATCH net 08/10] ethtool: strset: fix header attribute index in ethnl_req_get_phydev()

[Jakub Kicinski]

strset_prepare_data() passes ETHTOOL_A_HEADER_FLAGS (3) as the header
attribute to ethnl_req_get_phydev(). This is incorrect, in the main
attr space 3 is ETHTOOL_A_STRSET_COUNTS_ONLY, not the request
header attr. The correct constant is ETHTOOL_A_STRSET_HEADER (1).

ethnl_req_get_phydev() only uses this value for the extack,
so this is not a "functionally visible"(?) bug.

Fixes: e96c93aa4be9 ("net: ethtool: strset: Allow querying phy stats by index")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
CC: liuhangbin@gmail.com
CC: chleroy@kernel.org
---
 net/ethtool/strset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index bb1e829ba099..94c4718d31ae 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -311,7 +311,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_STRSET_HEADER,
 				      info->extack);
 
 	/* phydev can be NULL, check for errors only */
-- 
2.54.0

Re: [PATCH net 08/10] ethtool: strset: fix header attribute index in ethnl_req_get_phydev()

[Maxime Chevallier]

Hi,

On 5/26/26 17:35, Jakub Kicinski wrote:
> strset_prepare_data() passes ETHTOOL_A_HEADER_FLAGS (3) as the header
> attribute to ethnl_req_get_phydev(). This is incorrect, in the main
> attr space 3 is ETHTOOL_A_STRSET_COUNTS_ONLY, not the request
> header attr. The correct constant is ETHTOOL_A_STRSET_HEADER (1).
> 
> ethnl_req_get_phydev() only uses this value for the extack,
> so this is not a "functionally visible"(?) bug.
> 
> Fixes: e96c93aa4be9 ("net: ethtool: strset: Allow querying phy stats by index")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Indeed, my bad :(

Thanks !

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: liuhangbin@gmail.com
> CC: chleroy@kernel.org
> ---
>   net/ethtool/strset.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
> index bb1e829ba099..94c4718d31ae 100644
> --- a/net/ethtool/strset.c
> +++ b/net/ethtool/strset.c
> @@ -311,7 +311,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_STRSET_HEADER,
>   				      info->extack);
>   
>   	/* phydev can be NULL, check for errors only */

[PATCH net 09/10] ethtool: eeprom: add missing ethnl_ops_begin() / _complete() during fallback

[Jakub Kicinski]

All ethtool driver op calls should be sandwiched between
ethnl_ops_begin() / ethnl_ops_complete(). In Netlink eeprom code,
if the paged access failed we fall back to old API, but we
first call _complete() and the fallback never does its own
ethnl_ops_begin(). Move the fallback into the _begin() / _complete()
section.

Fixes: 96d971e307cc ("ethtool: Add fallback to get_module_eeprom from netlink command")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
---
 net/ethtool/eeprom.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index a557e3996c85..836316df3092 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -141,12 +141,11 @@ static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
 	return 0;
 
 err_ops:
+	if (ret == -EOPNOTSUPP)
+		ret = eeprom_fallback(request, reply);
 	ethnl_ops_complete(dev);
 err_free:
 	kfree(page_data.data);
-
-	if (ret == -EOPNOTSUPP)
-		return eeprom_fallback(request, reply);
 	return ret;
 }
 
-- 
2.54.0

Re: [PATCH net 09/10] ethtool: eeprom: add missing ethnl_ops_begin() / _complete() during fallback

[Maxime Chevallier]

Hi,

On 5/26/26 17:35, Jakub Kicinski wrote:
> All ethtool driver op calls should be sandwiched between
> ethnl_ops_begin() / ethnl_ops_complete(). In Netlink eeprom code,
> if the paged access failed we fall back to old API, but we
> first call _complete() and the fallback never does its own
> ethnl_ops_begin(). Move the fallback into the _begin() / _complete()
> section.
> 
> Fixes: 96d971e307cc ("ethtool: Add fallback to get_module_eeprom from netlink command")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks :)

Maxime

> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> ---
>   net/ethtool/eeprom.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
> index a557e3996c85..836316df3092 100644
> --- a/net/ethtool/eeprom.c
> +++ b/net/ethtool/eeprom.c
> @@ -141,12 +141,11 @@ static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
>   	return 0;
>   
>   err_ops:
> +	if (ret == -EOPNOTSUPP)
> +		ret = eeprom_fallback(request, reply);
>   	ethnl_ops_complete(dev);
>   err_free:
>   	kfree(page_data.data);
> -
> -	if (ret == -EOPNOTSUPP)
> -		return eeprom_fallback(request, reply);
>   	return ret;
>   }
>   

[PATCH net 10/10] ethtool: eeprom: add more safeties to EEPROM Netlink fallback

[Jakub Kicinski]

The Netlink fallback path for reading module EEPROM
(fallback_set_params()) validates that offset < eeprom_len,
but does not check that offset + length stays within eeprom_len.
The ioctl equivalent (ethtool_get_any_eeprom() in ioctl.c) has
always enforced both bounds:

  if (eeprom.offset + eeprom.len > total_len)
      return -EINVAL;

This could lead to surprises in both drivers and device FW.
Add the missing offset + length validation to fallback_set_params(),
mirroring the ioctl.

Similarly - ethtool core in general, and ethtool_get_any_eeprom()
in particular tries to zero-init all buffers passed to the drivers
to avoid any extra work of zeroing things out. eeprom_fallback()
uses a plain kmalloc(), change it to zalloc.

Fixes: 96d971e307cc ("ethtool: Add fallback to get_module_eeprom from netlink command")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
CC: vladyslavt@nvidia.com
---
 net/ethtool/eeprom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index 836316df3092..0b8cfeddb014 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -44,6 +44,9 @@ static int fallback_set_params(struct eeprom_req_info *request,
 	if (offset >= modinfo->eeprom_len)
 		return -EINVAL;
 
+	if (length > modinfo->eeprom_len - offset)
+		return -EINVAL;
+
 	eeprom->cmd = ETHTOOL_GMODULEEEPROM;
 	eeprom->len = length;
 	eeprom->offset = offset;
@@ -69,7 +72,7 @@ static int eeprom_fallback(struct eeprom_req_info *request,
 	if (err < 0)
 		return err;
 
-	data = kmalloc(eeprom.len, GFP_KERNEL);
+	data = kzalloc(eeprom.len, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 	err = ethtool_get_module_eeprom_call(dev, &eeprom, data);
-- 
2.54.0

Re: [PATCH net 10/10] ethtool: eeprom: add more safeties to EEPROM Netlink fallback

[Maxime Chevallier]

Hi,

On 5/26/26 17:35, Jakub Kicinski wrote:
> The Netlink fallback path for reading module EEPROM
> (fallback_set_params()) validates that offset < eeprom_len,
> but does not check that offset + length stays within eeprom_len.
> The ioctl equivalent (ethtool_get_any_eeprom() in ioctl.c) has
> always enforced both bounds:
> 
>    if (eeprom.offset + eeprom.len > total_len)
>        return -EINVAL;
> 
> This could lead to surprises in both drivers and device FW.
> Add the missing offset + length validation to fallback_set_params(),
> mirroring the ioctl.
> 
> Similarly - ethtool core in general, and ethtool_get_any_eeprom()
> in particular tries to zero-init all buffers passed to the drivers
> to avoid any extra work of zeroing things out. eeprom_fallback()
> uses a plain kmalloc(), change it to zalloc.
> 
> Fixes: 96d971e307cc ("ethtool: Add fallback to get_module_eeprom from netlink command")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: vladyslavt@nvidia.com
> ---
>   net/ethtool/eeprom.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
> index 836316df3092..0b8cfeddb014 100644
> --- a/net/ethtool/eeprom.c
> +++ b/net/ethtool/eeprom.c
> @@ -44,6 +44,9 @@ static int fallback_set_params(struct eeprom_req_info *request,
>   	if (offset >= modinfo->eeprom_len)
>   		return -EINVAL;
>   
> +	if (length > modinfo->eeprom_len - offset)
> +		return -EINVAL;
> +
>   	eeprom->cmd = ETHTOOL_GMODULEEEPROM;
>   	eeprom->len = length;
>   	eeprom->offset = offset;
> @@ -69,7 +72,7 @@ static int eeprom_fallback(struct eeprom_req_info *request,
>   	if (err < 0)
>   		return err;
>   
> -	data = kmalloc(eeprom.len, GFP_KERNEL);
> +	data = kzalloc(eeprom.len, GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
>   	err = ethtool_get_module_eeprom_call(dev, &eeprom, data);