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:	Wed, 16 Mar 2016 07:06:06 -0400
From:	Sowmini Varadhan <sowmini.varadhan@...cle.com>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	santosh.shilimkar@...cle.com, eric.dumazet@...il.com
Subject: Re: [PATCH net-next v3 1/2] RDS: TCP: Add sysctl tunables for
 sndbuf/rcvbuf on rds-tcp socket

On (03/16/16 11:29), Hannes Frederic Sowa wrote:
> 
> Normally we kmemdup a table per netns and update its data pointer,
> so we can reuse the proc_doint_minmax functions.

I actually thought of doing a separate kzalloc per netns, 
but it seemed like it would be a lot of memory bloat, when really,
all I want is just the per-netns data.

I could go and duplicate the whole table, but is there a significant
benefit to that, or some bad bug with this approach? 

One dubious (yes, I know, "solution looking for problem" category)
benefit is that this hack allows me to have both global, and per netns,
tunables... plus the reduced memory footprint (latter is probably
more interesting than former?)

> >+	rtn->rds_tcp_sysctl = register_net_sysctl(net, "net/rds/tcp",
> >+						  rds_tcp_sysctl_table);
> 
> You should add proper error handling here?

Agree, I will send this out in the next rev.

> >+	rtn->sndbuf_size = max_t(u32, user_atoi(buffer, *lenp) * 2,
> >+				 SOCK_MIN_SNDBUF);
> 
> user_atoi does return negative error values (as you implemented it),
> I think you should check before, otherwise the signed to unsigned
> conversion can cause huge memory allocations.

yes, good catch, that will have to be split into two lines. Shall
be fixing this in v4.

> Why actually implement user_atoi?

errhm. cut/paste from other drivers that do this. I needed to
do the copy_from_user() + kstrtoul, and user_atoi had some 
additional checks that seemed useful. 

--Sowmini


Powered by blists - more mailing lists