Re: [net PATCH 4/4] net: ethtool: keep rtnl_lock for the ioctl self test

[netdev-bot+sashiko@kernel.org]

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.

> 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,

[ ... ]

> 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