Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581752 > unrolled thread

[PATCH 3.2 086/126] igmp: do not remove igmp souce list info when set link down

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3.2 086/126] igmp: do not remove igmp souce list info when  set link down Ben Hutchings <ben@decadent.org.uk> - 2017-02-16 00:00 +0100
    Re: [PATCH 3.2 086/126] igmp: do not remove igmp souce list info when  set link down Hangbin Liu <liuhangbin@gmail.com> - 2017-02-16 02:50 +0100
      Re: [PATCH 3.2 086/126] igmp: do not remove igmp souce list info  when set link down Ben Hutchings <ben@decadent.org.uk> - 2017-02-16 17:10 +0100

#1581752 — [PATCH 3.2 086/126] igmp: do not remove igmp souce list info when set link down

3.2.85-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Hangbin Liu <liuhangbin@gmail.com>

commit 24803f38a5c0b6c57ed800b47e695f9ce474bc3a upstream.

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>
Signed-off-by: David S. Miller <davem@davemloft.net>
[bwh: Backported to 3.2:
 - Use IGMP_Unsolicited_Report_Count instead of sysctl_igmp_qrv
 - Adjust context]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -140,7 +140,7 @@
 	  time_before(jiffies, (in_dev)->mr_v2_seen)))
 
 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);
@@ -1077,10 +1077,14 @@ static void igmpv3_add_delrec(struct in_
 	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;
+	__be32 multiaddr = im->multiaddr;
 
 	spin_lock_bh(&in_dev->mc_tomb_lock);
 	pmc_prev = NULL;
@@ -1096,16 +1100,26 @@ static void igmpv3_del_delrec(struct in_
 			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 ?: IGMP_Unsolicited_Report_Count;
+		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;
@@ -1250,7 +1264,7 @@ void ip_mc_inc_group(struct in_device *i
 	rcu_assign_pointer(in_dev->mc_list, 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)
@@ -1340,8 +1354,12 @@ void ip_mc_remap(struct in_device *in_de
 
 	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 */
@@ -1362,7 +1380,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);
@@ -1397,8 +1414,12 @@ void ip_mc_up(struct in_device *in_dev)
 
 	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);
+	}
 }
 
 /*
@@ -1413,13 +1434,13 @@ void ip_mc_destroy_dev(struct in_device
 
 	/* 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);
 	}
 }

[toc] | [next] | [standalone]

#1582221

2017-02-16 6:41 GMT+08:00 Ben Hutchings <ben@decadent.org.uk>:
> 3.2.85-rc1 review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: Hangbin Liu <liuhangbin@gmail.com>
>
> commit 24803f38a5c0b6c57ed800b47e695f9ce474bc3a upstream.
>
> 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>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: Backported to 3.2:
>  - Use IGMP_Unsolicited_Report_Count instead of sysctl_igmp_qrv
>  - Adjust context]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Hi Ben,

There is a bug fix for this patch, please consider drop this patch or
include the fix.

commit 9c8bb163ae784be4f79ae504e78c862806087c54
Author: Hangbin Liu <liuhangbin@gmail.com>
Date:   Wed Feb 8 21:16:45 2017 +0800

    igmp, mld: Fix memory leak in igmpv3/mld_del_delrec()

    In function igmpv3/mld_add_delrec() we allocate pmc and put it in
    idev->mc_tomb, so we should free it when we don't need it in del_delrec().
    But I removed kfree(pmc) incorrectly in latest two patches. Now fix it.

    Fixes: 24803f38a5c0 ("igmp: do not remove igmp souce list info when ...")
    Fixes: 1666d49e1d41 ("mld: do not remove mld souce list info when ...")
    Reported-by: Daniel Borkmann <daniel@iogearbox.net>
    Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Thanks
Hangbin

[toc] | [prev] | [next] | [standalone]

#1582662 — Re: [PATCH 3.2 086/126] igmp: do not remove igmp souce list info when set link down

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2017-02-16 at 09:42 +0800, Hangbin Liu wrote:
> > 2017-02-16 6:41 GMT+08:00 Ben Hutchings <ben@decadent.org.uk>:
> > 3.2.85-rc1 review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Hangbin Liu <liuhangbin@gmail.com>
> > 
> > commit 24803f38a5c0b6c57ed800b47e695f9ce474bc3a upstream.
> > 
> > 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>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > [bwh: Backported to 3.2:
> >  - Use IGMP_Unsolicited_Report_Count instead of sysctl_igmp_qrv
> >  - Adjust context]
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> Hi Ben,
> 
> There is a bug fix for this patch, please consider drop this patch or
> include the fix.
> 
> commit 9c8bb163ae784be4f79ae504e78c862806087c54
> Author: Hangbin Liu <liuhangbin@gmail.com>
> Date:   Wed Feb 8 21:16:45 2017 +0800
> 
>     igmp, mld: Fix memory leak in igmpv3/mld_del_delrec()
[...]

Thanks.  As the second fix is quite recent, I'm going to drop this
patch for now (from both 3.2 and 3.16) and consider applying both of
them in later updates.

Ben.

-- 
Ben Hutchings
The most exhausting thing in life is being insincere. - Anne Morrow
Lindberg

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web