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]

Message-ID: <20221114202119.32011-1-kuniyu@amazon.com>
Date:   Mon, 14 Nov 2022 12:21:19 -0800
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <pabeni@...hat.com>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 6/6] udp: Introduce optional per-netns hash table.

From:   Paolo Abeni <pabeni@...hat.com>
Date:   Fri, 11 Nov 2022 09:53:31 +0100
> 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 

Will add memset()


> 
> > 
> > +
> > +	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.

Ugh, it will be 64K per netns ... ?

hash_entries              : 2 ^ n
BITS_TO_LONGS             : 2 ^ -m # arch specific
udp_bitmap_size(udptable) : 2 ^ (16 - n)
sizeof(unsigned long)     : 2 ^ m  # arch specific

(2 ^ n) * (2 ^ -m) * (2 ^ (16 - n)) * (2 ^ m)
= 2 ^ (n - m + 16 - n + m)
= 2 ^ 16
= 64 K


> 
> 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?

Sure, I'll change the min to 128.

128 * 2 * 16 = 4096 = 4K

---
$ pahole -C udp_hslot vmlinux
struct udp_hslot {
	struct hlist_head          head;                 /*     0     8 */
	int                        count;                /*     8     4 */
	spinlock_t                 lock;                 /*    12     4 */

	/* size: 16, cachelines: 1, members: 3 */
	/* last cacheline: 16 bytes */
} __attribute__((__aligned__(16)));
---


Thank you!


> 
> Thanks,
> 
> Paolo