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

[Stanislav Fomichev <sdf.kernel@gmail.com>]

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?