[RFC] tcp: setsockopt congestion control autoload

[RFC] tcp: setsockopt congestion control autoload

[Stephen Hemminger]

If user asks for a congestion control type with setsockopt() then it
may be available as a module not included in the kernel already. 
It should be autoloaded if needed.  This is done already when
the default selection is change with sysctl, but not when application
requests via sysctl.

Only reservation is are there any bad security implications from this?

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

--- orig/net/ipv4/tcp_cong.c	2006-10-25 13:55:34.000000000 -0700
+++ new/net/ipv4/tcp_cong.c	2006-10-25 13:58:39.000000000 -0700
@@ -153,9 +153,19 @@
 
 	rcu_read_lock();
 	ca = tcp_ca_find(name);
+	/* no change asking for existing value */
 	if (ca == icsk->icsk_ca_ops)
 		goto out;
 
+#ifdef CONFIG_KMOD
+	/* not found attempt to autoload module */
+	if (!ca) {
+		rcu_read_unlock();
+		request_module("tcp_%s", name);
+		rcu_read_lock();
+		ca = tcp_ca_find(name);
+	}
+#endif
 	if (!ca)
 		err = -ENOENT;
 

Re: [RFC] tcp: setsockopt congestion control autoload

[Patrick McHardy]

Stephen Hemminger wrote:
> If user asks for a congestion control type with setsockopt() then it
> may be available as a module not included in the kernel already. 
> It should be autoloaded if needed.  This is done already when
> the default selection is change with sysctl, but not when application
> requests via sysctl.
> 
> Only reservation is are there any bad security implications from this?

There are already a quite large number of precedents for this, I think
this is one of the less questionable ones, the potential for (local)
damage is limited to minimal tcp_ca_find performance impact if I don't
miss anything (assuming no bugs in the modules that cause crashes
or something like that).

The in my opinion most questionable autoloading is in af_netlink BTW,
it will autoload any netlink provider with an appropriate module alias,
which could be just about anything (examples include conntrack, which
has performance and other side-effects, ULOG, which in turn loads
iptables, xfrm_user, connector, ...). Other autoloading is usually
limited to a clear scope of what might be affected.

Re: [RFC] tcp: setsockopt congestion control autoload

[Evgeniy Polyakov]

On Wed, Oct 25, 2006 at 11:08:43AM -0700, Stephen Hemminger (shemminger@osdl.org) wrote:
> If user asks for a congestion control type with setsockopt() then it
> may be available as a module not included in the kernel already. 
> It should be autoloaded if needed.  This is done already when
> the default selection is change with sysctl, but not when application
> requests via sysctl.
> 
> Only reservation is are there any bad security implications from this?

What if system is badly configured, so it is possible to load malicious
module by kernel?

-- 
	Evgeniy Polyakov

Re: [RFC] tcp: setsockopt congestion control autoload

[Stephen Hemminger]

Evgeniy Polyakov wrote:
> On Wed, Oct 25, 2006 at 11:08:43AM -0700, Stephen Hemminger (shemminger@osdl.org) wrote:
>   
>> If user asks for a congestion control type with setsockopt() then it
>> may be available as a module not included in the kernel already. 
>> It should be autoloaded if needed.  This is done already when
>> the default selection is change with sysctl, but not when application
>> requests via sysctl.
>>
>> Only reservation is are there any bad security implications from this?
>>     
>
> What if system is badly configured, so it is possible to load malicious
> module by kernel?
>
>   
The kernel module loader has a fixed path. So one would have to be able 
to create a module
in /lib/modules/<kernel release> in order to get the malicious code 
loaded.  If the intruder could
put a module there, it would be just as easy to patch an existing module 
and have the
hack available on reboot.

Re: [RFC] tcp: setsockopt congestion control autoload

[Evgeniy Polyakov]

On Thu, Oct 26, 2006 at 07:34:57AM -0700, Stephen Hemminger (shemminger@osdl.org) wrote:
> Evgeniy Polyakov wrote:
> >On Wed, Oct 25, 2006 at 11:08:43AM -0700, Stephen Hemminger 
> >(shemminger@osdl.org) wrote:
> >  
> >>If user asks for a congestion control type with setsockopt() then it
> >>may be available as a module not included in the kernel already. 
> >>It should be autoloaded if needed.  This is done already when
> >>the default selection is change with sysctl, but not when application
> >>requests via sysctl.
> >>
> >>Only reservation is are there any bad security implications from this?
> >>    
> >
> >What if system is badly configured, so it is possible to load malicious
> >module by kernel?
> >
> The kernel module loader has a fixed path. So one would have to be able 
> to create a module
> in /lib/modules/<kernel release> in order to get the malicious code 
> loaded.  If the intruder could
> put a module there, it would be just as easy to patch an existing module 
> and have the
> hack available on reboot.

It just calls /sbin/modprobe, which in turn runs tons of scripts in
/etc/hotplug, modprobe and other places...
In the paranoid case we should not allow any user to load kernel
modules, even known ones. Should this option be guarded by some
capability check?

-- 
	Evgeniy Polyakov

Re: [RFC] tcp: setsockopt congestion control autoload

[Stephen Hemminger]

On Thu, 26 Oct 2006 18:57:13 +0400
Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:

> On Thu, Oct 26, 2006 at 07:34:57AM -0700, Stephen Hemminger (shemminger@osdl.org) wrote:
> > Evgeniy Polyakov wrote:
> > >On Wed, Oct 25, 2006 at 11:08:43AM -0700, Stephen Hemminger 
> > >(shemminger@osdl.org) wrote:
> > >  
> > >>If user asks for a congestion control type with setsockopt() then it
> > >>may be available as a module not included in the kernel already. 
> > >>It should be autoloaded if needed.  This is done already when
> > >>the default selection is change with sysctl, but not when application
> > >>requests via sysctl.
> > >>
> > >>Only reservation is are there any bad security implications from this?
> > >>    
> > >
> > >What if system is badly configured, so it is possible to load malicious
> > >module by kernel?
> > >
> > The kernel module loader has a fixed path. So one would have to be able 
> > to create a module
> > in /lib/modules/<kernel release> in order to get the malicious code 
> > loaded.  If the intruder could
> > put a module there, it would be just as easy to patch an existing module 
> > and have the
> > hack available on reboot.
> 
> It just calls /sbin/modprobe, which in turn runs tons of scripts in
> /etc/hotplug, modprobe and other places...
> In the paranoid case we should not allow any user to load kernel
> modules, even known ones. Should this option be guarded by some
> capability check?
> 

No capability check needed. Any additional paranoia belongs in /sbin/modprobe.

There seems to be lots of existing usage where a user can cause a module
to be loaded (see bin_fmt, xtables, etc).

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [RFC] tcp: setsockopt congestion control autoload

[Patrick McHardy]

Stephen Hemminger wrote:
> No capability check needed. Any additional paranoia belongs in /sbin/modprobe.
> 
> There seems to be lots of existing usage where a user can cause a module
> to be loaded (see bin_fmt, xtables, etc).


x_tables is restricted to CAP_NET_ADMIN, but in net/ alone we have
__sock_create (loads protocol families), sock_ioctl (loads bridge,
vlan or dlci), the already mentioned netlink case, inet_create
(loads IP protocols), inet6_create (similar to inet_create), and
a few more.

Re: [RFC] tcp: setsockopt congestion control autoload

[John Heffner]

My reservation in doing this would be that as an administrator, I may 
want to choose exactly what congestion control is available any any 
given time.  The different congestion control algorithms are not 
necessarily fair to each other.

If the modules are autoloaded, I could still enforce this by moving the 
modules out of /lib/modules, but I think it's cleaner to do it by 
loading/unloading modules as appropriate.

   -John


Stephen Hemminger wrote:
> If user asks for a congestion control type with setsockopt() then it
> may be available as a module not included in the kernel already. 
> It should be autoloaded if needed.  This is done already when
> the default selection is change with sysctl, but not when application
> requests via sysctl.
> 
> Only reservation is are there any bad security implications from this?
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> 
> --- orig/net/ipv4/tcp_cong.c	2006-10-25 13:55:34.000000000 -0700
> +++ new/net/ipv4/tcp_cong.c	2006-10-25 13:58:39.000000000 -0700
> @@ -153,9 +153,19 @@
>  
>  	rcu_read_lock();
>  	ca = tcp_ca_find(name);
> +	/* no change asking for existing value */
>  	if (ca == icsk->icsk_ca_ops)
>  		goto out;
>  
> +#ifdef CONFIG_KMOD
> +	/* not found attempt to autoload module */
> +	if (!ca) {
> +		rcu_read_unlock();
> +		request_module("tcp_%s", name);
> +		rcu_read_lock();
> +		ca = tcp_ca_find(name);
> +	}
> +#endif
>  	if (!ca)
>  		err = -ENOENT;
>  
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] tcp: setsockopt congestion control autoload

[David Miller]

From: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
Date: Thu, 26 Oct 2006 18:57:13 +0400

> It just calls /sbin/modprobe, which in turn runs tons of scripts in
> /etc/hotplug, modprobe and other places...
> In the paranoid case we should not allow any user to load kernel
> modules, even known ones. Should this option be guarded by some
> capability check?

Do you realize that sys_socket() already makes this kind of
thing happen already?

Re: [RFC] tcp: setsockopt congestion control autoload

[David Miller]

From: John Heffner <jheffner@psc.edu>
Date: Thu, 26 Oct 2006 13:29:26 -0400

> My reservation in doing this would be that as an administrator, I may 
> want to choose exactly what congestion control is available any any 
> given time.  The different congestion control algorithms are not 
> necessarily fair to each other.
> 
> If the modules are autoloaded, I could still enforce this by moving the 
> modules out of /lib/modules, but I think it's cleaner to do it by 
> loading/unloading modules as appropriate.

Fair enough, and for the folks doing tests of congestion control
algorithms they can run as root or whatever.

Re: [RFC] tcp: setsockopt congestion control autoload

[Hagen Paul Pfeifer]

* John Heffner | 2006-10-26 13:29:26 [-0400]:

>My reservation in doing this would be that as an administrator, I may 
>want to choose exactly what congestion control is available any any 
>given time.  The different congestion control algorithms are not 
>necessarily fair to each other.

ACK, completely right. A user without CAP_NET_ADMIN MUST NOT changed the
algorithm.  We know that there are some unfairness out there. And maybe some
time ago someone introduce a satellite-algorithm which is per definition
completely unfair to vanilla tcp.
We should guard this with a CAP_NET_ADMIN capability so that built-in modules
also shouldn't be enabled.

HGN

--
Signed and/or encrypted mails preferd. Key-Id = 0x98350C22
Fingerprint = 490F 557B 6C48 6D7E 5706  2EA2 4A22 8D45 9835 0C22 

Re: [RFC] tcp: setsockopt congestion control autoload

[John Heffner]

Hagen Paul Pfeifer wrote:
> * John Heffner | 2006-10-26 13:29:26 [-0400]:
> 
>> My reservation in doing this would be that as an administrator, I may 
>> want to choose exactly what congestion control is available any any 
>> given time.  The different congestion control algorithms are not 
>> necessarily fair to each other.
> 
> ACK, completely right. A user without CAP_NET_ADMIN MUST NOT changed the
> algorithm.  We know that there are some unfairness out there. And maybe some
> time ago someone introduce a satellite-algorithm which is per definition
> completely unfair to vanilla tcp.
> We should guard this with a CAP_NET_ADMIN capability so that built-in modules
> also shouldn't be enabled.

I don't know if I'd want to go that far.  For example, there's a nice 
protocol TCP-LP which is by design unfair in the other direction -- it 
yields to other traffic so that you can basically run a scavenger service.

If you really care about this, you could try to rank protocols based on 
aggressiveness (note this is not trivial) and do something like 'nice' 
where mortals can only nice up not down.  Practically speaking, I'm not 
sure this is necessary (worth the effort).

   -John

[PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Hagen Paul Pfeifer]

Check if user has CAP_NET_ADMIN capability to change congestion control
algorithm.

Under normal circumstances a application programmer doesn't have enough
information to choose the "right" algorithm (expect he is the pchar/pathchar
maintainer). At 99.9% only the local host administrator has the knowledge to
select a proper standard, system-wide algorithm (the remaining 0.1% are
for testing purpose). If we let the user select an alternative algorithm we
introduce one potential weak spot - so we ban this eventuality.

HGN


Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index af0aca1..c1ae2e9 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -10,6 +10,7 @@ #include <linux/module.h>
 #include <linux/mm.h>
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/capability.h>
 #include <net/tcp.h>

 static DEFINE_SPINLOCK(tcp_cong_list_lock);
@@ -151,6 +152,9 @@ int tcp_set_congestion_control(struct so
        struct tcp_congestion_ops *ca;
        int err = 0;

+       if (!capable(CAP_NET_ADMIN))
+               return -EPERM;
+
        rcu_read_lock();
        ca = tcp_ca_find(name);
        if (ca == icsk->icsk_ca_ops)

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Ian McDonald]

On 10/27/06, Hagen Paul Pfeifer <hagen@jauu.net> wrote:
>
> Check if user has CAP_NET_ADMIN capability to change congestion control
> algorithm.
>
> Under normal circumstances a application programmer doesn't have enough
> information to choose the "right" algorithm (expect he is the pchar/pathchar
> maintainer). At 99.9% only the local host administrator has the knowledge to
> select a proper standard, system-wide algorithm (the remaining 0.1% are
> for testing purpose). If we let the user select an alternative algorithm we
> introduce one potential weak spot - so we ban this eventuality.
>
I don't agree with this at all. I would love Firefox, BitTorrent etc
to implement usage of TCP-LP for example so they use "unused"
bandwidth only.

With this change applications can't do this.

If we are going to restrict by capabilities then I think we should
only restrict module loading - this way the admin of the box can
decide what algorithms can be used.

Ian
-- 
Ian McDonald
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group
Department of Computer Science
University of Waikato
New Zealand

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[David Miller]

This is driving me crazy...

Your email client turned the tabs into spaces in the patch making it
useless.

I want to ask why it is so hard for people to submit patches that are
not corrupted?  :-/

I type in this kind of email response at least 2 or 3 times every
single day that I review patches.  Spending the time to review a patch
only to find out that it is corrupted and doesn't apply consumes a
significant chunk of my time.

Again, send the patch in an email to yourself and try to apply the
patch from that email if you are in doubt.

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[David Miller]

From: "Ian McDonald" <ian.mcdonald@jandi.co.nz>
Date: Fri, 27 Oct 2006 12:59:30 +1300

> I don't agree with this at all. I would love Firefox, BitTorrent etc
> to implement usage of TCP-LP for example so they use "unused"
> bandwidth only.
> 
> With this change applications can't do this.
> 
> If we are going to restrict by capabilities then I think we should
> only restrict module loading - this way the admin of the box can
> decide what algorithms can be used.

You are using an example of a (supposedly) safe case of this
as a justification for allowing all cases.

It is bad, very bad, to allow arbitrary users to select arbitrary
congestion control algorithms.  It is just as bad as allowing them to
disable congestion control completely if that were an option.

If someone, for example, builds all the algorithms statically into
their kernel, for testing as root, this lets all users on the machine
do the same which is not right.

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Ian McDonald]

On 10/27/06, David Miller <davem@davemloft.net> wrote:
> From: "Ian McDonald" <ian.mcdonald@jandi.co.nz>
> Date: Fri, 27 Oct 2006 12:59:30 +1300
>
> > I don't agree with this at all. I would love Firefox, BitTorrent etc
> > to implement usage of TCP-LP for example so they use "unused"
> > bandwidth only.
> >
> > With this change applications can't do this.
> >
> > If we are going to restrict by capabilities then I think we should
> > only restrict module loading - this way the admin of the box can
> > decide what algorithms can be used.
>
> You are using an example of a (supposedly) safe case of this
> as a justification for allowing all cases.
>
> It is bad, very bad, to allow arbitrary users to select arbitrary
> congestion control algorithms.  It is just as bad as allowing them to
> disable congestion control completely if that were an option.

OK understand your point here but I think low priority TCP has its
use. Don't agree it is just as bad, but it is bad under the wrong
circumstances - it's still better than UDP which has no congestion
control...

Don't want to make it over complicated though.

I think the most sense would be to restrict it as shown as tcp-lp is
the exception and allow tcp-lp via another mechanism. That is a
situation where the user could specify how low priority they want the
traffic to be... If I ever get enough time I'll have a go at it but
can't see it this year :-(

It actually makes more sense to tie the congestion control algorithm
to the route/destination IP if we are going to change it but that is a
whole another exercise in itself.
>
> If someone, for example, builds all the algorithms statically into
> their kernel, for testing as root, this lets all users on the machine
> do the same which is not right.
>
This is the state at present as I understand it. However that doesn't
make it right.
-- 
Ian McDonald
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group
Department of Computer Science
University of Waikato
New Zealand

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Stephen Hemminger]

On Fri, 27 Oct 2006 01:52:56 +0200
Hagen Paul Pfeifer <hagen@jauu.net> wrote:

> 
> Check if user has CAP_NET_ADMIN capability to change congestion control
> algorithm.
> 
> Under normal circumstances a application programmer doesn't have enough
> information to choose the "right" algorithm (expect he is the pchar/pathchar
> maintainer). At 99.9% only the local host administrator has the knowledge to
> select a proper standard, system-wide algorithm (the remaining 0.1% are
> for testing purpose). If we let the user select an alternative algorithm we
> introduce one potential weak spot - so we ban this eventuality.
> 
> HGN

If you aren't doing experiments don't compile it in your kernel.
If distro's are including "unfair" congestion control file a bug report.

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Hagen Paul Pfeifer]

* David Miller | 2006-10-26 17:02:21 [-0700]:

>Your email client turned the tabs into spaces in the patch making it
>useless.

Sorry my mistake! I am en route and I paste the patch into my editor, who eat
all tabs. One more time: sorry!


Check if user has CAP_NET_ADMIN capability to change
congestion control algorithm.


Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>

---
 net/ipv4/tcp_cong.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index af0aca1..c1ae2e9 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -10,6 +10,7 @@ #include <linux/module.h>
 #include <linux/mm.h>
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/capability.h>
 #include <net/tcp.h>
 
 static DEFINE_SPINLOCK(tcp_cong_list_lock);
@@ -151,6 +152,9 @@ int tcp_set_congestion_control(struct so
 	struct tcp_congestion_ops *ca;
 	int err = 0;
 
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
 	rcu_read_lock();
 	ca = tcp_ca_find(name);
 	if (ca == icsk->icsk_ca_ops)
-- 
1.4.1.1

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Stephen Hemminger]

On Fri, 27 Oct 2006 12:43:11 +0200
Hagen Paul Pfeifer <hagen@jauu.net> wrote:

> * David Miller | 2006-10-26 17:02:21 [-0700]:
> 
> >Your email client turned the tabs into spaces in the patch making it
> >useless.
> 
> Sorry my mistake! I am en route and I paste the patch into my editor, who eat
> all tabs. One more time: sorry!
> 
> 
> Check if user has CAP_NET_ADMIN capability to change
> congestion control algorithm.
> 
> 
> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>

Please no, it makes the socket option useless.
If you want to tag some "bad apples" thats okay, but would need
some more infrastructure.

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Hagen Paul Pfeifer]

* Stephen Hemminger | 2006-10-27 07:41:02 [-0700]:

>Please no, it makes the socket option useless.

Technical no, in the sense of usability for everybody yes. You are right
Stephen, as a programmer I understand you complete!

But on the other side: We know for sure that this IS a problem if we allow
everybody to "prefer his socket".

In my opinion we should prefer fairness before usability! As John Heffner
introduce, we can introduce a ranking system for congestion control algorithms -
but this solution seems a little bit oversized and maybe can't be complete
guaranteed (complex interaction between the protocols in different
environment and so on, you know).

HGN




-- 
 /°\   --- JOIN NOW!!! --- 
 \ /  ASCII ribbon campaign
  X       against HTML     
 / \    in mail and news   

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[Stephen Hemminger]

Hagen Paul Pfeifer wrote:
> * Stephen Hemminger | 2006-10-27 07:41:02 [-0700]:
>
>   
>> Please no, it makes the socket option useless.
>>     
>
> Technical no, in the sense of usability for everybody yes. You are right
> Stephen, as a programmer I understand you complete!
>
> But on the other side: We know for sure that this IS a problem if we allow
> everybody to "prefer his socket".
>
> In my opinion we should prefer fairness before usability! As John Heffner
> introduce, we can introduce a ranking system for congestion control algorithms -
> but this solution seems a little bit oversized and maybe can't be complete
> guaranteed (complex interaction between the protocols in different
> environment and so on, you know).
>
> HGN
>
>   
If there is a dangerous choice, then it should be removed. Otherwise I can't
see the problem. It is a bigger risk to have to escalate the privileges 
of an application
just to allow it to use something.

[PATCH] tcp: don't allow unfair congestion control to be built without warning

[Stephen Hemminger]

My proposed method restricting TCP choices to fair algorithms.
This a net wide, not system wide issue, it should not be done
by kernel policy choice (capability), but by a build choice.

--- sky2.orig/net/ipv4/Kconfig	2006-10-27 10:10:47.000000000 -0700
+++ sky2/net/ipv4/Kconfig	2006-10-27 10:15:56.000000000 -0700
@@ -470,6 +470,16 @@
 
 if TCP_CONG_ADVANCED
 
+config TCP_CONG_UNFAIR
+        bool "Allow unfair congestion control algorithms"
+	depends on EXPERIMENTAL
+        ---help---
+	  Some of the congestion control algorithms are for testing
+	  and research purposes and should not deployed on public
+	  networks because of the possiblity of unfair behavior.
+	  These algorithms may be useful for future development
+	  or comparison purposes.
+
 config TCP_CONG_BIC
 	tristate "Binary Increase Congestion (BIC) control"
 	default m
@@ -551,7 +561,7 @@
 
 config TCP_CONG_SCALABLE
 	tristate "Scalable TCP"
-	depends on EXPERIMENTAL
+	depends on TCP_CONG_UNFAIR
 	default n
 	---help---
 	Scalable TCP is a sender-side only change to TCP which uses a

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[John Heffner]

I think "unfair" is a difficult word.  Unfair to what?  It's true that 
Scalable TCP is unfair to itself in that flows with unequal shares do 
not converge, but it's not clear what its interactions are with other 
congestion control algorithms.  It's not clear to me that it's 
significantly more unfair wrt. reno than BIC, etc.  "Known to be broken" 
might be more correct language. :)

One thought would be to use a module parameter that sets one bit of 
state: allow unprivileged use.  Each module could have a sensible 
default value.

   -John


Stephen Hemminger wrote:
> My proposed method restricting TCP choices to fair algorithms.
> This a net wide, not system wide issue, it should not be done
> by kernel policy choice (capability), but by a build choice.
> 
> --- sky2.orig/net/ipv4/Kconfig	2006-10-27 10:10:47.000000000 -0700
> +++ sky2/net/ipv4/Kconfig	2006-10-27 10:15:56.000000000 -0700
> @@ -470,6 +470,16 @@
>  
>  if TCP_CONG_ADVANCED
>  
> +config TCP_CONG_UNFAIR
> +        bool "Allow unfair congestion control algorithms"
> +	depends on EXPERIMENTAL
> +        ---help---
> +	  Some of the congestion control algorithms are for testing
> +	  and research purposes and should not deployed on public
> +	  networks because of the possiblity of unfair behavior.
> +	  These algorithms may be useful for future development
> +	  or comparison purposes.
> +
>  config TCP_CONG_BIC
>  	tristate "Binary Increase Congestion (BIC) control"
>  	default m
> @@ -551,7 +561,7 @@
>  
>  config TCP_CONG_SCALABLE
>  	tristate "Scalable TCP"
> -	depends on EXPERIMENTAL
> +	depends on TCP_CONG_UNFAIR
>  	default n
>  	---help---
>  	Scalable TCP is a sender-side only change to TCP which uses a

[PATCH] tcp: allow restricting congestion control choices

[Stephen Hemminger]

Here is an alternative that allows runtime based restriction on some
TCP congestion control choices. 

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

---
 include/net/tcp.h   |    1 +
 net/ipv4/tcp_cong.c |    4 ++++
 2 files changed, 5 insertions(+)

--- sky2.orig/include/net/tcp.h	2006-10-27 10:46:19.000000000 -0700
+++ sky2/include/net/tcp.h	2006-10-27 10:46:55.000000000 -0700
@@ -651,6 +651,7 @@
 
 	char 		name[TCP_CA_NAME_MAX];
 	struct module 	*owner;
+	int		restricted;	/* NET_ADMIN only */
 };
 
 extern int tcp_register_congestion_control(struct tcp_congestion_ops *type);
--- sky2.orig/net/ipv4/tcp_cong.c	2006-10-27 10:51:47.000000000 -0700
+++ sky2/net/ipv4/tcp_cong.c	2006-10-27 10:56:36.000000000 -0700
@@ -10,6 +10,7 @@
 #include <linux/mm.h>
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/capability.h>
 #include <net/tcp.h>
 
 static DEFINE_SPINLOCK(tcp_cong_list_lock);
@@ -159,6 +160,9 @@
 	if (!ca)
 		err = -ENOENT;
 
+	else if (ca->restricted && !capable(CAP_NET_ADMIN))
+		err = -EPERM;
+
 	else if (!try_module_get(ca->owner))
 		err = -EBUSY;
 

[PATCH] tcp: setsockopt congestion control autoload

[Stephen Hemminger]

If application asks for a congestion control type with setsockopt() 
then it may be available as a module not included in the kernel already. 
If it has permission to load modules then the tcp congestion
module should be autoloaded if needed.  This is done already when
the default selection is change with sysctl, but not when application
requests via sysctl.
 
Add a similar additional check to the sysctl path as well.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
 
---
 net/ipv4/tcp_cong.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

--- a/net/ipv4/tcp_cong.c	2006-10-27 10:56:36.000000000 -0700
+++ b/net/ipv4/tcp_cong.c	2006-10-27 11:09:36.000000000 -0700
@@ -114,7 +114,7 @@
 	spin_lock(&tcp_cong_list_lock);
 	ca = tcp_ca_find(name);
 #ifdef CONFIG_KMOD
-	if (!ca) {
+	if (!ca && capable(CAP_SYS_MODULE)) {
 		spin_unlock(&tcp_cong_list_lock);
 
 		request_module("tcp_%s", name);
@@ -154,9 +154,19 @@
 
 	rcu_read_lock();
 	ca = tcp_ca_find(name);
+	/* no change asking for existing value */
 	if (ca == icsk->icsk_ca_ops)
 		goto out;
 
+#ifdef CONFIG_KMOD
+	/* not found attempt to autoload module */
+	if (!ca && capable(CAP_SYS_MODULE)) {
+		rcu_read_unlock();
+		request_module("tcp_%s", name);
+		rcu_read_lock();
+		ca = tcp_ca_find(name);
+	}
+#endif
 	if (!ca)
 		err = -ENOENT;
 

Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Fri, 27 Oct 2006 10:30:16 -0700

> My proposed method restricting TCP choices to fair algorithms.
> This a net wide, not system wide issue, it should not be done
> by kernel policy choice (capability), but by a build choice.

I think this sucks even worse than the current situation.

How difficult is it to understand that an administrator might
like to be able to build in and experiment with some congestion
control algorithms, yet still be able to keep his normal users
from using them?

Re: [PATCH] Check if user has CAP_NET_ADMIN to change congestion control algorithm

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Fri, 27 Oct 2006 07:41:02 -0700

> Please no, it makes the socket option useless.
> If you want to tag some "bad apples" thats okay, but would need
> some more infrastructure.

The behavior of the TCP stack is a system wide decision.

If anything it should be "everything besides the default
and Reno are offlimits to unprivileged users" with an
administrative method to override that.

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[Stephen Hemminger]

On Fri, 27 Oct 2006 14:17:49 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Fri, 27 Oct 2006 10:30:16 -0700
> 
> > My proposed method restricting TCP choices to fair algorithms.
> > This a net wide, not system wide issue, it should not be done
> > by kernel policy choice (capability), but by a build choice.
> 
> I think this sucks even worse than the current situation.
> 
> How difficult is it to understand that an administrator might
> like to be able to build in and experiment with some congestion
> control algorithms, yet still be able to keep his normal users
> from using them?

Only some (very few) have any bad consequences. So the typical
distribution should be able to switch with most available for everyone,
and only a few needing special privileges.


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Fri, 27 Oct 2006 14:24:02 -0700

> Only some (very few) have any bad consequences. So the typical
> distribution should be able to switch with most available for everyone,
> and only a few needing special privileges.

I would strongly disagree as we've had several OOPS'er class bugs in
the less frequently used algorithms.

I stand by my position that an administrator's wish to do this is
quite valid.

It's bad enough that people are all over us for the default algorithm
we have choosen, so it'd be extremely irresponsible and even worse if
we allowed users to select any of the other "research" algorithms for
their TCP connections by default just because those modules happened
to be configured into the kernel.

This userspace convenience argument holds zero water.

Provide a way for the administrator to control the situation fully,
and choose a sane default which errs on the side of caution for the
sake of internet stability.

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[Stephen Hemminger]

On Fri, 27 Oct 2006 14:37:01 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Fri, 27 Oct 2006 14:24:02 -0700
> 
> > Only some (very few) have any bad consequences. So the typical
> > distribution should be able to switch with most available for everyone,
> > and only a few needing special privileges.
> 
> I would strongly disagree as we've had several OOPS'er class bugs in
> the less frequently used algorithms.
> 

Then tag those as restricted.  Why should we keep app's away from
the simple ones.

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Fri, 27 Oct 2006 14:59:13 -0700

> On Fri, 27 Oct 2006 14:37:01 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Stephen Hemminger <shemminger@osdl.org>
> > Date: Fri, 27 Oct 2006 14:24:02 -0700
> > 
> > > Only some (very few) have any bad consequences. So the typical
> > > distribution should be able to switch with most available for everyone,
> > > and only a few needing special privileges.
> > 
> > I would strongly disagree as we've had several OOPS'er class bugs in
> > the less frequently used algorithms.
> > 
> 
> Then tag those as restricted.  Why should we keep app's away from
> the simple ones.

You can't predict bugs, but what you can do is know that the lesser
used algorithms are by definition less tested and therefore more
likely to have bugs.  Everything except the default and Reno are
lesser used.

Safe by default, there is no other choice.  You fail to respond to
THAT part of my email.  That's the important point.  Let me
reiterate:

> It's bad enough that people are all over us for the default algorithm
> we have choosen, so it'd be extremely irresponsible and even worse if
> we allowed users to select any of the other "research" algorithms for
> their TCP connections by default just because those modules happened
> to be configured into the kernel.
>
> This userspace convenience argument holds zero water.
>
> Provide a way for the administrator to control the situation fully,
> and choose a sane default which errs on the side of caution for the
> sake of internet stability.

Please reread this and consider why it's important.

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[Stephen Hemminger]

On Fri, 27 Oct 2006 15:12:38 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Fri, 27 Oct 2006 14:59:13 -0700
> 
> > On Fri, 27 Oct 2006 14:37:01 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> > > From: Stephen Hemminger <shemminger@osdl.org>
> > > Date: Fri, 27 Oct 2006 14:24:02 -0700
> > > 
> > > > Only some (very few) have any bad consequences. So the typical
> > > > distribution should be able to switch with most available for everyone,
> > > > and only a few needing special privileges.
> > > 
> > > I would strongly disagree as we've had several OOPS'er class bugs in
> > > the less frequently used algorithms.
> > > 
> > 
> > Then tag those as restricted.  Why should we keep app's away from
> > the simple ones.
> 
> You can't predict bugs, but what you can do is know that the lesser
> used algorithms are by definition less tested and therefore more
> likely to have bugs.  Everything except the default and Reno are
> lesser used.

If they aren't usable they should be marked BROKEN or something
like that. The stability argument doesn't really work, we don't
like to let root kill the system either.
 
> Safe by default, there is no other choice.  You fail to respond to
> THAT part of my email.  That's the important point.  Let me
> reiterate:
> 
> > It's bad enough that people are all over us for the default algorithm
> > we have choosen, so it'd be extremely irresponsible and even worse if
> > we allowed users to select any of the other "research" algorithms for
> > their TCP connections by default just because those modules happened
> > to be configured into the kernel.

Make it hard for them to configure then.  I don't want your
distro to ship with the risky ones turned on.  But we should allow
use of reno, bic, cubic, lp, htcp, and westwood (maybe) by regular
users if admin allows.

> > This userspace convenience argument holds zero water.
> >
> > Provide a way for the administrator to control the situation fully,
> > and choose a sane default which errs on the side of caution for the
> > sake of internet stability.
> 
> Please reread this and consider why it's important.

The current situation is fine. You have to ask for them in the configuration,
and root has to either load the module or set it as default.

The restricted flag patch which you have ignored, would be a way to
allow them to be configured but tag the "bad apples" for only
root usage.

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Fri, 27 Oct 2006 15:21:49 -0700

> The restricted flag patch which you have ignored, would be a way to
> allow them to be configured but tag the "bad apples" for only
> root usage.

I haven't ignored it, it's in my backlog below more important
things like Appletalk OOPS'ers.

Re: [PATCH] tcp: don't allow unfair congestion control to be built without warning

[Stephen Hemminger]

How about another way of controlling this via sysctl.

First, add code to for read only:
	/proc/sys/net/ipv4/tcp_available_congestion_control  (or shorter name)
this will show all things compiled in (even if not loaded yet). Similar
to /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies

Second, add flag (allowed) to the tcp_congestion structure [inverse of
earlier restricted]

Third, add read-write
	/proc/sys/net/ipv4/tcp_allowed_congestion_control
to show and set/clear the allowed flag. Default value would be
"reno xxx" where xxx is what ever the default value from the kernel
config is (currently cubic).

I would use sysfs for this, but it make sense not to spread TCP stuff into
both sysctl and sysfs.

[RFC] tcp: available congetsion control

[Stephen Hemminger]

Nice way to see what congestion control modules are loaded.
It does impose a soft limit of 32 possibilities.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

---
 include/linux/sysctl.h     |    1 +
 include/net/tcp.h          |    3 +++
 net/ipv4/sysctl_net_ipv4.c |   25 ++++++++++++++++++++++++-
 net/ipv4/tcp_cong.c        |   14 ++++++++++++++
 4 files changed, 42 insertions(+), 1 deletion(-)

--- skge.orig/include/linux/sysctl.h
+++ skge/include/linux/sysctl.h
@@ -418,6 +418,7 @@ enum
 	NET_CIPSOV4_CACHE_BUCKET_SIZE=119,
 	NET_CIPSOV4_RBM_OPTFMT=120,
 	NET_CIPSOV4_RBM_STRICTVALID=121,
+	NET_TCP_AVAIL_CONG_CONTROL=122,
 };
 
 enum {
--- skge.orig/include/net/tcp.h
+++ skge/include/net/tcp.h
@@ -621,6 +621,8 @@ enum tcp_ca_event {
  * Interface for adding new TCP congestion control handlers
  */
 #define TCP_CA_NAME_MAX	16
+#define TCP_CA_MAX	32
+
 struct tcp_congestion_ops {
 	struct list_head	list;
 
@@ -659,6 +661,7 @@ extern void tcp_unregister_congestion_co
 extern void tcp_init_congestion_control(struct sock *sk);
 extern void tcp_cleanup_congestion_control(struct sock *sk);
 extern int tcp_set_default_congestion_control(const char *name);
+extern void tcp_get_available_congestion_control(char *name, int maxlen);
 extern void tcp_get_default_congestion_control(char *name);
 extern int tcp_set_congestion_control(struct sock *sk, const char *name);
 extern void tcp_slow_start(struct tcp_sock *tp);
--- skge.orig/net/ipv4/sysctl_net_ipv4.c
+++ skge/net/ipv4/sysctl_net_ipv4.c
@@ -108,6 +108,22 @@ static int proc_tcp_congestion_control(c
 	return ret;
 }
 
+static int proc_tcp_available_congestion_control(ctl_table *ctl,
+						 int write, struct file * filp,
+						 void __user *buffer, size_t *lenp,
+						 loff_t *ppos)
+{
+	char val[TCP_CA_MAX*(TCP_CA_NAME_MAX+1)];
+	ctl_table tbl = {
+		.data = val,
+		.maxlen = TCP_CA_MAX*(TCP_CA_NAME_MAX+1),
+	};
+
+	tcp_get_available_congestion_control(val, tbl.maxlen);
+
+	return proc_dostring(&tbl, write, filp, buffer, lenp, ppos);
+}
+
 static int sysctl_tcp_congestion_control(ctl_table *table, int __user *name,
 					 int nlen, void __user *oldval,
 					 size_t __user *oldlenp,
@@ -133,9 +149,9 @@ static int __init tcp_congestion_default
 {
 	return tcp_set_default_congestion_control(CONFIG_DEFAULT_TCP_CONG);
 }
-
 late_initcall(tcp_congestion_default);
 
+
 ctl_table ipv4_table[] = {
         {
 		.ctl_name	= NET_IPV4_TCP_TIMESTAMPS,
@@ -738,6 +754,13 @@ ctl_table ipv4_table[] = {
 		.proc_handler	= &proc_dointvec,
 	},
 #endif /* CONFIG_NETLABEL */
+	{
+		.ctl_name	= NET_TCP_AVAIL_CONG_CONTROL,
+		.procname	= "tcp_available_congestion_control",
+		.mode		= 0444,
+		.maxlen		= TCP_CA_MAX*(TCP_CA_NAME_MAX+1),
+		.proc_handler   = &proc_tcp_available_congestion_control,
+	},
 	{ .ctl_name = 0 }
 };
 
--- skge.orig/net/ipv4/tcp_cong.c
+++ skge/net/ipv4/tcp_cong.c
@@ -144,6 +144,20 @@ void tcp_get_default_congestion_control(
 	rcu_read_unlock();
 }
 
+/* Build string with list of available congestion control values */
+void tcp_get_available_congestion_control(char *name, int maxlen)
+{
+	struct tcp_congestion_ops *ca;
+	int offs = 0;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(ca, &tcp_cong_list, list) {
+		offs += snprintf(name + offs, maxlen - offs, "%s%s",
+				 offs == 0 ? "" : " ", ca->name);
+	}
+	rcu_read_unlock();
+}
+
 /* Change congestion control for socket */
 int tcp_set_congestion_control(struct sock *sk, const char *name)
 {