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
