diff mbox

[2/2] ipv6: Allow accepting RA from local IP addresses.

Message ID 1403113806-13004-2-git-send-email-greearb@candelatech.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Ben Greear June 18, 2014, 5:50 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

This can be used in virtual networking applications, and
may have other uses as well.  The option is disabled by
default, so no change to current operating behaviour
without the user explicitly changing the behaviour.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

This is against 3.14.7+

 Documentation/networking/ip-sysctl.txt | 12 ++++++++++++
 include/linux/ipv6.h                   |  1 +
 include/uapi/linux/ipv6.h              |  1 +
 include/uapi/linux/sysctl.h            |  1 +
 kernel/sysctl_binary.c                 |  1 +
 net/ipv6/addrconf.c                    | 10 ++++++++++
 net/ipv6/ndisc.c                       |  8 ++++++--
 7 files changed, 32 insertions(+), 2 deletions(-)

Comments

Hannes Frederic Sowa June 20, 2014, 3:40 p.m. UTC | #1 
On Mi, 2014-06-18 at 10:50 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> This can be used in virtual networking applications, and
> may have other uses as well.  The option is disabled by
> default, so no change to current operating behaviour
> without the user explicitly changing the behaviour.

Can you give a specific example for its use case? I currently don't see
the need for such an option.

 
> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> +	if (!(in6_dev->cnf.accept_ra_from_local ||
> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>  			  NULL, 0)) {
>  		ND_PRINTK(2, info,
>  			  "RA: %s, chk_addr failed for dev: %s\n",
> @@ -1293,7 +1295,9 @@ skip_linkparms:
>  	}
>  
>  #ifdef CONFIG_IPV6_ROUTE_INFO
> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> +	if (!(in6_dev->cnf.accept_ra_from_local ||
> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>  			  NULL, 0)) {
>  		ND_PRINTK(2, info,
>  			  "RA: %s, chk-addr (route info) is false for dev: %s\n",

Maybe ipv6_accept_ra_local() like ipv6_accept_ra() static local to the
file?

Also I am not sure if we want to provide an devconf_all for this setting
at all, like we don't evaluate it for accept_ra, too. At least I
wouldn't do so with the current state of ipv6/conf/{all,default}.

Bye,
Hannes


--
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
Ben Greear June 20, 2014, 4:31 p.m. UTC | #2 
On 06/20/2014 08:40 AM, Hannes Frederic Sowa wrote:
> On Mi, 2014-06-18 at 10:50 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> This can be used in virtual networking applications, and
>> may have other uses as well.  The option is disabled by
>> default, so no change to current operating behaviour
>> without the user explicitly changing the behaviour.
> 
> Can you give a specific example for its use case? I currently don't see
> the need for such an option.

I put radvd on one veth endpoint, and use other veth endpoint to act
as normal-ish endpoint with IPv6.

The one with radvd enables routing, using specific rules so that it
can only route to a few other interfaces.

Basically, I can emulate multi-hop routed and bridged networks, including with
OSPF and such on a single machine without the use of network
namespaces or virtual machines.

We use this to make network testing products, but I figure someone somewhere
will find a different reason to want this.  As far as I know, this used to
work w/out any kernel hacks, though I have not specifically verified
this.  It did show up as a regression in our testing, but possibly we
failed to test it properly years ago...


>> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>> +	if (!(in6_dev->cnf.accept_ra_from_local ||
>> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
>> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>  			  NULL, 0)) {
>>  		ND_PRINTK(2, info,
>>  			  "RA: %s, chk_addr failed for dev: %s\n",
>> @@ -1293,7 +1295,9 @@ skip_linkparms:
>>  	}
>>  
>>  #ifdef CONFIG_IPV6_ROUTE_INFO
>> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>> +	if (!(in6_dev->cnf.accept_ra_from_local ||
>> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
>> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>  			  NULL, 0)) {
>>  		ND_PRINTK(2, info,
>>  			  "RA: %s, chk-addr (route info) is false for dev: %s\n",
> 
> Maybe ipv6_accept_ra_local() like ipv6_accept_ra() static local to the
> file?

I don't have a preference either way, but will make the change if it helps
upstream acceptance.

> Also I am not sure if we want to provide an devconf_all for this setting
> at all, like we don't evaluate it for accept_ra, too. At least I
> wouldn't do so with the current state of ipv6/conf/{all,default}.

We often have thousands of interfaces on a system, it saves effort to
set this globally.  Note that it will not over-ride any other restraints,
so a routed interface will still not accept RA unless additional
existing procfs config changes are made, etc.

Both global and per-interface default to disabling this new feature,
so I think it is safe as I have written it.


Thanks,
Ben

> 
> Bye,
> Hannes
> 
>
Hannes Frederic Sowa June 23, 2014, 8:29 a.m. UTC | #3 
On Fr, 2014-06-20 at 09:31 -0700, Ben Greear wrote:
> On 06/20/2014 08:40 AM, Hannes Frederic Sowa wrote:
> > On Mi, 2014-06-18 at 10:50 -0700, greearb@candelatech.com wrote:
> >> From: Ben Greear <greearb@candelatech.com>
> >>
> >> This can be used in virtual networking applications, and
> >> may have other uses as well.  The option is disabled by
> >> default, so no change to current operating behaviour
> >> without the user explicitly changing the behaviour.
> > 
> > Can you give a specific example for its use case? I currently don't see
> > the need for such an option.
> 
> I put radvd on one veth endpoint, and use other veth endpoint to act
> as normal-ish endpoint with IPv6.
> 
> The one with radvd enables routing, using specific rules so that it
> can only route to a few other interfaces.
>
> Basically, I can emulate multi-hop routed and bridged networks, including with
> OSPF and such on a single machine without the use of network
> namespaces or virtual machines.
> 
> We use this to make network testing products, but I figure someone somewhere
> will find a different reason to want this.  As far as I know, this used to
> work w/out any kernel hacks, though I have not specifically verified
> this.  It did show up as a regression in our testing, but possibly we
> failed to test it properly years ago...

Ok, I see your point.

The forwarding knob is not always handled on a per-interface basis
because in some situations we don't know which interface we need to
process the packet on beforehand.

I don't know... In the end, it doesn't seem to cause any problems to me,
even if enabled, and you actually seem to use this feature, so should be
fine by me. Also, we already have some strange sysctls to play with.

> >> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> >> +	if (!(in6_dev->cnf.accept_ra_from_local ||
> >> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
> >> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> >>  			  NULL, 0)) {
> >>  		ND_PRINTK(2, info,
> >>  			  "RA: %s, chk_addr failed for dev: %s\n",
> >> @@ -1293,7 +1295,9 @@ skip_linkparms:
> >>  	}
> >>  
> >>  #ifdef CONFIG_IPV6_ROUTE_INFO
> >> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> >> +	if (!(in6_dev->cnf.accept_ra_from_local ||
> >> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
> >> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> >>  			  NULL, 0)) {
> >>  		ND_PRINTK(2, info,
> >>  			  "RA: %s, chk-addr (route info) is false for dev: %s\n",
> > 
> > Maybe ipv6_accept_ra_local() like ipv6_accept_ra() static local to the
> > file?
> 
> I don't have a preference either way, but will make the change if it helps
> upstream acceptance.

Just thought it would make the code a bit more readable.

> > Also I am not sure if we want to provide an devconf_all for this setting
> > at all, like we don't evaluate it for accept_ra, too. At least I
> > wouldn't do so with the current state of ipv6/conf/{all,default}.
> 
> We often have thousands of interfaces on a system, it saves effort to
> set this globally.  Note that it will not over-ride any other restraints,
> so a routed interface will still not accept RA unless additional
> existing procfs config changes are made, etc.
> 
> Both global and per-interface default to disabling this new feature,
> so I think it is safe as I have written it.

How do you handle the forwarding flag? You enable forwarding globally
and afterwards you disable it again on some interfaces? Otherwise you
won't get correct forwarding behavior.

Hm, I still don't like it to be a possible global setting (I am in favor
that it gets handled like ipv6_accept_ra). It looks much clearer to me
if it would behave like the accept_ra knob. Is it a problem to enable it
only on a per interface basis regarding races when the interfaces get
instantiated?

Bye,
Hannes


--
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
Ben Greear June 23, 2014, 5:28 p.m. UTC | #4 
On 06/23/2014 01:29 AM, Hannes Frederic Sowa wrote:
> On Fr, 2014-06-20 at 09:31 -0700, Ben Greear wrote:
>> On 06/20/2014 08:40 AM, Hannes Frederic Sowa wrote:
>>> On Mi, 2014-06-18 at 10:50 -0700, greearb@candelatech.com wrote:
>>>> From: Ben Greear <greearb@candelatech.com>
>>>>
>>>> This can be used in virtual networking applications, and
>>>> may have other uses as well.  The option is disabled by
>>>> default, so no change to current operating behaviour
>>>> without the user explicitly changing the behaviour.
>>>
>>> Can you give a specific example for its use case? I currently don't see
>>> the need for such an option.
>>
>> I put radvd on one veth endpoint, and use other veth endpoint to act
>> as normal-ish endpoint with IPv6.
>>
>> The one with radvd enables routing, using specific rules so that it
>> can only route to a few other interfaces.
>>
>> Basically, I can emulate multi-hop routed and bridged networks, including with
>> OSPF and such on a single machine without the use of network
>> namespaces or virtual machines.
>>
>> We use this to make network testing products, but I figure someone somewhere
>> will find a different reason to want this.  As far as I know, this used to
>> work w/out any kernel hacks, though I have not specifically verified
>> this.  It did show up as a regression in our testing, but possibly we
>> failed to test it properly years ago...
> 
> Ok, I see your point.
> 
> The forwarding knob is not always handled on a per-interface basis
> because in some situations we don't know which interface we need to
> process the packet on beforehand.
> 
> I don't know... In the end, it doesn't seem to cause any problems to me,
> even if enabled, and you actually seem to use this feature, so should be
> fine by me. Also, we already have some strange sysctls to play with.
> 
>>>> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>>> +	if (!(in6_dev->cnf.accept_ra_from_local ||
>>>> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
>>>> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>>>  			  NULL, 0)) {
>>>>  		ND_PRINTK(2, info,
>>>>  			  "RA: %s, chk_addr failed for dev: %s\n",
>>>> @@ -1293,7 +1295,9 @@ skip_linkparms:
>>>>  	}
>>>>  
>>>>  #ifdef CONFIG_IPV6_ROUTE_INFO
>>>> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>>> +	if (!(in6_dev->cnf.accept_ra_from_local ||
>>>> +	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
>>>> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>>>  			  NULL, 0)) {
>>>>  		ND_PRINTK(2, info,
>>>>  			  "RA: %s, chk-addr (route info) is false for dev: %s\n",
>>>
>>> Maybe ipv6_accept_ra_local() like ipv6_accept_ra() static local to the
>>> file?
>>
>> I don't have a preference either way, but will make the change if it helps
>> upstream acceptance.
> 
> Just thought it would make the code a bit more readable.
> 
>>> Also I am not sure if we want to provide an devconf_all for this setting
>>> at all, like we don't evaluate it for accept_ra, too. At least I
>>> wouldn't do so with the current state of ipv6/conf/{all,default}.
>>
>> We often have thousands of interfaces on a system, it saves effort to
>> set this globally.  Note that it will not over-ride any other restraints,
>> so a routed interface will still not accept RA unless additional
>> existing procfs config changes are made, etc.
>>
>> Both global and per-interface default to disabling this new feature,
>> so I think it is safe as I have written it.
> 
> How do you handle the forwarding flag? You enable forwarding globally
> and afterwards you disable it again on some interfaces? Otherwise you
> won't get correct forwarding behavior.
> 
> Hm, I still don't like it to be a possible global setting (I am in favor
> that it gets handled like ipv6_accept_ra). It looks much clearer to me
> if it would behave like the accept_ra knob. Is it a problem to enable it
> only on a per interface basis regarding races when the interfaces get
> instantiated?

I disable forwarding on all interfaces, then specifically enable it on the
ones in virtual routers.  It seems to work as expected.  I recall we do
have a multicast bug open for ipv6, but it's low priority and I have not
looked at the problem in detail yet.  Possibly it is related to some
of this...

I can make accept-local-ra a strictly per-interface setting.

I'll post a new patch soon-ish.

Thanks,
Ben
diff mbox

Patch

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index a30ecc6..322cf5f 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -1223,6 +1223,18 @@  accept_ra_defrtr - BOOLEAN
 	Functional default: enabled if accept_ra is enabled.
 			    disabled if accept_ra is disabled.
 
+accept_ra_from_local - BOOLEAN
+	Accept RA with source-address that is found on local machine
+        if the RA is otherwise proper and able to be accepted.
+        Default is to NOT accept these as it may be an un-intended
+        network loop.
+
+	Functional default:
+           enabled if accept_ra_from_local is enabled
+               on specific interface or 'all'.
+	   disabled if accept_ra_from_local is disabled
+               on 'all' AND on specific interface.
+
 accept_ra_pinfo - BOOLEAN
 	Learn Prefix Information in Router Advertisement.
 
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 2faef33..de635d1 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -39,6 +39,7 @@  struct ipv6_devconf {
 #endif
 	__s32		proxy_ndp;
 	__s32		accept_source_route;
+	__s32		accept_ra_from_local;
 #ifdef CONFIG_IPV6_OPTIMISTIC_DAD
 	__s32		optimistic_dad;
 #endif
diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index 593b0e3..efa2666 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -163,6 +163,7 @@  enum {
 	DEVCONF_MLDV1_UNSOLICITED_REPORT_INTERVAL,
 	DEVCONF_MLDV2_UNSOLICITED_REPORT_INTERVAL,
 	DEVCONF_SUPPRESS_FRAG_NDISC,
+	DEVCONF_ACCEPT_RA_FROM_LOCAL,
 	DEVCONF_MAX
 };
 
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 7544146..ce15796 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -568,6 +568,7 @@  enum {
 	NET_IPV6_ACCEPT_RA_RT_INFO_MAX_PLEN=22,
 	NET_IPV6_PROXY_NDP=23,
 	NET_IPV6_ACCEPT_SOURCE_ROUTE=25,
+	NET_IPV6_ACCEPT_RA_FROM_LOCAL=26,
 	__NET_IPV6_MAX
 };
 
diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index 6eacbcf..e10b51a 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -523,6 +523,7 @@  static const struct bin_table bin_net_ipv6_conf_var_table[] = {
 	{ CTL_INT,	NET_IPV6_ACCEPT_RA_RT_INFO_MAX_PLEN,	"accept_ra_rt_info_max_plen" },
 	{ CTL_INT,	NET_IPV6_PROXY_NDP,			"proxy_ndp" },
 	{ CTL_INT,	NET_IPV6_ACCEPT_SOURCE_ROUTE,		"accept_source_route" },
+	{ CTL_INT,	NET_IPV6_ACCEPT_RA_FROM_LOCAL,		"accept_ra_from_local" },
 	{}
 };
 
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6c7fa08..f0b7305 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -186,6 +186,7 @@  static struct ipv6_devconf ipv6_devconf __read_mostly = {
 	.max_desync_factor	= MAX_DESYNC_FACTOR,
 	.max_addresses		= IPV6_MAX_ADDRESSES,
 	.accept_ra_defrtr	= 1,
+	.accept_ra_from_local	= 0,
 	.accept_ra_pinfo	= 1,
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	.accept_ra_rtr_pref	= 1,
@@ -222,6 +223,7 @@  static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 	.max_desync_factor	= MAX_DESYNC_FACTOR,
 	.max_addresses		= IPV6_MAX_ADDRESSES,
 	.accept_ra_defrtr	= 1,
+	.accept_ra_from_local	= 0,
 	.accept_ra_pinfo	= 1,
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	.accept_ra_rtr_pref	= 1,
@@ -4326,6 +4328,7 @@  static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 	array[DEVCONF_FORCE_TLLAO] = cnf->force_tllao;
 	array[DEVCONF_NDISC_NOTIFY] = cnf->ndisc_notify;
 	array[DEVCONF_SUPPRESS_FRAG_NDISC] = cnf->suppress_frag_ndisc;
+	array[DEVCONF_ACCEPT_RA_FROM_LOCAL] = cnf->accept_ra_from_local;
 }
 
 static inline size_t inet6_ifla6_size(void)
@@ -5173,6 +5176,13 @@  static struct addrconf_sysctl_table
 			.proc_handler	= proc_dointvec
 		},
 		{
+			.procname	= "accept_ra_from_local",
+			.data		= &ipv6_devconf.accept_ra_from_local,
+			.maxlen		= sizeof(int),
+			.mode		= 0644,
+			.proc_handler	= proc_dointvec,
+		},
+		{
 			/* sentinel */
 		}
 	},
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 2fef3d5..f074e8c 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1151,7 +1151,9 @@  static void ndisc_router_discovery(struct sk_buff *skb)
 		goto skip_defrtr;
 	}
 
-	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
+	if (!(in6_dev->cnf.accept_ra_from_local ||
+	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
+	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
 			  NULL, 0)) {
 		ND_PRINTK(2, info,
 			  "RA: %s, chk_addr failed for dev: %s\n",
@@ -1293,7 +1295,9 @@  skip_linkparms:
 	}
 
 #ifdef CONFIG_IPV6_ROUTE_INFO
-	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
+	if (!(in6_dev->cnf.accept_ra_from_local ||
+	      dev_net(in6_dev->dev)->ipv6.devconf_all->accept_ra_from_local) &&
+	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
 			  NULL, 0)) {
 		ND_PRINTK(2, info,
 			  "RA: %s, chk-addr (route info) is false for dev: %s\n",