On 11/18/25 3:31 PM, Michael S. Tsirkin wrote:
> On Tue, Nov 18, 2025 at 08:38:59AM -0600, Daniel Jurgens wrote:
>> Add support for IP_USER type rules from ethtool.
>>
>> +static void parse_ip4(struct iphdr *mask, struct iphdr *key,
>> + const struct ethtool_rx_flow_spec *fs)
>> +{
>> + const struct ethtool_usrip4_spec *l3_mask = &fs->m_u.usr_ip4_spec;
>> + const struct ethtool_usrip4_spec *l3_val = &fs->h_u.usr_ip4_spec;
>> +
>> + mask->saddr = l3_mask->ip4src;
>> + mask->daddr = l3_mask->ip4dst;
>> + key->saddr = l3_val->ip4src;
>> + key->daddr = l3_val->ip4dst;
>> +
>> + if (l3_mask->proto) {
>
> you seem to check mask for proto here but the ethtool_usrip4_spec doc
> seems to say the mask for proto must be 0.
>
>
> what gives?
>
Then for user_ip flows ethtool should provide 0 as the mask, and based
on your comment below I'm verifying that.
I can move this hunk to the TCP/UDP patch if you prefer.
>
>> + mask->protocol = l3_mask->proto;
>> + key->protocol = l3_val->proto;
>> + }
>> +}
>> + size_t size = sizeof(struct ethhdr);
>> +
>> *num_hdrs = 1;
>> *key_size = sizeof(struct ethhdr);
>
> So *key_size is assigned here ...
>
>> +
>> + if (fs->flow_type == ETHER_FLOW)
>> + goto done;
>> +
>> + ++(*num_hdrs);
>> + if (has_ipv4(fs->flow_type))
>> + size += sizeof(struct iphdr);
>> +
>
> ... never used
>
>> +done:
>> + *key_size = size;
>
> and over-written here.
>
>
> what is going on here, is that this is spaghetti code
> misusing goto for if instructions which obscures the flow.
>
> It should be if (fs->flow_type != ETHER_FLOW) {
>
> ... rest of code ...
> }
>
> and then it will be clear doing *key_size = size once is enough.
>
Done
>
>> /*
>> +
>> + if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
>> + fs->h_u.usr_ip4_spec.tos ||
>> + fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4)
>> + return -EOPNOTSUPP;
>
> So include/uapi/linux/ethtool.h says:
>
> * struct ethtool_usrip4_spec - general flow specification for IPv4
> * @ip4src: Source host
> * @ip4dst: Destination host
> * @l4_4_bytes: First 4 bytes of transport (layer 4) header
> * @tos: Type-of-service
> * @ip_ver: Value must be %ETH_RX_NFC_IP4; mask must be 0
> * @proto: Transport protocol number; mask must be 0
>
> I guess this ETH_RX_NFC_IP4 check validates that userspace follows this
> documentation? But then shouldn't you check the mask
> as well? and mask for proto?
>
Done
>
>
>
>> - setup_eth_hdr_key_mask(selector, key, fs);
>> + setup_eth_hdr_key_mask(selector, key, fs, num_hdrs);
>> + if (num_hdrs == 1)
>> + goto validate;
>
>
> Please stop abusing goto's for if.
> this is not error handling, not breaking out of loops ...
>
>
> please do not.
>
Done
>
>> +
>> + selector = next_selector(selector);
>> +
>> + err = setup_ip_key_mask(selector, key + sizeof(struct ethhdr), fs);
>> + if (err)
>> + goto err_classifier;
>>
>> +validate:
>> err = validate_classifier_selectors(ff, classifier, num_hdrs);
>> if (err)
>> goto err_key;
>> --
>> 2.50.1
>
