lkml.org 
[lkml]   [2011]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 1/1] igmp: call ip_mc_clear_src() only when we have no users of ip_mc_list
On Tue, May 17, 2011 at 10:42:59AM -0700, David Stevens wrote:
> Veaceslav,
> It looks to me like this will leak the source filters if we are
> called from ip_mc_destroy_dev(),
> Even with your previous patch, you're assuming that we don't free the
> ip_mc_list and so we have the
> same one when we up the device, but if there are no timers running, it
> looks like refcnt canl go to 0 and free
> it. If we can ever free the ip_mc_list when users != 0 (or going to 0
> immediately after the drop), we
> have to do the ip_mc_clear_src() or leak the list. I haven't looked at
> this code in years, so I'll need
> to refresh my memory.
> So, I'll look at that a bit more; at a minimum, I think you need
> to do the clear_src
> also in the destroy case. We could lose the filters and set the exclude
> count to users, instead
> of 1; but I like the idea of keeping the source filters across a down/up,
> if we can be sure there
> are no cases where we free the ip_mc_list without first freeing all the
> filters.
>
> +-DLS

Yes, you are completely right, we can leak the sources on
ip_mc_destroy_dev() when we've ip_ma_put() it inside all the timers. Also,
I've seen that we called igmp_group_dropped() for every mc in dev->mc_list,
however we've done it already in ip_mc_down() before, which wouldn't lead
to anything (cause the device is already ->dead, and all timers are
stopped), but just would be a waste of time.

So, does this patch seem ok? If yes, I'll send it with the changelog.

---
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 1fd3d9c..57ca93a 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1169,20 +1169,18 @@ static void igmp_group_dropped(struct ip_mc_list *im)

if (!in_dev->dead) {
if (IGMP_V1_SEEN(in_dev))
- goto done;
+ return;
if (IGMP_V2_SEEN(in_dev)) {
if (reporter)
igmp_send_report(in_dev, im, IGMP_HOST_LEAVE_MESSAGE);
- goto done;
+ return;
}
/* IGMPv3 */
igmpv3_add_delrec(in_dev, im);

igmp_ifc_event(in_dev);
}
-done:
#endif
- ip_mc_clear_src(im);
}

static void igmp_group_added(struct ip_mc_list *im)
@@ -1319,6 +1317,7 @@ void ip_mc_dec_group(struct in_device *in_dev, __be32 addr)
*ip = i->next_rcu;
in_dev->mc_count--;
igmp_group_dropped(i);
+ ip_mc_clear_src(i);

if (!in_dev->dead)
ip_rt_multicast_event(in_dev);
@@ -1428,7 +1427,8 @@ void ip_mc_destroy_dev(struct in_device *in_dev)
in_dev->mc_list = i->next_rcu;
in_dev->mc_count--;

- igmp_group_dropped(i);
+ /* We've dropped the groups in ip_mc_down already */
+ ip_mc_clear_src(i);
ip_ma_put(i);
}
}

\
 
 \ /
  Last update: 2011-05-20 18:31    [W:0.059 / U:0.584 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site