Re: [net-next,v23,14/14] virtio_net: Add get ethtool flow rules ops

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The two new getters report the UAPI-documented "size of the
  rule table" (struct ethtool_rxnfc::data) with two contradictory values.

--

virtio_net: Add get ethtool flow rules ops

This adds a get_rxnfc ethtool op to virtio_net handling
ETHTOOL_GRXCLSRLCNT, ETHTOOL_GRXCLSRULE and ETHTOOL_GRXCLSRLALL, so
userspace can query the rule count, a single rule, and the list of rule
locations.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a1e042f356f5e..911a5142605bd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -6575,6 +6575,60 @@ static int virtnet_ethtool_flow_remove(struct virtnet_ff *ff, int location)
>  	return err;
>  }
>  
> +static int virtnet_ethtool_get_flow_count(struct virtnet_ff *ff,
> +					  struct ethtool_rxnfc *info)
> +{
> +	if (!ff->ff_supported)
> +		return -EOPNOTSUPP;
> +
> +	info->rule_cnt = ff->ethtool.num_rules;
> +	info->data = min(le32_to_cpu(ff->ff_caps->rules_limit),
> +			 le32_to_cpu(ff->ff_caps->rules_per_group_limit)) |
> +		     RX_CLS_LOC_SPECIAL;
> +
> +	return 0;
> +}
> +

[ ... ]

> +static int
> +virtnet_ethtool_get_all_flows(struct virtnet_ff *ff,
> +			      struct ethtool_rxnfc *info, u32 *rule_locs)
> +{
> +	struct virtnet_ethtool_rule *eth_rule;
> +	unsigned long i = 0;
> +	int idx = 0;
> +
> +	if (!ff->ff_supported)
> +		return -EOPNOTSUPP;
> +
> +	xa_for_each(&ff->ethtool.rules, i, eth_rule) {
> +		if (idx == info->rule_cnt)
> +			return -EMSGSIZE;
> +		rule_locs[idx++] = i;
> +	}
> +
> +	info->data = le32_to_cpu(ff->ff_caps->rules_limit);

[Severity: Medium]
Should these two handlers report the same value in info->data?

include/uapi/linux/ethtool.h documents the field the same way for both
commands:

 * For %ETHTOOL_GRXCLSRLCNT, ... If @data is non-zero on return then it is the
 * size of the rule table, plus the flag %RX_CLS_LOC_SPECIAL ...
 * For %ETHTOOL_GRXCLSRLALL, ... On return, @data is the size of the rule table

virtnet_ethtool_get_flow_count() reports:

	info->data = min(le32_to_cpu(ff->ff_caps->rules_limit),
			 le32_to_cpu(ff->ff_caps->rules_per_group_limit)) |
		     RX_CLS_LOC_SPECIAL;

while virtnet_ethtool_get_all_flows() reports the unclamped
ff_caps->rules_limit.  On a device that advertises
rules_per_group_limit < rules_limit these two answers differ for the same
table.

Since every ethtool rule goes into one group (VIRTNET_FF_MAX_GROUPS is 1
and insert_rule() hardcodes the group), isn't the usable capacity
min(rules_limit, rules_per_group_limit)?

	ff_rule->group_id = cpu_to_le32(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);

If so, does ETHTOOL_GRXCLSRLALL overstate the table size here, so that
inserts past rules_per_group_limit get refused by the device?

There is a related question about the location space itself.
virtnet_ethtool_flow_insert() allocates locations from the unclamped
limit:

	err = xa_alloc(&ff->ethtool.rules, &fs->location, eth_rule,
		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->rules_limit) - 1),
		       GFP_KERNEL);

so locations handed back in fs.location, and then reported in
rule_locs[] by virtnet_ethtool_get_all_flows(), can exceed the table size
now advertised by ETHTOOL_GRXCLSRLCNT.  Should all three sites use the
same limit?

> +	info->rule_cnt = idx;
> +
> +	return 0;
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com