problem setting net.ipvX.conf.all.forwarding via sysctl() system call

problem setting net.ipvX.conf.all.forwarding via sysctl() system call

[Jan Oravec]

Hello,


When net.ipvX.conf.all.forwarding is enabled via sysctl() system call,
forwarding is not enabled on all interfaces as it is when it is changed
using /proc filesystem.

For IPv6, it is obviously because sysctl 'strategy' handler is not defined.
For IPv4, it is because ipv4_sysctl_forward_strategy only copy new value to
check whether it has changed and does not update ipv4_devconf.forwarding
before calling inet_forward_change(). (it is copied internally by sysctl
after ipv4_sysctl_forward_strategy because we return positive number)

I am not good in kernel parallel computing strategy, whether it requires
some locking or it is safe to do:

--- sysctl_net_ipv4.c.old       2003-08-03 17:37:44.000000000 +0200
+++ sysctl_net_ipv4.c   2003-08-03 17:38:18.000000000 +0200
@@ -109,8 +109,9 @@ static int ipv4_sysctl_forward_strategy(
                 }
         }
  
+        ipv4_devconf.forwarding=new;
         inet_forward_change();
-        return 1;
+        return 0;
 }
  
 ctl_table ipv4_table[] = {



Best Regards,

-- 
Jan Oravec                           XS26 coordinator
6COM s.r.o.                          'Access to IPv6'
http://www.6com.sk                   http://www.xs26.net

Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20030803154427.GA12926@wsx.ksp.sk> (at Sun, 3 Aug 2003 17:44:27 +0200), Jan Oravec <jan.oravec@6com.sk> says:

> For IPv4, it is because ipv4_sysctl_forward_strategy only copy new value to
> check whether it has changed and does not update ipv4_devconf.forwarding
> before calling inet_forward_change(). (it is copied internally by sysctl
> after ipv4_sysctl_forward_strategy because we return positive number)
> 
> I am not good in kernel parallel computing strategy, whether it requires
> some locking or it is safe to do:
> 
> --- sysctl_net_ipv4.c.old       2003-08-03 17:37:44.000000000 +0200
> +++ sysctl_net_ipv4.c   2003-08-03 17:38:18.000000000 +0200
> @@ -109,8 +109,9 @@ static int ipv4_sysctl_forward_strategy(
>                  }
>          }
>   
> +        ipv4_devconf.forwarding=new;
                                   = new;
>          inet_forward_change();
> -        return 1;
> +        return 0;
>  }
>   
>  ctl_table ipv4_table[] = {

It seems correct, however, this patch cannot apply against 
current tree.  Pleaes resend the patch.
And please make sure to diff like this:
diff -u linux-2.6/net/ipv4/sysctl_net_ipv4.c.old linux-2.6/net/ipv4/sysctl_net_ipv4.c

Thank you.

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

[PATCH] IPV6: strategy hander for net.ipv6.conf.*.forwarding (is Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call)

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20030803154427.GA12926@wsx.ksp.sk> (at Sun, 3 Aug 2003 17:44:27 +0200), Jan Oravec <jan.oravec@6com.sk> says:

> For IPv6, it is obviously because sysctl 'strategy' handler is not defined.

Here's tha patch to add strategy handler for 
net.ipv6.conf.*.forwarding.  Thanks.

Index: linux-2.6/net/ipv6/addrconf.c
===================================================================
RCS file: /home/cvs/linux-2.5/net/ipv6/addrconf.c,v
retrieving revision 1.48
diff -u -r1.48 addrconf.c
--- linux-2.6/net/ipv6/addrconf.c	25 Jul 2003 23:58:59 -0000	1.48
+++ linux-2.6/net/ipv6/addrconf.c	8 Aug 2003 07:12:13 -0000
@@ -2593,6 +2593,48 @@
         return ret;
 }
 
+static int addrconf_sysctl_forward_strategy(ctl_table *table, 
+					    int *name, int nlen,
+					    void *oldval, size_t *oldlenp,
+					    void *newval, size_t newlen,
+					    void **context)
+{
+	int *valp = table->data;
+	int new;
+
+	if (!newval || !newlen)
+		return 0;
+	if (newlen != sizeof(int))
+		return -EINVAL;
+	if (get_user(new, (int *)newval))
+		return -EFAULT;
+	if (new == *valp)
+		return 0;
+	if (oldval && oldlenp) {
+		size_t len;
+		if (get_user(len, oldlenp))
+			return -EFAULT;
+		if (len) {
+			if (len > table->maxlen)
+				len = table->maxlen;
+			if (copy_to_user(oldval, valp, len))
+				return -EFAULT;
+			if (put_user(len, oldlenp))
+				return -EFAULT;
+		}
+	}
+
+	*valp = new;
+	if (valp != &ipv6_devconf.forwarding &&
+	    valp != &ipv6_devconf_dflt.forwarding) {
+		struct inet6_dev *idev = (struct inet6_dev *)table->extra1;
+		if (!idev)
+			return -ENODEV;
+		addrconf_forward_change(idev);
+	}
+	return 0;
+}
+
 static struct addrconf_sysctl_table
 {
 	struct ctl_table_header *sysctl_header;
@@ -2611,6 +2653,7 @@
 			.maxlen		=	sizeof(int),
 			.mode		=	0644,
          		.proc_handler	=	&addrconf_sysctl_forward,
+			.strategy	=	&addrconf_sysctl_forward_strategy,
 		},
 		{
 			.ctl_name	=	NET_IPV6_HOP_LIMIT,


-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call

[Jan Oravec]

Hello,


this is a new patch

> It seems correct, however, this patch cannot apply against 
> current tree.  Pleaes resend the patch.
> And please make sure to diff like this:
> diff -u linux-2.6/net/ipv4/sysctl_net_ipv4.c.old linux-2.6/net/ipv4/sysctl_net_ipv4.c

diff -u linux-2.6.0-test2/net/ipv4/sysctl_net_ipv4.c.old linux-2.6.0-test2/net/ipv4/sysctl_net_ipv4.c
--- linux-2.6.0-test2/net/ipv4/sysctl_net_ipv4.c.old	2003-07-27 19:06:19.000000000 +0200
+++ linux-2.6.0-test2/net/ipv4/sysctl_net_ipv4.c	2003-08-08 10:56:31.000000000 +0200
@@ -109,8 +109,9 @@
 		}
 	}
 
+	ipv4_devconf.forwarding = new;
 	inet_forward_change();
-	return 1;
+	return 0;
 }
 
 ctl_table ipv4_table[] = {


Thanks,

-- 
Jan Oravec                           XS26 coordinator
6COM s.r.o.                          'Access to IPv6'
http://www.6com.sk                   http://www.xs26.net

Re: [PATCH] IPV6: strategy hander for net.ipv6.conf.*.forwarding (is Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call)

[Jan Oravec]

On Fri, Aug 08, 2003 at 05:50:30PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:

> +	*valp = new;
> +	if (valp != &ipv6_devconf.forwarding &&
> +	    valp != &ipv6_devconf_dflt.forwarding) {
> +		struct inet6_dev *idev = (struct inet6_dev *)table->extra1;
> +		if (!idev)
> +			return -ENODEV;
> +		addrconf_forward_change(idev);
> +	}
> +	return 0;
> +}

Shouldn't we set ipv6_devconf_dflt.forwarding and call
addr_forward_change(NULL) in case that valp==&ipv6_devconf.forwarding?


-- 
Jan Oravec                           XS26 coordinator
6COM s.r.o.                          'Access to IPv6'
http://www.6com.sk                   http://www.xs26.net

Re: [PATCH] IPV6: strategy hander for net.ipv6.conf.*.forwarding (is Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call)

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20030808093704.GA18131@wsx.ksp.sk> (at Fri, 8 Aug 2003 11:37:04 +0200), Jan Oravec <jan.oravec@6com.sk> says:

> On Fri, Aug 08, 2003 at 05:50:30PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> > +	*valp = new;
> > +	if (valp != &ipv6_devconf.forwarding &&
> > +	    valp != &ipv6_devconf_dflt.forwarding) {
> > +		struct inet6_dev *idev = (struct inet6_dev *)table->extra1;
> > +		if (!idev)
> > +			return -ENODEV;
> > +		addrconf_forward_change(idev);
> > +	}
> > +	return 0;
> > +}
> 
> Shouldn't we set ipv6_devconf_dflt.forwarding and call
> addr_forward_change(NULL) in case that valp==&ipv6_devconf.forwarding?

Oh, You're right. Here's the revised one:

Index: linux-2.6/net/ipv6/addrconf.c
===================================================================
RCS file: /home/cvs/linux-2.5/net/ipv6/addrconf.c,v
retrieving revision 1.48
diff -u -r1.48 addrconf.c
--- linux-2.6/net/ipv6/addrconf.c	25 Jul 2003 23:58:59 -0000	1.48
+++ linux-2.6/net/ipv6/addrconf.c	8 Aug 2003 08:21:56 -0000
@@ -2593,6 +2593,51 @@
         return ret;
 }
 
+static int addrconf_sysctl_forward_strategy(ctl_table *table, 
+					    int *name, int nlen,
+					    void *oldval, size_t *oldlenp,
+					    void *newval, size_t newlen,
+					    void **context)
+{
+	int *valp = table->data;
+	int new;
+
+	if (!newval || !newlen)
+		return 0;
+	if (newlen != sizeof(int))
+		return -EINVAL;
+	if (get_user(new, (int *)newval))
+		return -EFAULT;
+	if (new == *valp)
+		return 0;
+	if (oldval && oldlenp) {
+		size_t len;
+		if (get_user(len, oldlenp))
+			return -EFAULT;
+		if (len) {
+			if (len > table->maxlen)
+				len = table->maxlen;
+			if (copy_to_user(oldval, valp, len))
+				return -EFAULT;
+			if (put_user(len, oldlenp))
+				return -EFAULT;
+		}
+	}
+
+	*valp = new;
+	if (valp != &ipv6_devconf_dflt.forwarding) {
+		struct inet6_dev *idev;
+		if (valp != &ipv6_devconf.forwarding) {
+			idev = (struct inet6_dev *)table->extra1;
+			if (unlikely(idev == NULL))
+				return -ENODEV;
+		} else
+			idev = NULL;
+		addrconf_forward_change(idev);
+	}
+	return 0;
+}
+
 static struct addrconf_sysctl_table
 {
 	struct ctl_table_header *sysctl_header;
@@ -2611,6 +2656,7 @@
 			.maxlen		=	sizeof(int),
 			.mode		=	0644,
          		.proc_handler	=	&addrconf_sysctl_forward,
+			.strategy	=	&addrconf_sysctl_forward_strategy,
 		},
 		{
 			.ctl_name	=	NET_IPV6_HOP_LIMIT,

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [PATCH] IPV6: strategy hander for net.ipv6.conf.*.forwarding (is Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call)

[Jan Oravec]

On Fri, Aug 08, 2003 at 06:51:35PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> Oh, You're right. Here's the revised one:

Thanks, I tried it and it works.


-- 
Jan Oravec                           XS26 coordinator
6COM s.r.o.                          'Access to IPv6'
http://www.6com.sk                   http://www.xs26.net

Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call

[David S. Miller]

On Fri, 8 Aug 2003 11:11:24 +0200
Jan Oravec <jan.oravec@6com.sk> wrote:

> this is a new patch
 ...
> +	ipv4_devconf.forwarding = new;
>  	inet_forward_change();
> -	return 1;
> +	return 0;

This is still wrong.

First of all we have the table->data pointer in "valp"
so let's use that to set the value.  In this way we can
use this function for other sysctl values if we ever desire
to do that.

Second, if we set the table->data value, we must return > 0.
This tells the caller that we've done the sysctl value update.
If we return zero, it would update the value a second time.

Here is the fix I'm going to use.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1136  -> 1.1137 
#	net/ipv4/sysctl_net_ipv4.c	1.11    -> 1.12   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/08/09	davem@nuts.ninka.net	1.1137
# [IPV4]: Fix setting net.ipv4.conf.all.forwarding via sysctl() system call.
# --------------------------------------------
#
diff -Nru a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
--- a/net/ipv4/sysctl_net_ipv4.c	Sat Aug  9 01:02:17 2003
+++ b/net/ipv4/sysctl_net_ipv4.c	Sat Aug  9 01:02:17 2003
@@ -109,6 +109,7 @@
 		}
 	}
 
+	*valp = new;
 	inet_forward_change();
 	return 1;
 }

Re: [PATCH] IPV6: strategy hander for net.ipv6.conf.*.forwarding (is Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call)

[David S. Miller]

On Fri, 08 Aug 2003 18:51:35 +0900 (JST)
YOSHIFUJI Hideaki / _$B5HF#1QL@ <yoshfuji@linux-ipv6.org> wrote:

> In article <20030808093704.GA18131@wsx.ksp.sk> (at Fri, 8 Aug 2003 11:37:04 +0200), Jan Oravec <jan.oravec@6com.sk> says:
> 
> > On Fri, Aug 08, 2003 at 05:50:30PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> > 
> > > +	*valp = new;
> > > +	if (valp != &ipv6_devconf.forwarding &&
> > > +	    valp != &ipv6_devconf_dflt.forwarding) {
> > > +		struct inet6_dev *idev = (struct inet6_dev *)table->extra1;
> > > +		if (!idev)
> > > +			return -ENODEV;
> > > +		addrconf_forward_change(idev);
> > > +	}
> > > +	return 0;
> > > +}
> > 
> > Shouldn't we set ipv6_devconf_dflt.forwarding and call
> > addr_forward_change(NULL) in case that valp==&ipv6_devconf.forwarding?
> 
> Oh, You're right. Here's the revised one:

As I mentioned for the ipv4 forwarding sysctl bug fix,
if you will set table->data yourself you should return
> 0 (for example "1") from your strategy handler.

The patch looks fine otherwise.

Please fix this, thank you.

Re: [PATCH] IPV6: strategy hander for net.ipv6.conf.*.forwarding (is Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call)

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20030809010845.01ebcbe9.davem@redhat.com> (at Sat, 9 Aug 2003 01:08:45 -0700), "David S. Miller" <davem@redhat.com> says:

> As I mentioned for the ipv4 forwarding sysctl bug fix,
> if you will set table->data yourself you should return
> > 0 (for example "1") from your strategy handler.
:
> Please fix this, thank you.

Here it is.

Index: linux-2.6/net/ipv6/addrconf.c
===================================================================
RCS file: /home/cvs/linux-2.5/net/ipv6/addrconf.c,v
retrieving revision 1.48
diff -u -r1.48 addrconf.c
--- linux-2.6/net/ipv6/addrconf.c	25 Jul 2003 23:58:59 -0000	1.48
+++ linux-2.6/net/ipv6/addrconf.c	9 Aug 2003 08:51:42 -0000
@@ -2593,6 +2593,53 @@
         return ret;
 }
 
+static int addrconf_sysctl_forward_strategy(ctl_table *table, 
+					    int *name, int nlen,
+					    void *oldval, size_t *oldlenp,
+					    void *newval, size_t newlen,
+					    void **context)
+{
+	int *valp = table->data;
+	int new;
+
+	if (!newval || !newlen)
+		return 0;
+	if (newlen != sizeof(int))
+		return -EINVAL;
+	if (get_user(new, (int *)newval))
+		return -EFAULT;
+	if (new == *valp)
+		return 0;
+	if (oldval && oldlenp) {
+		size_t len;
+		if (get_user(len, oldlenp))
+			return -EFAULT;
+		if (len) {
+			if (len > table->maxlen)
+				len = table->maxlen;
+			if (copy_to_user(oldval, valp, len))
+				return -EFAULT;
+			if (put_user(len, oldlenp))
+				return -EFAULT;
+		}
+	}
+
+	if (valp != &ipv6_devconf_dflt.forwarding) {
+		struct inet6_dev *idev;
+		if (valp != &ipv6_devconf.forwarding) {
+			idev = (struct inet6_dev *)table->extra1;
+			if (unlikely(idev == NULL))
+				return -ENODEV;
+		} else
+			idev = NULL;
+		*valp = new;
+		addrconf_forward_change(idev);
+	} else
+		*valp = new;
+
+	return 1;
+}
+
 static struct addrconf_sysctl_table
 {
 	struct ctl_table_header *sysctl_header;
@@ -2611,6 +2658,7 @@
 			.maxlen		=	sizeof(int),
 			.mode		=	0644,
          		.proc_handler	=	&addrconf_sysctl_forward,
+			.strategy	=	&addrconf_sysctl_forward_strategy,
 		},
 		{
 			.ctl_name	=	NET_IPV6_HOP_LIMIT,


-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [PATCH] IPV6: strategy hander for net.ipv6.conf.*.forwarding (is Re: problem setting net.ipvX.conf.all.forwarding via sysctl() system call)

[David S. Miller]

On Sat, 09 Aug 2003 19:21:58 +0900 (JST)
YOSHIFUJI Hideaki / _$B5HF#1QL@ <yoshfuji@linux-ipv6.org> wrote:

> In article <20030809010845.01ebcbe9.davem@redhat.com> (at Sat, 9 Aug 2003 01:08:45 -0700), "David S. Miller" <davem@redhat.com> says:
> 
> > As I mentioned for the ipv4 forwarding sysctl bug fix,
> > if you will set table->data yourself you should return
> > > 0 (for example "1") from your strategy handler.
> :
> > Please fix this, thank you.
> 
> Here it is.

Patch applied, thank you.