[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?