netdev - Re: [PATCH net-next v2] net: Add options as a flexible array to struct ip_tunnel

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 for Android: free password hash cracker in your pocket

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

Message-ID: <d98d0c20-741d-4d87-b39e-5aa8eed4624c@nvidia.com>
Date: Wed, 12 Feb 2025 20:13:28 +0200
From: Gal Pressman <gal@...dia.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 Tariq Toukan <tariqt@...dia.com>, Louis Peens <louis.peens@...igine.com>,
 Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
 Pravin B Shelar <pshelar@....org>, Yotam Gigi <yotam.gi@...il.com>,
 Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
 Jiri Pirko <jiri@...nulli.us>, Kees Cook <kees@...nel.org>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>, dev@...nvswitch.org,
 linux-hardening@...r.kernel.org, Ilya Maximets <i.maximets@....org>,
 Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net-next v2] net: Add options as a flexible array to
 struct ip_tunnel_info

On 12/02/2025 18:29, Alexander Lobakin wrote:
> From: Gal Pressman <gal@...dia.com>
> Date: Wed, 12 Feb 2025 16:09:53 +0200
> 
>> Remove the hidden assumption that options are allocated at the end of
>> the struct, and teach the compiler about them using a flexible array.
> 
> [...]
> 
>> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
>> index 84c15402931c..4160731dcb6e 100644
>> --- a/include/net/dst_metadata.h
>> +++ b/include/net/dst_metadata.h
>> @@ -163,11 +163,8 @@ static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb)
>>  	if (!new_md)
>>  		return ERR_PTR(-ENOMEM);
>>  
>> -	unsafe_memcpy(&new_md->u.tun_info, &md_dst->u.tun_info,
>> -		      sizeof(struct ip_tunnel_info) + md_size,
>> -		      /* metadata_dst_alloc() reserves room (md_size bytes) for
>> -		       * options right after the ip_tunnel_info struct.
>> -		       */);
>> +	memcpy(&new_md->u.tun_info, &md_dst->u.tun_info,
>> +	       sizeof(struct ip_tunnel_info) + md_size);
>>  #ifdef CONFIG_DST_CACHE
>>  	/* Unclone the dst cache if there is one */
>>  	if (new_md->u.tun_info.dst_cache.cache) {
>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>> index 1aa31bdb2b31..517f78070be0 100644
>> --- a/include/net/ip_tunnels.h
>> +++ b/include/net/ip_tunnels.h
>> @@ -93,12 +93,6 @@ struct ip_tunnel_encap {
>>  	GENMASK((sizeof_field(struct ip_tunnel_info,		\
>>  			      options_len) * BITS_PER_BYTE) - 1, 0)
>>  
>> -#define ip_tunnel_info_opts(info)				\
>> -	_Generic(info,						\
>> -		 const struct ip_tunnel_info * : ((const void *)((info) + 1)),\
>> -		 struct ip_tunnel_info * : ((void *)((info) + 1))\
>> -	)
> 
> You could leave this macro inplace and just change `(info) + 1` to
> `(info)->options` avoiding changes in lots of files and adding casts
> everywhere.

I'd rather not, having a macro to do 'info->options' doesn't help code
readability IMHO.

> 
>> -
>>  struct ip_tunnel_info {
>>  	struct ip_tunnel_key	key;
>>  	struct ip_tunnel_encap	encap;
>> @@ -107,6 +101,7 @@ struct ip_tunnel_info {
>>  #endif
>>  	u8			options_len;
>>  	u8			mode;
>> +	u8			options[] __aligned(sizeof(void *)) __counted_by(options_len);
> 
> Since 96 % 16 == 0, I'd check if __aligned_largest would change the
> bytecode anyhow... Sometimes it does.

__aligned_largest aligns the field to 16 bytes, so the struct remains
the same (96 bytes).
When should __aligned_larget be used? What needs 16 bytes alignment?