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

[Jakub Kicinski <kuba@kernel.org>]

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?

> +};
> +
>  /* Bareudp section  */
>  enum {
>  	IFLA_BAREUDP_UNSPEC,
-- 
pw-bot: cr