[PATCH v2 1/2] ipv6: Add more debugging around accept-ra logic.

[PATCH v2 1/2] ipv6: Add more debugging around accept-ra logic.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This is disabled by default, just like similar debug info
already in this module.  But, makes it easier to find out
why RA is not being accepted when debugging strange behaviour.

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

v2: No changes since last time.  Patch is against 3.14.8+

 net/ipv6/ndisc.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 09a22f4..2fef3d5 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1073,6 +1073,9 @@ static void ndisc_router_discovery(struct sk_buff *skb)
 	optlen = (skb_tail_pointer(skb) - skb_transport_header(skb)) -
 		sizeof(struct ra_msg);
 
+	ND_PRINTK(2, info,
+		  "RA: %s, dev: %s\n",
+		  __func__, skb->dev->name);
 	if (!(ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL)) {
 		ND_PRINTK(2, warn, "RA: source address is not link-local\n");
 		return;
@@ -1105,13 +1108,21 @@ static void ndisc_router_discovery(struct sk_buff *skb)
 		return;
 	}
 
-	if (!ipv6_accept_ra(in6_dev))
+	if (!ipv6_accept_ra(in6_dev)) {
+		ND_PRINTK(2, info,
+			  "RA: %s, did not accept ra for dev: %s\n",
+			  __func__, skb->dev->name);
 		goto skip_linkparms;
+	}
 
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
 	/* skip link-specific parameters from interior routers */
-	if (skb->ndisc_nodetype == NDISC_NODETYPE_NODEFAULT)
+	if (skb->ndisc_nodetype == NDISC_NODETYPE_NODEFAULT) {
+		ND_PRINTK(2, info,
+			  "RA: %s, nodetype is NODEFAULT, dev: %s\n",
+			  __func__, skb->dev->name);
 		goto skip_linkparms;
+	}
 #endif
 
 	if (in6_dev->if_flags & IF_RS_SENT) {
@@ -1133,11 +1144,20 @@ static void ndisc_router_discovery(struct sk_buff *skb)
 				(ra_msg->icmph.icmp6_addrconf_other ?
 					IF_RA_OTHERCONF : 0);
 
-	if (!in6_dev->cnf.accept_ra_defrtr)
+	if (!in6_dev->cnf.accept_ra_defrtr) {
+		ND_PRINTK(2, info,
+			  "RA: %s, defrtr is false for dev: %s\n",
+			  __func__, skb->dev->name);
 		goto skip_defrtr;
+	}
 
-	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr, NULL, 0))
+	if (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",
+			  __func__, skb->dev->name);
 		goto skip_defrtr;
+	}
 
 	lifetime = ntohs(ra_msg->icmph.icmp6_rt_lifetime);
 
@@ -1166,8 +1186,10 @@ static void ndisc_router_discovery(struct sk_buff *skb)
 		rt = NULL;
 	}
 
+	ND_PRINTK(3, info, "RA: rt: %p  lifetime: %d, for dev: %s\n",
+		  rt, lifetime, skb->dev->name);
 	if (rt == NULL && lifetime) {
-		ND_PRINTK(3, dbg, "RA: adding default router\n");
+		ND_PRINTK(3, info, "RA: adding default router\n");
 
 		rt = rt6_add_dflt_router(&ipv6_hdr(skb)->saddr, skb->dev, pref);
 		if (rt == NULL) {
@@ -1263,12 +1285,21 @@ skip_linkparms:
 			     NEIGH_UPDATE_F_ISROUTER);
 	}
 
-	if (!ipv6_accept_ra(in6_dev))
+	if (!ipv6_accept_ra(in6_dev)) {
+		ND_PRINTK(2, info,
+			  "RA: %s, accept_ra is false for dev: %s\n",
+			  __func__, skb->dev->name);
 		goto out;
+	}
 
 #ifdef CONFIG_IPV6_ROUTE_INFO
-	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr, NULL, 0))
+	if (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",
+			  __func__, skb->dev->name);
 		goto skip_routeinfo;
+	}
 
 	if (in6_dev->cnf.accept_ra_rtr_pref && ndopts.nd_opts_ri) {
 		struct nd_opt_hdr *p;
@@ -1296,8 +1327,12 @@ skip_routeinfo:
 
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
 	/* skip link-specific ndopts from interior routers */
-	if (skb->ndisc_nodetype == NDISC_NODETYPE_NODEFAULT)
+	if (skb->ndisc_nodetype == NDISC_NODETYPE_NODEFAULT) {
+		ND_PRINTK(2, info,
+			  "RA: %s, nodetype is NODEFAULT (interior routes), dev: %s\n",
+			  __func__, skb->dev->name);
 		goto out;
+	}
 #endif
 
 	if (in6_dev->cnf.accept_ra_pinfo && ndopts.nd_opts_pi) {
-- 
1.7.11.7

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

[greearb]

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>
---

v2:  Add suggested static helper method, do not consider
     the 'all' config option, only specific iface.
Patch is against 3.14.8+

 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                       | 23 +++++++++++++++++------
 7 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index a30ecc6..0656756 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 a specific interface.
+	   disabled if accept_ra_from_local is disabled
+               on a 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..2165d5e 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1057,6 +1057,19 @@ errout:
 	rtnl_set_sk_err(net, RTNLGRP_ND_USEROPT, err);
 }
 
+static bool ipv6_accept_ra_local(struct inet6_dev *in6_dev, struct sk_buf *skb)
+{
+	/* Do not accept RA with source-addr found on local machine unless
+	 * accept_ra_from_local is set to true.
+	 */
+	if (!in6_dev->cnf.accept_ra_from_local &&
+	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
+			  NULL, 0))
+		return false;
+	return true;
+}
+
+
 static void ndisc_router_discovery(struct sk_buff *skb)
 {
 	struct ra_msg *ra_msg = (struct ra_msg *)skb_transport_header(skb);
@@ -1151,10 +1164,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,
-			  NULL, 0)) {
+	if (!ipv6_accept_ra_local(in6_dev, skb)) {
 		ND_PRINTK(2, info,
-			  "RA: %s, chk_addr failed for dev: %s\n",
+			  "RA: %s, accept_ra_local failed for dev: %s\n",
 			  __func__, skb->dev->name);
 		goto skip_defrtr;
 	}
@@ -1293,10 +1305,9 @@ skip_linkparms:
 	}
 
 #ifdef CONFIG_IPV6_ROUTE_INFO
-	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
-			  NULL, 0)) {
+	if (!ipv6_accept_ra_local(in6_dev, skb)) {
 		ND_PRINTK(2, info,
-			  "RA: %s, chk-addr (route info) is false for dev: %s\n",
+			  "RA: %s, accept_ra_local (route info) failed for dev: %s\n",
 			  __func__, skb->dev->name);
 		goto skip_routeinfo;
 	}
-- 
1.7.11.7

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

[YOSHIFUJI Hideaki]

Hello.

(2014/06/25 6:14), 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

                                  standard compliant behavior?

> without the user explicitly changing the behaviour.
> 

Would you include your specific example?

> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> 
> v2:  Add suggested static helper method, do not consider
>       the 'all' config option, only specific iface.
> Patch is against 3.14.8+
> 
>   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                       | 23 +++++++++++++++++------
>   7 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
> index a30ecc6..0656756 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 a specific interface.
> +	   disabled if accept_ra_from_local is disabled
> +               on a 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..2165d5e 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -1057,6 +1057,19 @@ errout:
>   	rtnl_set_sk_err(net, RTNLGRP_ND_USEROPT, err);
>   }
>   
> +static bool ipv6_accept_ra_local(struct inet6_dev *in6_dev, struct sk_buf *skb)
> +{
> +	/* Do not accept RA with source-addr found on local machine unless
> +	 * accept_ra_from_local is set to true.
> +	 */
> +	if (!in6_dev->cnf.accept_ra_from_local &&
> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> +			  NULL, 0))
> +		return false;
> +	return true;
> +}
> +
> +
>   static void ndisc_router_discovery(struct sk_buff *skb)
>   {
>   	struct ra_msg *ra_msg = (struct ra_msg *)skb_transport_header(skb);
> @@ -1151,10 +1164,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,
> -			  NULL, 0)) {
> +	if (!ipv6_accept_ra_local(in6_dev, skb)) {
>   		ND_PRINTK(2, info,
> -			  "RA: %s, chk_addr failed for dev: %s\n",
> +			  "RA: %s, accept_ra_local failed for dev: %s\n",
>   			  __func__, skb->dev->name);
>   		goto skip_defrtr;
>   	}

Hmm, without global knob, I see little benefit by
having new helper.

At least, it should be called ipv6_chk_addr_ra(),
ipv6_check_ra_saddr(), ipv6_is_nonlocal_ra() or
something else.

I think we do not need to change debugging output,
or we could say "RA from local address detected; 
default router ignored." or something like.

> @@ -1293,10 +1305,9 @@ skip_linkparms:
>   	}
>   
>   #ifdef CONFIG_IPV6_ROUTE_INFO
> -	if (ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> -			  NULL, 0)) {
> +	if (!ipv6_accept_ra_local(in6_dev, skb)) {
>   		ND_PRINTK(2, info,
> -			  "RA: %s, chk-addr (route info) is false for dev: %s\n",
> +			  "RA: %s, accept_ra_local (route info) failed for dev: %s\n",
>   			  __func__, skb->dev->name);

I think it original is just okay because this message 
does not appears if the new option is enabled by hand.

About debugging output, similar for default router.

>   		goto skip_routeinfo;
>   	}
> 

--yoshfuji

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

[Ben Greear]

On 06/24/2014 03:22 PM, YOSHIFUJI Hideaki wrote:
> Hello.
> 
> (2014/06/25 6:14), 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
> 
>                                    standard compliant behavior?

I've no idea.  Can you point me to the proper standard (and
pertinent section)?

>> without the user explicitly changing the behaviour.
>>
> 
> Would you include your specific example?

I gave one in a response to comments on v1 of this patch.

Basically, I make a single OS instance look like a bunch of
routers, bridges, and hosts.  Without use of network namespaces,
virtual machines, or other such virtualization.  Just clever use
of ip rules and routes.  So, I need interfaces to be able to accept
RA from other interfaces on the same system.

http://www.spinics.net/lists/netdev/msg286764.html


>> +static bool ipv6_accept_ra_local(struct inet6_dev *in6_dev, struct sk_buf *skb)
>> +{
>> +	/* Do not accept RA with source-addr found on local machine unless
>> +	 * accept_ra_from_local is set to true.
>> +	 */
>> +	if (!in6_dev->cnf.accept_ra_from_local &&
>> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>> +			  NULL, 0))
>> +		return false;
>> +	return true;
>> +}
>> +
>> +
>>    static void ndisc_router_discovery(struct sk_buff *skb)
>>    {
>>    	struct ra_msg *ra_msg = (struct ra_msg *)skb_transport_header(skb);
>> @@ -1151,10 +1164,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,
>> -			  NULL, 0)) {
>> +	if (!ipv6_accept_ra_local(in6_dev, skb)) {
>>    		ND_PRINTK(2, info,
>> -			  "RA: %s, chk_addr failed for dev: %s\n",
>> +			  "RA: %s, accept_ra_local failed for dev: %s\n",
>>    			  __func__, skb->dev->name);
>>    		goto skip_defrtr;
>>    	}
> 
> Hmm, without global knob, I see little benefit by
> having new helper.

A previous reviewer requested it.  I don't care either
way, seems fine to open-code it to me.

> At least, it should be called ipv6_chk_addr_ra(),
> ipv6_check_ra_saddr(), ipv6_is_nonlocal_ra() or
> something else.
> 
> I think we do not need to change debugging output,
> or we could say "RA from local address detected;
> default router ignored." or something like.

That does seem like a more useful error message.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Hannes Frederic Sowa]

On Di, 2014-06-24 at 20:19 -0700, Ben Greear wrote:
> 
> On 06/24/2014 03:22 PM, YOSHIFUJI Hideaki wrote:
> > Hello.
> > 
> > (2014/06/25 6:14), 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
> > 
> >                                    standard compliant behavior?
> 
> I've no idea.  Can you point me to the proper standard (and
> pertinent section)?
>
> >> without the user explicitly changing the behaviour.
> >>
> > 
> > Would you include your specific example?
> 
> I gave one in a response to comments on v1 of this patch.

It would be nice if you could include this into the changelog.

> Basically, I make a single OS instance look like a bunch of
> routers, bridges, and hosts.  Without use of network namespaces,
> virtual machines, or other such virtualization.  Just clever use
> of ip rules and routes.  So, I need interfaces to be able to accept
> RA from other interfaces on the same system.
> 
> http://www.spinics.net/lists/netdev/msg286764.html
> 
> 
> >> +static bool ipv6_accept_ra_local(struct inet6_dev *in6_dev, struct sk_buf *skb)
> >> +{
> >> +	/* Do not accept RA with source-addr found on local machine unless
> >> +	 * accept_ra_from_local is set to true.
> >> +	 */
> >> +	if (!in6_dev->cnf.accept_ra_from_local &&
> >> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> >> +			  NULL, 0))
> >> +		return false;
> >> +	return true;
> >> +}
> >> +
> >> +
> >>    static void ndisc_router_discovery(struct sk_buff *skb)
> >>    {
> >>    	struct ra_msg *ra_msg = (struct ra_msg *)skb_transport_header(skb);
> >> @@ -1151,10 +1164,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,
> >> -			  NULL, 0)) {
> >> +	if (!ipv6_accept_ra_local(in6_dev, skb)) {
> >>    		ND_PRINTK(2, info,
> >> -			  "RA: %s, chk_addr failed for dev: %s\n",
> >> +			  "RA: %s, accept_ra_local failed for dev: %s\n",
> >>    			  __func__, skb->dev->name);
> >>    		goto skip_defrtr;
> >>    	}
> > 
> > Hmm, without global knob, I see little benefit by
> > having new helper.
> 
> A previous reviewer requested it.  I don't care either
> way, seems fine to open-code it to me.

Hmm, sorry to revert my opinion here. Passing a whole skb reference to
the helper function disqualifies this as a small helper function. ;)

I first thought about something like:

static bool ipv6_accept_ra_local(idev) {
        return in6_dev->cnf.accept_ra_from_local ||
               dev_net(idev->dev)->devconf_all.accept_ra_from_local;
}

...but without devconf_all->... test or alike it doesn't seem to make
much sense if you only process one flag, sorry.

Sorry,
Hannes

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

[YOSHIFUJI Hideaki/吉藤英明]

Hi,

2014/06/25 12:19, Ben Greear wrote:>
>
> On 06/24/2014 03:22 PM, YOSHIFUJI Hideaki wrote:
>> Hello.
>>
>> (2014/06/25 6:14), 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
>>
>>                                     standard compliant behavior?
>
> I've no idea.  Can you point me to the proper standard (and
> pertinent section)?

I was wrong.

I found this code was added by commit 9f56220 ("ipv6: Do not
use routes from locally generated RAs") to fix behavior when
accept_ra == 2.

But I do not understand why it is not enough to restrict local
address on receiving interface.

Andi, would you explain?

>
>>> without the user explicitly changing the behaviour.
>>>
>>
>> Would you include your specific example?
>
> I gave one in a response to comments on v1 of this patch.
>
> Basically, I make a single OS instance look like a bunch of
> routers, bridges, and hosts.  Without use of network namespaces,
> virtual machines, or other such virtualization.  Just clever use
> of ip rules and routes.  So, I need interfaces to be able to accept
> RA from other interfaces on the same system.
>
> http://www.spinics.net/lists/netdev/msg286764.html
>
>

>>> +static bool ipv6_accept_ra_local(struct inet6_dev *in6_dev, struct
sk_buf *skb)
>>> +{
>>> +	/* Do not accept RA with source-addr found on local machine unless
>>> +	 * accept_ra_from_local is set to true.
>>> +	 */
>>> +	if (!in6_dev->cnf.accept_ra_from_local &&
>>> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>> +			  NULL, 0))
>>> +		return false;
>>> +	return true;
>>> +}
>>> +
>>> +
>>>     static void ndisc_router_discovery(struct sk_buff *skb)
>>>     {
>>>     	struct ra_msg *ra_msg = (struct ra_msg *)skb_transport_header(skb);
>>> @@ -1151,10 +1164,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,
>>> -			  NULL, 0)) {
>>> +	if (!ipv6_accept_ra_local(in6_dev, skb)) {
>>>     		ND_PRINTK(2, info,
>>> -			  "RA: %s, chk_addr failed for dev: %s\n",
>>> +			  "RA: %s, accept_ra_local failed for dev: %s\n",
>>>     			  __func__, skb->dev->name);
>>>     		goto skip_defrtr;
>>>     	}
>>
>> Hmm, without global knob, I see little benefit by
>> having new helper.
>
> A previous reviewer requested it.  I don't care either
> way, seems fine to open-code it to me.
>
>> At least, it should be called ipv6_chk_addr_ra(),
>> ipv6_check_ra_saddr(), ipv6_is_nonlocal_ra() or
>> something else.
>>
>> I think we do not need to change debugging output,
>> or we could say "RA from local address detected;
>> default router ignored." or something like.
>
> That does seem like a more useful error message.
>
> Thanks,
> Ben
>

--yoshfuji

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

[David Miller]

From: YOSHIFUJI Hideaki/吉藤英明 <hideaki.yoshifuji@miraclelinux.com>
Date: Thu, 26 Jun 2014 09:49:54 +0900

> Hi,
> 
> 2014/06/25 12:19, Ben Greear wrote:>
>>
>> On 06/24/2014 03:22 PM, YOSHIFUJI Hideaki wrote:
>>> Hello.
>>>
>>> (2014/06/25 6:14), 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
>>>
>>>                                     standard compliant behavior?
>>
>> I've no idea.  Can you point me to the proper standard (and
>> pertinent section)?
> 
> I was wrong.
> 
> I found this code was added by commit 9f56220 ("ipv6: Do not
> use routes from locally generated RAs") to fix behavior when
> accept_ra == 2.
> 
> But I do not understand why it is not enough to restrict local
> address on receiving interface.
> 
> Andi, would you explain?

Added Andi to CC: list...

>>
>>>> without the user explicitly changing the behaviour.
>>>>
>>>
>>> Would you include your specific example?
>>
>> I gave one in a response to comments on v1 of this patch.
>>
>> Basically, I make a single OS instance look like a bunch of
>> routers, bridges, and hosts.  Without use of network namespaces,
>> virtual machines, or other such virtualization.  Just clever use
>> of ip rules and routes.  So, I need interfaces to be able to accept
>> RA from other interfaces on the same system.
>>
>> http://www.spinics.net/lists/netdev/msg286764.html
>>
>>
> 
>>>> +static bool ipv6_accept_ra_local(struct inet6_dev *in6_dev, struct
> sk_buf *skb)
>>>> +{
>>>> +	/* Do not accept RA with source-addr found on local machine unless
>>>> +	 * accept_ra_from_local is set to true.
>>>> +	 */
>>>> +	if (!in6_dev->cnf.accept_ra_from_local &&
>>>> +	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
>>>> +			  NULL, 0))
>>>> +		return false;
>>>> +	return true;
>>>> +}
>>>> +
>>>> +
>>>>     static void ndisc_router_discovery(struct sk_buff *skb)
>>>>     {
>>>>     	struct ra_msg *ra_msg = (struct ra_msg *)skb_transport_header(skb);
>>>> @@ -1151,10 +1164,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,
>>>> -			  NULL, 0)) {
>>>> +	if (!ipv6_accept_ra_local(in6_dev, skb)) {
>>>>     		ND_PRINTK(2, info,
>>>> -			  "RA: %s, chk_addr failed for dev: %s\n",
>>>> +			  "RA: %s, accept_ra_local failed for dev: %s\n",
>>>>     			  __func__, skb->dev->name);
>>>>     		goto skip_defrtr;
>>>>     	}
>>>
>>> Hmm, without global knob, I see little benefit by
>>> having new helper.
>>
>> A previous reviewer requested it.  I don't care either
>> way, seems fine to open-code it to me.
>>
>>> At least, it should be called ipv6_chk_addr_ra(),
>>> ipv6_check_ra_saddr(), ipv6_is_nonlocal_ra() or
>>> something else.
>>>
>>> I think we do not need to change debugging output,
>>> or we could say "RA from local address detected;
>>> default router ignored." or something like.
>>
>> That does seem like a more useful error message.
>>
>> Thanks,
>> Ben
>>
> 
> --yoshfuji
> --
> 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: [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses.

[Hannes Frederic Sowa]

Hello!

On Do, 2014-06-26 at 09:49 +0900, YOSHIFUJI Hideaki/吉藤英明 wrote:
> >> (2014/06/25 6:14), 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
> >>
> >>                                     standard compliant behavior?
> >
> > I've no idea.  Can you point me to the proper standard (and
> > pertinent section)?
> 
> I was wrong.
> 
> I found this code was added by commit 9f56220 ("ipv6: Do not
> use routes from locally generated RAs") to fix behavior when
> accept_ra == 2.
> 
> But I do not understand why it is not enough to restrict local
> address on receiving interface.
> 
> Andi, would you explain?

Wouldn't we alter existing behaviour in case someone connects several
NICs in a router to the same network while only running radvd on one
interface. Additional addresses would show up where prior none would
have.

I am in favor of checking all addresses in the current namespace.

Greetings,
Hannes