netdev - Re: [PATCH v2 net-next 6/6] udp: Introduce optional per-netns hash table.

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]

Date:   Fri, 11 Nov 2022 09:53:31 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 6/6] udp: Introduce optional per-netns hash
 table.

On Thu, 2022-11-10 at 20:00 -0800, Kuniyuki Iwashima wrote:
> @@ -408,6 +409,28 @@ static int proc_tcp_ehash_entries(struct ctl_table *table, int write,
>  	return proc_dointvec(&tbl, write, buffer, lenp, ppos);
>  }
>  
> +static int proc_udp_hash_entries(struct ctl_table *table, int write,
> +				 void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	struct net *net = container_of(table->data, struct net,
> +				       ipv4.sysctl_udp_child_hash_entries);
> +	int udp_hash_entries;
> +	struct ctl_table tbl;
> +
> +	udp_hash_entries = net->ipv4.udp_table->mask + 1;
> +
> +	/* A negative number indicates that the child netns
> +	 * shares the global udp_table.
> +	 */
> +	if (!net_eq(net, &init_net) && net->ipv4.udp_table == &udp_table)
> +		udp_hash_entries *= -1;
> +
> +	tbl.data = &udp_hash_entries;
> +	tbl.maxlen = sizeof(int);

I see the procfs code below will only use tbl.data and tbl.maxlen, but
perhaps is cleaner intially explicitly memset tbl to 0 

> 
> +
> +	return proc_dointvec(&tbl, write, buffer, lenp, ppos);
> +}
> +
>  #ifdef CONFIG_IP_ROUTE_MULTIPATH
>  static int proc_fib_multipath_hash_policy(struct ctl_table *table, int write,
>  					  void *buffer, size_t *lenp,

[...]

> @@ -3308,22 +3308,112 @@ u32 udp_flow_hashrnd(void)
>  }
>  EXPORT_SYMBOL(udp_flow_hashrnd);
>  
> -static int __net_init udp_sysctl_init(struct net *net)
> +static void __net_init udp_sysctl_init(struct net *net)
>  {
> -	net->ipv4.udp_table = &udp_table;
> -
>  	net->ipv4.sysctl_udp_rmem_min = PAGE_SIZE;
>  	net->ipv4.sysctl_udp_wmem_min = PAGE_SIZE;
>  
>  #ifdef CONFIG_NET_L3_MASTER_DEV
>  	net->ipv4.sysctl_udp_l3mdev_accept = 0;
>  #endif
> +}
> +
> +static struct udp_table __net_init *udp_pernet_table_alloc(unsigned int hash_entries)
> +{
> +	unsigned long hash_size, bitmap_size;
> +	struct udp_table *udptable;
> +	int i;
> +
> +	udptable = kmalloc(sizeof(*udptable), GFP_KERNEL);
> +	if (!udptable)
> +		goto out;
> +
> +	udptable->log = ilog2(hash_entries);
> +	udptable->mask = hash_entries - 1;
> +
> +	hash_size = L1_CACHE_ALIGN(hash_entries * 2 * sizeof(struct udp_hslot));
> +	bitmap_size = hash_entries *
> +		BITS_TO_LONGS(udp_bitmap_size(udptable)) * sizeof(unsigned long);

Ouch, I'm very sorry. I did not realize we need a bitmap per hash
bucket. This leads to a constant 8k additional memory overhead per
netns, undependently from arch long bitsize.

I guess it's still acceptable, but perhaps worth mentioning in the
commit message?

Again sorry for the back && forth, I'm reconsidering all the above
given my dumb misunderstanding.

I see that a minumum size of 256 hash buckets does not match your use
case, still... if lower values of the per netns hash size are inflated
to e.g. 128 and we keep the bitmap on stack (should be 64 bytes wide, I
guess still an acceptable value), the per netns memory overhead will be
128*2*<hash bucket size> = 8K, less that what we get the above schema
and any smaller hash - a single hash bucket leads to a 32 + 8K memory
overhead.

TL;DR: what about accepting any per netns hash table size, but always
allocate at least 128 buckets and keep the bitmap on the stack?

Thanks,

Paolo