netdev - Re: [PATCH net-next 1/2] geneve: Allow users to specify source port range

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] [day] [month] [year] [list]

Message-ID: <d98fe9b7-6206-4be0-89e2-312e25909a04@iogearbox.net>
Date: Wed, 26 Feb 2025 08:03:45 +0100
From: Daniel Borkmann <daniel@...earbox.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] geneve: Allow users to specify source port
 range

On 2/26/25 3:26 AM, Jakub Kicinski wrote:
> On Mon, 24 Feb 2025 16:39:26 +0100 Daniel Borkmann wrote:
>> @@ -1083,8 +1087,8 @@ static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
>>   
>>   		use_cache = ip_tunnel_dst_cache_usable(skb, info);
>>   		tos = geneve_get_dsfield(skb, dev, info, &use_cache);
>> -		sport = udp_flow_src_port(geneve->net, skb,
>> -					  1, USHRT_MAX, true);
>> +		sport = udp_flow_src_port(geneve->net, skb, geneve->cfg.port_min,
> 
> nit: we do still prefer breaking at 80 columns if it doesn't make code
> less readable.
> 
>> +					  geneve->cfg.port_max, true);
>>   
>>   		rt = udp_tunnel_dst_lookup(skb, dev, geneve->net, 0, &saddr,
>>   					   &info->key,
> 
>> @@ -1279,6 +1284,17 @@ static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
>>   		}
>>   	}
>>   
>> +	if (data[IFLA_GENEVE_PORT_RANGE]) {
>> +		const struct ifla_geneve_port_range *p =
>> +			nla_data(data[IFLA_GENEVE_PORT_RANGE]);
> 
> nit: would be more readable as fully separate assignment
> 
>> +		if (p->high < p->low) {
>> +			NL_SET_ERR_MSG_ATTR(extack, data[IFLA_GENEVE_PORT_RANGE],
>> +					    "Invalid source port range");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>   	return 0;
>>   }
> 
>> @@ -1450,6 +1451,11 @@ enum ifla_geneve_df {
>>   	GENEVE_DF_MAX = __GENEVE_DF_END - 1,
>>   };
>>   
>> +struct ifla_geneve_port_range {
>> +	__u16 low;
>> +	__u16 high;
> 
> I agree with the choice in abstract, but since VXLAN uses byte swapped
> fields I think we may be setting an annoying trap for user space
> implementations. I'd err on the side of consistency. No?

If this is preferred I can change it, it's an odd/useless back and forth
dance for sure in vxlan, but if consistency is preferred I'll change it
this way and note it in the commit message.

>> +};
>> +
>>   /* Bareudp section  */
>>   enum {
>>   	IFLA_BAREUDP_UNSPEC,