* [PATCH v2 1/2] ipv6: Add more debugging around accept-ra logic. @ 2014-06-24 21:14 greearb 2014-06-24 21:14 ` [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses greearb 0 siblings, 1 reply; 8+ messages in thread From: greearb @ 2014-06-24 21:14 UTC (permalink / raw) To: netdev; +Cc: Ben Greear 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses. 2014-06-24 21:14 [PATCH v2 1/2] ipv6: Add more debugging around accept-ra logic greearb @ 2014-06-24 21:14 ` greearb 2014-06-24 22:22 ` YOSHIFUJI Hideaki 0 siblings, 1 reply; 8+ messages in thread From: greearb @ 2014-06-24 21:14 UTC (permalink / raw) To: netdev; +Cc: Ben Greear 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses. 2014-06-24 21:14 ` [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses greearb @ 2014-06-24 22:22 ` YOSHIFUJI Hideaki 2014-06-25 3:19 ` Ben Greear 0 siblings, 1 reply; 8+ messages in thread From: YOSHIFUJI Hideaki @ 2014-06-24 22:22 UTC (permalink / raw) To: greearb, netdev; +Cc: YOSHIFUJI Hideaki, hideaki.yoshifuji 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses. 2014-06-24 22:22 ` YOSHIFUJI Hideaki @ 2014-06-25 3:19 ` Ben Greear 2014-06-25 8:48 ` Hannes Frederic Sowa 2014-06-26 0:49 ` YOSHIFUJI Hideaki/吉藤英明 0 siblings, 2 replies; 8+ messages in thread From: Ben Greear @ 2014-06-25 3:19 UTC (permalink / raw) To: YOSHIFUJI Hideaki, netdev; +Cc: YOSHIFUJI Hideaki, hideaki.yoshifuji 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses. 2014-06-25 3:19 ` Ben Greear @ 2014-06-25 8:48 ` Hannes Frederic Sowa 2014-06-26 0:49 ` YOSHIFUJI Hideaki/吉藤英明 1 sibling, 0 replies; 8+ messages in thread From: Hannes Frederic Sowa @ 2014-06-25 8:48 UTC (permalink / raw) To: Ben Greear Cc: YOSHIFUJI Hideaki, netdev, YOSHIFUJI Hideaki, hideaki.yoshifuji 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses. 2014-06-25 3:19 ` Ben Greear 2014-06-25 8:48 ` Hannes Frederic Sowa @ 2014-06-26 0:49 ` YOSHIFUJI Hideaki/吉藤英明 2014-06-26 0:57 ` David Miller 2014-06-27 6:24 ` Hannes Frederic Sowa 1 sibling, 2 replies; 8+ messages in thread From: YOSHIFUJI Hideaki/吉藤英明 @ 2014-06-26 0:49 UTC (permalink / raw) To: Ben Greear, YOSHIFUJI Hideaki, netdev Cc: hideaki.yoshifuji, 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses. 2014-06-26 0:49 ` YOSHIFUJI Hideaki/吉藤英明 @ 2014-06-26 0:57 ` David Miller 2014-06-27 6:24 ` Hannes Frederic Sowa 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2014-06-26 0:57 UTC (permalink / raw) To: hideaki.yoshifuji; +Cc: greearb, hideaki, netdev, yoshfuji, andi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses. 2014-06-26 0:49 ` YOSHIFUJI Hideaki/吉藤英明 2014-06-26 0:57 ` David Miller @ 2014-06-27 6:24 ` Hannes Frederic Sowa 1 sibling, 0 replies; 8+ messages in thread From: Hannes Frederic Sowa @ 2014-06-27 6:24 UTC (permalink / raw) To: YOSHIFUJI Hideaki/吉藤英明 Cc: Ben Greear, YOSHIFUJI Hideaki, netdev, YOSHIFUJI Hideaki 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-06-27 6:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-24 21:14 [PATCH v2 1/2] ipv6: Add more debugging around accept-ra logic greearb 2014-06-24 21:14 ` [PATCH v2 2/2] ipv6: Allow accepting RA from local IP addresses greearb 2014-06-24 22:22 ` YOSHIFUJI Hideaki 2014-06-25 3:19 ` Ben Greear 2014-06-25 8:48 ` Hannes Frederic Sowa 2014-06-26 0:49 ` YOSHIFUJI Hideaki/吉藤英明 2014-06-26 0:57 ` David Miller 2014-06-27 6:24 ` Hannes Frederic Sowa
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.