| Message ID | 20130521150616.GA8513@obelix.rh |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, May 21, 2013 at 12:06:16PM -0300, Flavio Leitner wrote: > On Sat, May 18, 2013 at 07:31:05PM +0200, Hannes Frederic Sowa wrote: > > On Fri, May 17, 2013 at 12:24:49AM -0300, Flavio Leitner wrote: > > > A tcpdump captured while adding an IPv6 link-local address shows > > > two MLD reports. One with source ``::'' and another with the permanent > > > address. > > > > > > Well, if you increase dad_retransmits from 1 to 10, for instance, > > > then both MLD reports are sent with source address ``::'' which > > > according with specs should be ignored by the routers. > > > > > > [...] > > > > > > Therefore, I believe this is a bug in IPv6 MLD because it should > > > sent at least 2 MLD reports after DAD is completed. > > > The specs says: > > > > > > [..] > > > > > > Does that make any sense? > > > > Yes, this is unfortunate. RFC3590 clarifies this also for MLDv1 messages. > > > > Could you try following patch I just came up with? > > It does work, but it would be better to send [Robustness Variable] > times, right? Sure, this is the more robust approach. Patch is tested and seems fine: Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org> I tried to track if we actually used the unspecified address to send out mld reports, but doing this correctly (multiple link-local addresses in dad state) needed a bit of ugly lock handling and the code got bloated. So I do think it is not worth the effort. Thanks, 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
First, I wouldn't call this a "bug"; it is listed as a "SHOULD", not a
"MUST", so
it is not required for compliance, though it is a good feature to add.
Second, I think doing this on all LL addresses unconditionally isn't a
good
idea. If some configuration is using thousands of LL addrs for reasons of
their
own, they would suddenly be blasted with unexpected MLD reports where
they have none now. I don't see why tracking it would be such a problem --
if nothing else, you could simply add a counter for the number of valid LL
addrs
incremented on DAD completion, and send these whenever it transitions from
0 to 1.
+-DLS
--
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
On Wed, May 22, 2013 at 07:30:39AM -0400, David Stevens wrote: > First, I wouldn't call this a "bug"; it is listed as a "SHOULD", not a > "MUST", so > it is not required for compliance, though it is a good feature to add. Ok. > Second, I think doing this on all LL addresses unconditionally isn't a > good > idea. If some configuration is using thousands of LL addrs for reasons of > their > own, they would suddenly be blasted with unexpected MLD reports where > they have none now. I don't see why tracking it would be such a problem -- > if nothing else, you could simply add a counter for the number of valid LL > addrs > incremented on DAD completion, and send these whenever it transitions from > 0 to 1. I understand your concerns and will revisit the patch and check if a dad_completed_counter would simplify the code. Thanks, 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
On Wed, May 22, 2013 at 07:30:39AM -0400, David Stevens wrote: > First, I wouldn't call this a "bug"; it is listed as a "SHOULD", not a > "MUST", so > it is not required for compliance, though it is a good feature to add. > > Second, I think doing this on all LL addresses unconditionally isn't a > good > idea. If some configuration is using thousands of LL addrs for reasons of > their > own, they would suddenly be blasted with unexpected MLD reports where > they have none now. I don't see why tracking it would be such a problem -- > if nothing else, you could simply add a counter for the number of valid LL > addrs > incremented on DAD completion, and send these whenever it transitions from > 0 to 1. It becomes a bug (and that's why I started with 'possible') if dad completes after the two MLD reports sent with ``::'' source, because routers will ignore those initial reports and the system is left out. Thanks,
Flavio Leitner <fbl@redhat.com> wrote on 05/22/2013 01:27:11 PM: > It becomes a bug (and that's why I started with 'possible') if dad > completes after the two MLD reports sent with ``::'' source, because > routers will ignore those initial reports and the system is left out. No, unsolicited reports are not necessary for MLD. If there are no other listeners for a group, the reports will be triggered within one query interval after a valid LL addr is there. +-DLS -- 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
On Wed, May 22, 2013 at 02:24:34PM -0400, David Stevens wrote: > Flavio Leitner <fbl@redhat.com> wrote on 05/22/2013 01:27:11 PM: > > > It becomes a bug (and that's why I started with 'possible') if dad > > completes after the two MLD reports sent with ``::'' source, because > > routers will ignore those initial reports and the system is left out. > > No, unsolicited reports are not necessary for MLD. If there are no > other listeners for a group, the reports will be triggered within > one query interval after a valid LL addr is there. Agreed that they aren't necessary, but I also don't think it's a good solution to wait for the query interval.
On Wed, May 22, 2013 at 07:30:39AM -0400, David Stevens wrote: > Second, I think doing this on all LL addresses unconditionally isn't a > good > idea. If some configuration is using thousands of LL addrs for reasons of > their > own, they would suddenly be blasted with unexpected MLD reports where > they have none now. I don't see why tracking it would be such a problem -- > if nothing else, you could simply add a counter for the number of valid LL > addrs > incremented on DAD completion, and send these whenever it transitions from > 0 to 1. While testing some code, I noticed that we seem to have a similar problem with the generation of router soliciations. We send out MAX_RTR_SOLICITATIONS for each link-local address dad completion. So the tracking could have some more users and I will move it to addrconf.c. I'll hope I cope with all the tiny races I found in my last version. :) Greetings, 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
On Thu, May 23, 2013 at 10:21:56PM +0200, Hannes Frederic Sowa wrote: > On Wed, May 22, 2013 at 07:30:39AM -0400, David Stevens wrote: > > Second, I think doing this on all LL addresses unconditionally isn't a > > good > > idea. If some configuration is using thousands of LL addrs for reasons of > > their > > own, they would suddenly be blasted with unexpected MLD reports where > > they have none now. I don't see why tracking it would be such a problem -- > > if nothing else, you could simply add a counter for the number of valid LL > > addrs > > incremented on DAD completion, and send these whenever it transitions from > > 0 to 1. > > While testing some code, I noticed that we seem to have a similar > problem with the generation of router soliciations. We send out > MAX_RTR_SOLICITATIONS for each link-local address dad completion. > > So the tracking could have some more users and I will move it to > addrconf.c. I'll hope I cope with all the tiny races I found in my last > version. :) Sorry, I could not finish this patch before I got distracted with other things. I had a look at it again yesterday and came up with the following plan: a) split up router advertisment and dad timer b) remove src-argument from ndisc_send_rs and calculate it from the ll addresses of the interface when sending the RS c) introduce ll_dad_completion counter d) rebase the above patch onto this utilising the ll_dad_completion counter Does this plan sound solid? I plan to send out a patch for a) today. Greetings, 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
diff --git a/include/net/addrconf.h b/include/net/addrconf.h index 84a6440..ddd331c 100644 --- a/include/net/addrconf.h +++ b/include/net/addrconf.h @@ -155,6 +155,7 @@ extern bool ipv6_chk_mcast_addr(struct net_device *dev, const struct in6_addr *group, const struct in6_addr *src_addr); +extern void ipv6_mc_dad_complete(struct inet6_dev *idev); /* * identify MLD packets for MLD filter exceptions */ diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h index 100fb8c..5c195b1 100644 --- a/include/net/if_inet6.h +++ b/include/net/if_inet6.h @@ -172,10 +172,12 @@ struct inet6_dev { unsigned char mc_qrv; unsigned char mc_gq_running; unsigned char mc_ifc_count; + unsigned char mc_dad_count; unsigned long mc_v1_seen; unsigned long mc_maxdelay; struct timer_list mc_gq_timer; /* general query timer */ struct timer_list mc_ifc_timer; /* interface change timer */ + struct timer_list mc_dad_timer; /* dad complete mc timer */ struct ifacaddr6 *ac_list; rwlock_t lock; diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index d1ab6ab..800222b 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -3272,6 +3272,7 @@ out: static void addrconf_dad_completed(struct inet6_ifaddr *ifp) { struct net_device *dev = ifp->idev->dev; + int type = ipv6_addr_type(&ifp->addr); /* * Configure the address for reception. Now it is valid. @@ -3279,6 +3280,12 @@ static void addrconf_dad_completed(struct inet6_ifaddr *ifp) ipv6_ifa_notify(RTM_NEWADDR, ifp); + /* While dad is in progress mld report's source address is in6_addrany. + * Resend with proper ll now. + */ + if (type & IPV6_ADDR_LINKLOCAL) + ipv6_mc_dad_complete(ifp->idev); + /* If added prefix is link local and we are prepared to process router advertisements, start sending router solicitations. */ @@ -3286,7 +3293,7 @@ static void addrconf_dad_completed(struct inet6_ifaddr *ifp) if (ipv6_accept_ra(ifp->idev) && ifp->idev->cnf.rtr_solicits > 0 && (dev->flags&IFF_LOOPBACK) == 0 && - (ipv6_addr_type(&ifp->addr) & IPV6_ADDR_LINKLOCAL)) { + (type & IPV6_ADDR_LINKLOCAL)) { /* * If a host as already performed a random delay * [...] as part of DAD [...] there is no need diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c index bfa6cc3..9941ff7 100644 --- a/net/ipv6/mcast.c +++ b/net/ipv6/mcast.c @@ -999,6 +999,14 @@ static void mld_ifc_start_timer(struct inet6_dev *idev, int delay) in6_dev_hold(idev); } +static void mld_dad_start_timer(struct inet6_dev *idev, int delay) +{ + int tv = net_random() % delay; + + if (!mod_timer(&idev->mc_dad_timer, jiffies+tv+2)) + in6_dev_hold(idev); +} + /* * IGMP handling (alias multicast ICMPv6 messages) */ @@ -1814,6 +1822,41 @@ err_out: goto out; } +void ipv6_mc_dad_complete(struct inet6_dev *idev) +{ + idev->mc_dad_count = idev->mc_qrv; + mld_dad_start_timer(idev, 1); +} + +static void mld_resend_report(struct inet6_dev *idev) +{ + if (MLD_V1_SEEN(idev)) { + struct ifmcaddr6 *mcaddr; + read_lock_bh(&idev->lock); + for (mcaddr = idev->mc_list; mcaddr; mcaddr = mcaddr->next) { + if (!(mcaddr->mca_flags & MAF_NOREPORT)) + igmp6_send(&mcaddr->mca_addr, idev->dev, + ICMPV6_MGM_REPORT); + } + read_unlock_bh(&idev->lock); + } else { + mld_send_report(idev, NULL); + } +} + +static void mld_dad_timer_expire(unsigned long data) +{ + struct inet6_dev *idev = (struct inet6_dev *)data; + + mld_resend_report(idev); + if (idev->mc_dad_count) { + idev->mc_dad_count--; + if (idev->mc_dad_count) + mld_dad_start_timer(idev, idev->mc_maxdelay); + } + __in6_dev_put(idev); +} + static int ip6_mc_del1_src(struct ifmcaddr6 *pmc, int sfmode, const struct in6_addr *psfsrc) { @@ -2231,6 +2274,8 @@ void ipv6_mc_down(struct inet6_dev *idev) idev->mc_gq_running = 0; if (del_timer(&idev->mc_gq_timer)) __in6_dev_put(idev); + if (del_timer(&idev->mc_dad_timer)) + __in6_dev_put(idev); for (i = idev->mc_list; i; i=i->next) igmp6_group_dropped(i); @@ -2267,6 +2312,8 @@ void ipv6_mc_init_dev(struct inet6_dev *idev) idev->mc_ifc_count = 0; setup_timer(&idev->mc_ifc_timer, mld_ifc_timer_expire, (unsigned long)idev); + setup_timer(&idev->mc_dad_timer, mld_dad_timer_expire, + (unsigned long)idev); idev->mc_qrv = MLD_QRV_DEFAULT; idev->mc_maxdelay = IGMP6_UNSOLICITED_IVAL; idev->mc_v1_seen = 0;