Message ID | 1479111388-25383-1-git-send-email-liuhangbin@gmail.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Hangbin Liu <liuhangbin@gmail.com> Date: Mon, 14 Nov 2016 16:16:28 +0800 > In commit 24cf3af3fed5 ("igmp: call ip_mc_clear_src..."), we forgot to remove > igmpv3_clear_delrec() in ip_mc_down(), which also called ip_mc_clear_src(). > This make us clear all IGMPv3 source filter info after NETDEV_DOWN. > Move igmpv3_clear_delrec() to ip_mc_destroy_dev() and then no need > ip_mc_clear_src() in ip_mc_destroy_dev(). > > On the other hand, we should restore back instead of free all source filter > info in igmpv3_del_delrec(). Or we will not able to restore IGMPv3 source > filter info after NETDEV_UP and NETDEV_POST_TYPE_CHANGE. > > Fixes: 24cf3af3fed5 ("igmp: call ip_mc_clear_src() only when ...") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> Applied.
Hi Liu, I saw that you have sent patches to the list for IGMP and have a question on IGMP on IPv6. Hope you can clarify. I have posted the question already to the list and is reproduced below. Let me know if you have an answer. ========= See email with subject "IPv6 IGMP issue in v4.4.44 ?? ======================== Cut-n-paste from that email.... I see an issue with IGMP for IPv6 when I test HSR redundancy network interface. As soon as I set up an HSR interface, I see some IGMP messages (destination mac address: 33 33 00 00 00 02 going over HSR interface to slave interfaces, at the egress where as for IPv6, I see similar messages going directly over the Ethernet interfaces that are attached to HSR master. It appears that the NETDEV_CHANGEUPPER is not handled properly and the mcast snoop sends the packets over the old interfaces at timer expiry. A dump of the message at the slave Ethernet interface looks like below. IPv4 [ 64.643842] 33 33 00 00 00 02 70 ff 76 1c 0f 8d 89 2f 10 3e fc [ 64.649910] 18 86 dd 60 00 00 00 00 10 3a ff fe 80 00 00 00 [ 64.655705] 00 00 00 72 ff 76 ff fe 1c 0f 8d ff 02 00 00 00 [ 64.661503] 00 00 00 00 00 00 00 00 00 00 02 85 00 8d dc You can see this is tagged with HSR. IPv6 [ 65.559130] 33 33 00 00 00 02 70 ff 76 1c 0f 8d 86 dd 60 00 00 [ 65.565205] 00 00 10 3a ff fe 80 00 00 00 00 00 00 72 ff 76 [ 65.571011] ff fe 1c 0f 8d ff 02 00 00 00 00 00 00 00 00 00 [ 65.576806] 00 00 00 00 02 85 00 8d dc 00 00 00 00 01 01 This is going directly to the slave Ethernet interface. When I put a WARN_ONCE, I found this is coming directly from mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() Do you think this is fixed in latest kernel at master? If so, could you point me to some commits.
On 03/22/2017 11:04 AM, Murali Karicheri wrote: > Hi Liu, > > I saw that you have sent patches to the list for IGMP and have a question on IGMP on IPv6. > Hope you can clarify. I have posted the question already to the list and is reproduced > below. Let me know if you have an answer. > > ========= See email with subject "IPv6 IGMP issue in v4.4.44 ?? ======================== > Cut-n-paste from that email.... > > I see an issue with IGMP for IPv6 when I test HSR redundancy network > interface. As soon as I set up an HSR interface, I see some IGMP messages > (destination mac address: 33 33 00 00 00 02 going over HSR interface to > slave interfaces, at the egress where as for IPv6, I see similar messages > going directly over the Ethernet interfaces that are attached to > HSR master. It appears that the NETDEV_CHANGEUPPER is not handled properly > and the mcast snoop sends the packets over the old interfaces at timer > expiry. > > A dump of the message at the slave Ethernet interface looks like below. > > IPv4 > > [ 64.643842] 33 33 00 00 00 02 70 ff 76 1c 0f 8d 89 2f 10 3e fc > [ 64.649910] 18 86 dd 60 00 00 00 00 10 3a ff fe 80 00 00 00 > [ 64.655705] 00 00 00 72 ff 76 ff fe 1c 0f 8d ff 02 00 00 00 > [ 64.661503] 00 00 00 00 00 00 00 00 00 00 02 85 00 8d dc > > > You can see this is tagged with HSR. > > IPv6 > > [ 65.559130] 33 33 00 00 00 02 70 ff 76 1c 0f 8d 86 dd 60 00 00 > [ 65.565205] 00 00 10 3a ff fe 80 00 00 00 00 00 00 72 ff 76 > [ 65.571011] ff fe 1c 0f 8d ff 02 00 00 00 00 00 00 00 00 00 > [ 65.576806] 00 00 00 00 02 85 00 8d dc 00 00 00 00 01 01 > > This is going directly to the slave Ethernet interface. > > When I put a WARN_ONCE, I found this is coming directly from > mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() > > Do you think this is fixed in latest kernel at master? If so, could > you point me to some commits. > > Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if this is fixed by some commit or I need to debug? I see IGMPv6 has some fixes on the list to make it similar to IGMPv4. So can someone clarify this is is a bug at IGMPv6 code or I need to look into the HSR driver code? Since IGMPv4 is going over the HSR interface I am assuming this is a bug in the IGMPv6 code. But since I have not experience with this code can some expert comment please? Murali
Hello, On Thu, Apr 13, 2017 at 9:36 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: > On 03/22/2017 11:04 AM, Murali Karicheri wrote: >> This is going directly to the slave Ethernet interface. >> >> When I put a WARN_ONCE, I found this is coming directly from >> mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() >> >> Do you think this is fixed in latest kernel at master? If so, could >> you point me to some commits. >> >> > Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if > this is fixed by some commit or I need to debug? I see IGMPv6 has some > fixes on the list to make it similar to IGMPv4. So can someone clarify this is > is a bug at IGMPv6 code or I need to look into the HSR driver code? > Since IGMPv4 is going over the HSR interface I am assuming this is a > bug in the IGMPv6 code. But since I have not experience with this code > can some expert comment please? > How did you configure your network interfaces and IPv4/IPv6 multicast? IOW, how did you reproduce this? For example, did you change your HSR setup when this happened since you mentioned NETDEV_CHANGEUPPER?
On 04/17/2017 05:38 PM, Cong Wang wrote: > Hello, > > On Thu, Apr 13, 2017 at 9:36 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >> On 03/22/2017 11:04 AM, Murali Karicheri wrote: >>> This is going directly to the slave Ethernet interface. >>> >>> When I put a WARN_ONCE, I found this is coming directly from >>> mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() >>> >>> Do you think this is fixed in latest kernel at master? If so, could >>> you point me to some commits. >>> >>> >> Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if >> this is fixed by some commit or I need to debug? I see IGMPv6 has some >> fixes on the list to make it similar to IGMPv4. So can someone clarify this is >> is a bug at IGMPv6 code or I need to look into the HSR driver code? >> Since IGMPv4 is going over the HSR interface I am assuming this is a >> bug in the IGMPv6 code. But since I have not experience with this code >> can some expert comment please? >> > > How did you configure your network interfaces and IPv4/IPv6 multicast? > IOW, how did you reproduce this? For example, did you change your > HSR setup when this happened since you mentioned > NETDEV_CHANGEUPPER? > Thanks for responding! Really appreciate. I didn't set up anything explicitly for IPv4/IPv6 multicast. As part of my testing, I dump the packets going through the slave interfaces attached to the hsr interface (for example my Ethernet interfaces eth2 and eth3 are attached to the hsr interface and I dump the packets at the egress of eth2 and eth3 in my driver along with that at hsr xmit function). As soon as I create the hsr interface, I see a bunch of packets going directly through the lower interface, not through the upper one (i.e hsr interface) and these are of eth_type = 86 dd. Please ignore my reference to NETDEV_CHANGEUPPER for now as it was wild guess. I have not done any debugging, but the WARN_ONCE which I have placed in the lower level driver looking for eth_type = 86 dd provided the above trace.
On 04/18/2017 01:12 PM, Murali Karicheri wrote: > On 04/17/2017 05:38 PM, Cong Wang wrote: >> Hello, >> >> On Thu, Apr 13, 2017 at 9:36 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >>> On 03/22/2017 11:04 AM, Murali Karicheri wrote: >>>> This is going directly to the slave Ethernet interface. >>>> >>>> When I put a WARN_ONCE, I found this is coming directly from >>>> mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() >>>> >>>> Do you think this is fixed in latest kernel at master? If so, could >>>> you point me to some commits. >>>> >>>> >>> Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if >>> this is fixed by some commit or I need to debug? I see IGMPv6 has some >>> fixes on the list to make it similar to IGMPv4. So can someone clarify this is >>> is a bug at IGMPv6 code or I need to look into the HSR driver code? >>> Since IGMPv4 is going over the HSR interface I am assuming this is a >>> bug in the IGMPv6 code. But since I have not experience with this code >>> can some expert comment please? >>> >> >> How did you configure your network interfaces and IPv4/IPv6 multicast? >> IOW, how did you reproduce this? For example, did you change your >> HSR setup when this happened since you mentioned >> NETDEV_CHANGEUPPER? >> > Thanks for responding! Really appreciate. > > I didn't set up anything explicitly for IPv4/IPv6 multicast. As part of > my testing, I dump the packets going through the slave interfaces attached > to the hsr interface (for example my Ethernet interfaces eth2 and eth3 > are attached to the hsr interface and I dump the packets at the egress > of eth2 and eth3 in my driver along with that at hsr xmit function). As > soon as I create the hsr interface, I see a bunch of packets going directly > through the lower interface, not through the upper one (i.e hsr interface) > and these are of eth_type = 86 dd. Please ignore my reference to > NETDEV_CHANGEUPPER for now as it was wild guess. > > I have not done any debugging, but the WARN_ONCE which I have placed > in the lower level driver looking for eth_type = 86 dd provided the > above trace. > Here is the command I have used to create the hsr interface... ip link add name hsr0 type hsr slave1 eth2 slave2 eth3 supervision 45 version 1
On Tue, Apr 18, 2017 at 10:20 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: > On 04/18/2017 01:12 PM, Murali Karicheri wrote: >> On 04/17/2017 05:38 PM, Cong Wang wrote: >>> Hello, >>> >>> On Thu, Apr 13, 2017 at 9:36 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >>>> On 03/22/2017 11:04 AM, Murali Karicheri wrote: >>>>> This is going directly to the slave Ethernet interface. >>>>> >>>>> When I put a WARN_ONCE, I found this is coming directly from >>>>> mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() >>>>> >>>>> Do you think this is fixed in latest kernel at master? If so, could >>>>> you point me to some commits. >>>>> >>>>> >>>> Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if >>>> this is fixed by some commit or I need to debug? I see IGMPv6 has some >>>> fixes on the list to make it similar to IGMPv4. So can someone clarify this is >>>> is a bug at IGMPv6 code or I need to look into the HSR driver code? >>>> Since IGMPv4 is going over the HSR interface I am assuming this is a >>>> bug in the IGMPv6 code. But since I have not experience with this code >>>> can some expert comment please? >>>> >>> >>> How did you configure your network interfaces and IPv4/IPv6 multicast? >>> IOW, how did you reproduce this? For example, did you change your >>> HSR setup when this happened since you mentioned >>> NETDEV_CHANGEUPPER? >>> >> Thanks for responding! Really appreciate. >> >> I didn't set up anything explicitly for IPv4/IPv6 multicast. As part of >> my testing, I dump the packets going through the slave interfaces attached >> to the hsr interface (for example my Ethernet interfaces eth2 and eth3 >> are attached to the hsr interface and I dump the packets at the egress >> of eth2 and eth3 in my driver along with that at hsr xmit function). As >> soon as I create the hsr interface, I see a bunch of packets going directly >> through the lower interface, not through the upper one (i.e hsr interface) >> and these are of eth_type = 86 dd. Please ignore my reference to >> NETDEV_CHANGEUPPER for now as it was wild guess. OK. Note: I know nothing about HSR, I assume it is similar to bonding in your case? >> >> I have not done any debugging, but the WARN_ONCE which I have placed >> in the lower level driver looking for eth_type = 86 dd provided the >> above trace. >> > Here is the command I have used to create the hsr interface... > > ip link add name hsr0 type hsr slave1 eth2 slave2 eth3 supervision 45 version 1 Did you assign IPv4 and IPv6 addresses to the HSR master device?
On 04/18/2017 06:37 PM, Cong Wang wrote: > On Tue, Apr 18, 2017 at 10:20 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >> On 04/18/2017 01:12 PM, Murali Karicheri wrote: >>> On 04/17/2017 05:38 PM, Cong Wang wrote: >>>> Hello, >>>> >>>> On Thu, Apr 13, 2017 at 9:36 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >>>>> On 03/22/2017 11:04 AM, Murali Karicheri wrote: >>>>>> This is going directly to the slave Ethernet interface. >>>>>> >>>>>> When I put a WARN_ONCE, I found this is coming directly from >>>>>> mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() >>>>>> >>>>>> Do you think this is fixed in latest kernel at master? If so, could >>>>>> you point me to some commits. >>>>>> >>>>>> >>>>> Ping... I see this behavior is also seen on v4.9.x Kernel. Any clue if >>>>> this is fixed by some commit or I need to debug? I see IGMPv6 has some >>>>> fixes on the list to make it similar to IGMPv4. So can someone clarify this is >>>>> is a bug at IGMPv6 code or I need to look into the HSR driver code? >>>>> Since IGMPv4 is going over the HSR interface I am assuming this is a >>>>> bug in the IGMPv6 code. But since I have not experience with this code >>>>> can some expert comment please? >>>>> >>>> >>>> How did you configure your network interfaces and IPv4/IPv6 multicast? >>>> IOW, how did you reproduce this? For example, did you change your >>>> HSR setup when this happened since you mentioned >>>> NETDEV_CHANGEUPPER? >>>> >>> Thanks for responding! Really appreciate. >>> >>> I didn't set up anything explicitly for IPv4/IPv6 multicast. As part of >>> my testing, I dump the packets going through the slave interfaces attached >>> to the hsr interface (for example my Ethernet interfaces eth2 and eth3 >>> are attached to the hsr interface and I dump the packets at the egress >>> of eth2 and eth3 in my driver along with that at hsr xmit function). As >>> soon as I create the hsr interface, I see a bunch of packets going directly >>> through the lower interface, not through the upper one (i.e hsr interface) >>> and these are of eth_type = 86 dd. Please ignore my reference to >>> NETDEV_CHANGEUPPER for now as it was wild guess. > > OK. Note: I know nothing about HSR, I assume it is similar to bonding > in your case? > Similar in the sense it glues two standard Ethernet interfaces and run HSR protocol frames over it to support redundancy. >>> >>> I have not done any debugging, but the WARN_ONCE which I have placed >>> in the lower level driver looking for eth_type = 86 dd provided the >>> above trace. >>> >> Here is the command I have used to create the hsr interface... >> >> ip link add name hsr0 type hsr slave1 eth2 slave2 eth3 supervision 45 version 1 > > Did you assign IPv4 and IPv6 addresses to the HSR master device? No. I just used IPv4. From the trace mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() do you know what is it trying to do? Is it some neighbor discovery message or something going over the lower interface instead of the hsr interface? Murali >
On Tue, Apr 25, 2017 at 9:16 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: > On 04/18/2017 06:37 PM, Cong Wang wrote: >> Did you assign IPv4 and IPv6 addresses to the HSR master device? > > No. I just used IPv4. From the trace mld_ifc_timer_expire() -> mld_sendpack() -> ip6_output() > do you know what is it trying to do? Is it some neighbor discovery message or something > going over the lower interface instead of the hsr interface? > IPv6 is special here because it has link-local address configured automatically for each device, so this mld_ifc_timer is armed for link-local multicasts. See: 464 /* Join interface-local all-node multicast group */ 465 ipv6_dev_mc_inc(dev, &in6addr_interfacelocal_allnodes); 466 467 /* Join all-node multicast group */ 468 ipv6_dev_mc_inc(dev, &in6addr_linklocal_allnodes); 469 470 /* Join all-router multicast group if forwarding is set */ 471 if (ndev->cnf.forwarding && (dev->flags & IFF_MULTICAST)) 472 ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c index 606cc3e..15db786 100644 --- a/net/ipv4/igmp.c +++ b/net/ipv4/igmp.c @@ -162,7 +162,7 @@ static int unsolicited_report_interval(struct in_device *in_dev) } static void igmpv3_add_delrec(struct in_device *in_dev, struct ip_mc_list *im); -static void igmpv3_del_delrec(struct in_device *in_dev, __be32 multiaddr); +static void igmpv3_del_delrec(struct in_device *in_dev, struct ip_mc_list *im); static void igmpv3_clear_delrec(struct in_device *in_dev); static int sf_setstate(struct ip_mc_list *pmc); static void sf_markstate(struct ip_mc_list *pmc); @@ -1130,10 +1130,15 @@ static void igmpv3_add_delrec(struct in_device *in_dev, struct ip_mc_list *im) spin_unlock_bh(&in_dev->mc_tomb_lock); } -static void igmpv3_del_delrec(struct in_device *in_dev, __be32 multiaddr) +/* + * restore ip_mc_list deleted records + */ +static void igmpv3_del_delrec(struct in_device *in_dev, struct ip_mc_list *im) { struct ip_mc_list *pmc, *pmc_prev; - struct ip_sf_list *psf, *psf_next; + struct ip_sf_list *psf; + struct net *net = dev_net(in_dev->dev); + __be32 multiaddr = im->multiaddr; spin_lock_bh(&in_dev->mc_tomb_lock); pmc_prev = NULL; @@ -1149,16 +1154,26 @@ static void igmpv3_del_delrec(struct in_device *in_dev, __be32 multiaddr) in_dev->mc_tomb = pmc->next; } spin_unlock_bh(&in_dev->mc_tomb_lock); + + spin_lock_bh(&im->lock); if (pmc) { - for (psf = pmc->tomb; psf; psf = psf_next) { - psf_next = psf->sf_next; - kfree(psf); + im->interface = pmc->interface; + im->crcount = in_dev->mr_qrv ?: net->ipv4.sysctl_igmp_qrv; + im->sfmode = pmc->sfmode; + if (pmc->sfmode == MCAST_INCLUDE) { + im->tomb = pmc->tomb; + im->sources = pmc->sources; + for (psf = im->sources; psf; psf = psf->sf_next) + psf->sf_crcount = im->crcount; } in_dev_put(pmc->interface); - kfree(pmc); } + spin_unlock_bh(&im->lock); } +/* + * flush ip_mc_list deleted records + */ static void igmpv3_clear_delrec(struct in_device *in_dev) { struct ip_mc_list *pmc, *nextpmc; @@ -1366,7 +1381,7 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr) ip_mc_hash_add(in_dev, im); #ifdef CONFIG_IP_MULTICAST - igmpv3_del_delrec(in_dev, im->multiaddr); + igmpv3_del_delrec(in_dev, im); #endif igmp_group_added(im); if (!in_dev->dead) @@ -1626,8 +1641,12 @@ void ip_mc_remap(struct in_device *in_dev) ASSERT_RTNL(); - for_each_pmc_rtnl(in_dev, pmc) + for_each_pmc_rtnl(in_dev, pmc) { +#ifdef CONFIG_IP_MULTICAST + igmpv3_del_delrec(in_dev, pmc); +#endif igmp_group_added(pmc); + } } /* Device going down */ @@ -1648,7 +1667,6 @@ void ip_mc_down(struct in_device *in_dev) in_dev->mr_gq_running = 0; if (del_timer(&in_dev->mr_gq_timer)) __in_dev_put(in_dev); - igmpv3_clear_delrec(in_dev); #endif ip_mc_dec_group(in_dev, IGMP_ALL_HOSTS); @@ -1688,8 +1706,12 @@ void ip_mc_up(struct in_device *in_dev) #endif ip_mc_inc_group(in_dev, IGMP_ALL_HOSTS); - for_each_pmc_rtnl(in_dev, pmc) + for_each_pmc_rtnl(in_dev, pmc) { +#ifdef CONFIG_IP_MULTICAST + igmpv3_del_delrec(in_dev, pmc); +#endif igmp_group_added(pmc); + } } /* @@ -1704,13 +1726,13 @@ void ip_mc_destroy_dev(struct in_device *in_dev) /* Deactivate timers */ ip_mc_down(in_dev); +#ifdef CONFIG_IP_MULTICAST + igmpv3_clear_delrec(in_dev); +#endif while ((i = rtnl_dereference(in_dev->mc_list)) != NULL) { in_dev->mc_list = i->next_rcu; in_dev->mc_count--; - - /* We've dropped the groups in ip_mc_down already */ - ip_mc_clear_src(i); ip_ma_put(i); } }
In commit 24cf3af3fed5 ("igmp: call ip_mc_clear_src..."), we forgot to remove igmpv3_clear_delrec() in ip_mc_down(), which also called ip_mc_clear_src(). This make us clear all IGMPv3 source filter info after NETDEV_DOWN. Move igmpv3_clear_delrec() to ip_mc_destroy_dev() and then no need ip_mc_clear_src() in ip_mc_destroy_dev(). On the other hand, we should restore back instead of free all source filter info in igmpv3_del_delrec(). Or we will not able to restore IGMPv3 source filter info after NETDEV_UP and NETDEV_POST_TYPE_CHANGE. Fixes: 24cf3af3fed5 ("igmp: call ip_mc_clear_src() only when ...") Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- net/ipv4/igmp.c | 50 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 14 deletions(-)