Re: [PATCH net-next 02/14] net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked

[Jakub Sitnicki <jakub@cloudflare.com>]

On Tue, Jun 02, 2026 at 01:20 PM +02, Nicolai Buchwitz wrote:
> Hi Jakub ^ 2
>
> On 2.6.2026 11:17, Jakub Sitnicki wrote:
>> On Thu, May 28, 2026 at 04:16 PM -07, Jakub Kicinski wrote:
>>> __ethtool_get_link_ksettings() is exported and called from sysfs
>>> and many drivers. Looks like commit 2bcf4772e45a ("net: ethtool:
>>> try to protect all callback with netdev instance lock")
>>> missed adding the lock around it. Not treating this as a fix because
>>> I don't think any driver cares at this point, but if we want to
>>> remove the rtnl_lock protection this will become critical.
>>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>
>> [...]
>
>>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>
>> [...]
>
>>>  /* Internal kernel helper to query a device ethtool_link_settings. */
>>> -int __ethtool_get_link_ksettings(struct net_device *dev,
>>> -				 struct ethtool_link_ksettings *link_ksettings)
>>> +int ethtool_get_link_ksettings_locked(struct net_device *dev,
>>> +				      struct ethtool_link_ksettings *link_ksettings)
>>>  {
>>> -	ASSERT_RTNL();
>>> +	netdev_ops_assert_locked(dev);
>> Not sure why we're using the "compat" assertion here, which falls back
>> to check if RTNL is held, instead of the newly introduced
>> netdev_assert_locked_if_ops().
>> The contract here is that all callers are expected to hold the
>> netdev lock (if needed), IIUC.
>> [...]
>
> AFAIU that's on purpose: the original helper had ASSERT_RTNL(), and
> legacy callers still enter with rtnl_lock via the wrapper. _if_ops
> would silently pass for them if rtnl ever went missing.

But the legacy callers continue using __ethtool_get_link_ksettings(),
which has an explicit ASSERT_RTNL(), so I still don't get it:

| +/* Convenience helper for callers that hold only rtnl_lock(). */
| +int __ethtool_get_link_ksettings(struct net_device *dev,
| +				 struct ethtool_link_ksettings *link_ksettings)
| +{
| +	int ret;
| +
| +	ASSERT_RTNL();
| +
| +	netdev_lock_ops(dev);
| +	ret = ethtool_get_link_ksettings_locked(dev, link_ksettings);
| +	netdev_unlock_ops(dev);
| +	return ret;
| +}
|  EXPORT_SYMBOL(__ethtool_get_link_ksettings);

Anyway, no sense speculating. Let's wait for Jakub.