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: <84f835ca7aacb831561cb9fb49ad4e014bf2b1fd.camel@redhat.com>
Date:   Mon, 14 Nov 2022 21:55:11 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        kuni1840@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 6/6] udp: Introduce optional per-netns hash
 table.

On Mon, 2022-11-14 at 12:21 -0800, Kuniyuki Iwashima wrote:
> 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

For the records, I still think it's 8k ;)

BITS_TO_LONGS(n) * sizeof(unsigned long) is always equal to n/8
regardless of the arch, while the above math gives BITS_TO_LONGS(n) *
sizeof(unsigned long) == n.

Cheers,

Paolo