Re: [net-next,v23,14/14] virtio_net: Add get ethtool flow rules ops
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-02 13:12:33
Also in:
virtualization
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.
quoted hunk ↗ jump to hunk
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