Re: [net PATCH 4/4] net: ethtool: keep rtnl_lock for the ioctl self test
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-05 20:37:32
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 1 · Medium: 0 · Low: 0 - [High] Incomplete fix: the rtnl requirement for ETHTOOL_TEST is made a per-driver opt-in (ETHTOOL_OP_NEEDS_RTNL_TEST) and set only on fbnic,… -- net: ethtool: keep rtnl_lock for the ioctl self test The ETHTOOL_TEST ioctl on an ops-locked driver runs with only the netdev instance lock, so fbnic's offline self test tears the device down through netif_close() without rtnl_lock. The patch adds an opt-in bit, ETHTOOL_OP_NEEDS_RTNL_TEST, handled in ethtool_ioctl_needs_rtnl(), and sets it in fbnic_ethtool_ops.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c index 0e47088ec44b..423f179c9d47 100644 --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c@@ -2025,7 +2025,8 @@ static const struct ethtool_ops fbnic_ethtool_ops = { ETHTOOL_OP_NEEDS_RTNL_SPAUSEPARAM | ETHTOOL_OP_NEEDS_RTNL_SCHANNELS | ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM | - ETHTOOL_OP_NEEDS_RTNL_GLINK, + ETHTOOL_OP_NEEDS_RTNL_GLINK | + ETHTOOL_OP_NEEDS_RTNL_TEST, .get_drvinfo = fbnic_get_drvinfo, .get_regs_len = fbnic_get_regs_len, .get_regs = fbnic_get_regs,
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/net/ethtool/common.h b/net/ethtool/common.h index 4e5356e26f40..ae32e7fdb563 100644 --- a/net/ethtool/common.h +++ b/net/ethtool/common.h@@ -163,6 +163,8 @@ ethtool_ioctl_needs_rtnl(const struct net_device *dev, u32 ethcmd) return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_RSS; case ETHTOOL_GLINK: return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_GLINK; + case ETHTOOL_TEST: + return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_TEST; } return false; }
[Severity: High]
Making the rtnl requirement for ETHTOOL_TEST a per-driver opt-in leaves the
same condition reachable on bnxt, doesn't it?
bnxt is ops-locked, since bnxt_init_one() sets dev->queue_mgmt_ops
unconditionally:
dev->queue_mgmt_ops = &bnxt_queue_mgmt_ops_unsupp;
if (BNXT_SUPPORTS_QUEUE_API(bp))
dev->queue_mgmt_ops = &bnxt_queue_mgmt_ops;
But bnxt_ethtool_ops does not carry the new bit:
.op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
ETHTOOL_OP_NEEDS_RTNL_SCOALESCE |
ETHTOOL_OP_NEEDS_RTNL_RSS,
so with this patch applied, __dev_ethtool() still computes need_rtnl == false
for ETHTOOL_TEST on bnxt, and bnxt_self_test() runs under
netdev_lock_ops() only.
The offline branch of bnxt_self_test() then does:
bnxt_close_nic(bp, true, false);
...
rc = bnxt_half_open_nic(bp);
...
bnxt_half_close_nic(bp);
rc = bnxt_open_nic(bp, true, true);
and both bnxt_half_open_nic() and bnxt_open_nic() call netif_close() on
failure:
drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_half_open_nic() {
...
half_open_err:
bnxt_free_skbs(bp);
bnxt_free_mem(bp, true);
netif_close(bp->dev);
return rc;
}
IFF_UP is still set at that point, since __bnxt_close_nic() never clears it,
so netif_close() goes into netif_close_many() -> __dev_close_many() with its
ASSERT_RTNL() and the rtnl-protected rcu_dereference_protected() in
netpoll_poll_disable(), which is the same call chain quoted in the commit
message, only on bnxt.
Would it make sense to either set ETHTOOL_OP_NEEDS_RTNL_TEST on bnxt as well,
or to keep rtnl_lock unconditionally for ETHTOOL_TEST on the ioctl path?
Given self_test is an ioctl-only and rarely used command, an unconditional
case here would also cover any future ops-locked driver that forgets the
opt-in.
I checked the other ops-locked drivers for the same exposure: mlx5's
mlx5e_self_test() only runs loopback under priv->state_lock and never calls
netif_close()/netif_open(), and gve, netdevsim, netkit, dummy, bnge and
mlxsw do not implement .self_test.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/178838795612.11136.15512516427273655916.stgit%40ahduyck-xeon-server.home.arpa