[PATCH net-next] net: ethtool: request rtnl_lock for ETHTOOL_MSG_PORT_GET

[PATCH net-next] net: ethtool: request rtnl_lock for ETHTOOL_MSG_PORT_GET

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

port_prepare_data() resolves the requested port through
phy_link_topo_get_port(), which requires rtnl_lock to be held before it
reads topo->ports:

	/* ports in the topo are RTNL protected, but may be accessed under
	 * netdev_lock for ops-locked devices. For now keep them under rtnl
	 * protection, as no ops-locked devices have phy_port capabilities
	 * yet.
	 */
	ASSERT_RTNL();

The dump side honours this, port_dump_one_dev() takes rtnl_lock()
itself. The doit side does not. ETHTOOL_MSG_PORT_GET is served by
ethnl_default_doit(), which keeps rtnl_lock only when the device is not
ops-locked, or when ethtool_nl_msg_needs_rtnl() asks for it:

	need_rtnl = !netdev_need_ops_lock(req_info->dev) ||
		    ethtool_nl_msg_needs_rtnl(req_info->dev, cmd);

ethtool_nl_msg_needs_rtnl() has no case for ETHTOOL_MSG_PORT_GET and
falls through to "return false", so on a device that opts into the
instance lock (request_ops_lock, queue_mgmt_ops or net_shaper_ops)
PORT_GET runs with netdev_lock only. Such a device that does have a
link topology trips the ASSERT_RTNL() warning and reads topo->ports
without the protection the xarray is documented to need.

Add the missing case. Like TSCONFIG_GET/SET, PORT_GET has no
corresponding ethtool ioctl, so it is Netlink-only and always needs
rtnl_lock.

The bug is latent as no device opts into the instance lock while also
carrying a link topology, so it cannot be observed yet and the change is
compile-tested only.

This was found during an audit of net/ethtool/ performed with the help
of an AI coding assistant (Codex), which also proposed the analysis and
the fix. I verified the Fixes commit and the callers of
ethtool_nl_msg_needs_rtnl().

Fixes: 27859d18bf41 ("net: ethtool: Introduce ethtool command to list ports")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 net/ethtool/common.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index b062e99e89db..372304722ecc 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -124,6 +124,14 @@ ethtool_nl_msg_needs_rtnl(const struct net_device *dev, u8 cmd)
 		 * these cases are Netlink-only.
 		 */
 		return true;
+	case ETHTOOL_MSG_PORT_GET:
+		/*
+		 * port_prepare_data() resolves the port through
+		 * phy_link_topo_get_port(), which asserts rtnl_lock before
+		 * reading topo->ports. There is no corresponding ethtool
+		 * ioctl, so this case is Netlink-only.
+		 */
+		return true;
 	}
 	return false;
 }
-- 
2.25.1

Re: [PATCH net-next] net: ethtool: request rtnl_lock for ETHTOOL_MSG_PORT_GET

[Stanislav Fomichev]

On 11/11, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> port_prepare_data() resolves the requested port through
> phy_link_topo_get_port(), which requires rtnl_lock to be held before it
> reads topo->ports:
> 
> 	/* ports in the topo are RTNL protected, but may be accessed under
> 	 * netdev_lock for ops-locked devices. For now keep them under rtnl
> 	 * protection, as no ops-locked devices have phy_port capabilities
> 	 * yet.
> 	 */
> 	ASSERT_RTNL();
> 
> The dump side honours this, port_dump_one_dev() takes rtnl_lock()
> itself. The doit side does not. ETHTOOL_MSG_PORT_GET is served by
> ethnl_default_doit(), which keeps rtnl_lock only when the device is not
> ops-locked, or when ethtool_nl_msg_needs_rtnl() asks for it:
> 
> 	need_rtnl = !netdev_need_ops_lock(req_info->dev) ||
> 		    ethtool_nl_msg_needs_rtnl(req_info->dev, cmd);
> 
> ethtool_nl_msg_needs_rtnl() has no case for ETHTOOL_MSG_PORT_GET and
> falls through to "return false", so on a device that opts into the
> instance lock (request_ops_lock, queue_mgmt_ops or net_shaper_ops)
> PORT_GET runs with netdev_lock only. Such a device that does have a
> link topology trips the ASSERT_RTNL() warning and reads topo->ports
> without the protection the xarray is documented to need.
> 
> Add the missing case. Like TSCONFIG_GET/SET, PORT_GET has no
> corresponding ethtool ioctl, so it is Netlink-only and always needs
> rtnl_lock.

[..]

> The bug is latent as no device opts into the instance lock while also
> carrying a link topology, so it cannot be observed yet and the change is
> compile-tested only.

Then why fix it if there is no bug? And phy_link_topo_add_port has
a big comment explaining the expectations?

Re: [PATCH net-next] net: ethtool: request rtnl_lock for ETHTOOL_MSG_PORT_GET

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix covers only one of several commands with the same bug.

--

net: ethtool: request rtnl_lock for ETHTOOL_MSG_PORT_GET

This commit adds an ETHTOOL_MSG_PORT_GET case to
ethtool_nl_msg_needs_rtnl(), so that ethnl_default_doit() keeps rtnl_lock
for PORT_GET on ops-locked devices. It does this because
phy_link_topo_get_port() asserts RTNL before it reads topo->ports.

> diff --git a/net/ethtool/common.h b/net/ethtool/common.h
> index b062e99e89dbf..372304722ecc4 100644
> --- a/net/ethtool/common.h
> +++ b/net/ethtool/common.h
> @@ -124,6 +124,14 @@ ethtool_nl_msg_needs_rtnl(const struct net_device *dev, u8 cmd)
>  		 * these cases are Netlink-only.
>  		 */
>  		return true;
> +	case ETHTOOL_MSG_PORT_GET:
> +		/*
> +		 * port_prepare_data() resolves the port through
> +		 * phy_link_topo_get_port(), which asserts rtnl_lock before
> +		 * reading topo->ports. There is no corresponding ethtool
> +		 * ioctl, so this case is Netlink-only.
> +		 */
> +		return true;
>  	}
>  	return false;
>  }

[Severity: High]
Is PORT_GET the only command with this problem? The commit message says
an ops-locked device with a link topology "trips the ASSERT_RTNL()
warning", and it calls this patch "the missing case". However,
ethnl_req_get_phydev() checks the same condition before it reads the
topology:

net/ethtool/netlink.c:ethnl_req_get_phydev() {
    ...
	/* If there is no PHY in sight there's no need for assert locking */
	if (!phy_link_topo_empty(req_info->dev))
		ASSERT_RTNL();

	if (!req_info->phy_index)
		return req_info->dev->phydev;

	phydev = phy_link_topo_get_phy(req_info->dev, req_info->phy_index);
    ...
}

The prepare_data or set callbacks of these commands call
ethnl_req_get_phydev() unconditionally:

  STRSET_GET      strset_prepare_data()
  LINKSTATE_GET   linkstate_prepare_data()
  STATS_GET
  PHY_GET
  PLCA_GET_CFG, PLCA_SET_CFG, PLCA_GET_STATUS
  PSE_GET, PSE_SET
  MSE_GET

All of them go through ethnl_default_doit(), ethnl_default_dump_one() or
ethnl_default_set_doit(), which compute:

	need_rtnl = !netdev_need_ops_lock(req_info->dev) ||
		    ethtool_nl_msg_needs_rtnl(req_info->dev, cmd);

ethtool_nl_msg_needs_rtnl() still has no case for these commands, so they
fall through to "return false". LINKSTATE_GET is covered only when the
driver sets ETHTOOL_OP_NEEDS_RTNL_GLINK.

The cable test handlers ethnl_act_cable_test() and
ethnl_act_cable_test_tdr() have the same gap. They call
ethnl_req_get_phydev() after netdev_lock_ops_compat(), which takes only
netdev_lock() on an ops-locked device.

So take the device the commit message describes, one that is ops-locked
and has a non-NULL dev->link_topo. Wouldn't a plain "ethtool <dev>" still
hit the ASSERT_RTNL() WARN through STRSET_GET or LINKSTATE_GET, before
PORT_GET is ever issued? Those paths would also still look up topo->phys
and use the returned phy_device without RTNL. That could race with PHY
removal, for example an SFP unplug, which runs under RTNL.

As with PORT_GET, this is latent today. None of the in-tree drivers that
use request_ops_lock, queue_mgmt_ops or net_shaper_ops register PHYs into
the link topology.

The PORT_GET change on its own looks correct. Should the patch either
cover these sibling commands or narrow what the commit message claims?
Some ways to cover them:

  - add a case for each of these commands
  - force need_rtnl whenever !phy_link_topo_empty(dev)
  - move the topology xarrays under the instance lock

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111072724.3295527-1-xiaolinkui%40126.com