netdev - Re: [PATCH net-next v11 09/12] virtio_net: Implement IPv4 ethtool flow rules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d2b57943-0991-4823-9997-2bd6044c7abc@nvidia.com>
Date: Wed, 19 Nov 2025 01:03:36 -0600
From: Dan Jurgens <danielj@...dia.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: netdev@...r.kernel.org, jasowang@...hat.com, pabeni@...hat.com,
 virtualization@...ts.linux.dev, parav@...dia.com, shshitrit@...dia.com,
 yohadt@...dia.com, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com,
 jgg@...pe.ca, kevin.tian@...el.com, kuba@...nel.org, andrew+netdev@...n.ch,
 edumazet@...gle.com
Subject: Re: [PATCH net-next v11 09/12] virtio_net: Implement IPv4 ethtool
 flow rules

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
>