Re: Deadlock problem with dev->refcnt somewhere in netlink/multicast... [PATCH]

Re: Deadlock problem with dev->refcnt somewhere in netlink/multicast... [PATCH]

[David Stevens]

[-- Attachment #1.1: Type: text/plain, Size: 2209 bytes --]





Below is a patch for the reference count problem you ran into.

The problem is that the IGMP code was using "IFF_UP" to determine
if a report should be sent or a timer should be started. But it is not
necessarily cleared during a destroy_dev.

The IPv4 code was also missing an ip_mc_down() call added in the
IPv6 code for a similar case by Jan Oravec.

The patch is for 2.4.x but also applies to 2.6.x. In-line for
whitespace-mangled
viewing and attached for applying.

Thanks for reporting the problem, Willy, and let me know if you have any
problems with the patch.
                              +-DLS

diff -ruN linux-2.4.25-pre8/net/ipv4/igmp.c linux-2.4.25-pre8F2/net/ipv4/igmp.c
--- linux-2.4.25-pre8/net/ipv4/igmp.c     2004-01-29 16:21:28.000000000 -0800
+++ linux-2.4.25-pre8F2/net/ipv4/igmp.c   2004-02-02 14:43:40.000000000 -0800
@@ -1051,7 +1051,7 @@
      reporter = im->reporter;
      igmp_stop_timer(im);

-     if (in_dev->dev->flags & IFF_UP) {
+     if (!in_dev->dead) {
            if (IGMP_V1_SEEN(in_dev))
                  goto done;
            if (IGMP_V2_SEEN(in_dev)) {
@@ -1082,6 +1082,8 @@
      if (im->multiaddr == IGMP_ALL_HOSTS)
            return;

+     if (in_dev->dead)
+           return;
      if (IGMP_V1_SEEN(in_dev) || IGMP_V2_SEEN(in_dev)) {
            spin_lock_bh(&im->lock);
            igmp_start_timer(im, IGMP_Initial_Report_Delay);
@@ -1155,7 +1157,7 @@
      igmpv3_del_delrec(in_dev, im->multiaddr);
 #endif
      igmp_group_added(im);
-     if (in_dev->dev->flags & IFF_UP)
+     if (!in_dev->dead)
            ip_rt_multicast_event(in_dev);
 out:
      return;
@@ -1179,7 +1181,7 @@
                        write_unlock_bh(&in_dev->lock);
                        igmp_group_dropped(i);

-                       if (in_dev->dev->flags & IFF_UP)
+                       if (!in_dev->dead)
                              ip_rt_multicast_event(in_dev);

                        ip_ma_put(i);
@@ -1255,6 +1257,9 @@

      ASSERT_RTNL();

+     /* Deactivate timers */
+     ip_mc_down(in_dev);
+
      write_lock_bh(&in_dev->lock);
      while ((i = in_dev->mc_list) != NULL) {
            in_dev->mc_list = i->next;

(See attached file: 2.4igmpref.patch)

[-- Attachment #1.2: Type: text/html, Size: 2305 bytes --]

[-- Attachment #2: 2.4igmpref.patch --]
[-- Type: application/octet-stream, Size: 1288 bytes --]

diff -ruN linux-2.4.25-pre8/net/ipv4/igmp.c linux-2.4.25-pre8F2/net/ipv4/igmp.c
--- linux-2.4.25-pre8/net/ipv4/igmp.c	2004-01-29 16:21:28.000000000 -0800
+++ linux-2.4.25-pre8F2/net/ipv4/igmp.c	2004-02-02 14:43:40.000000000 -0800
@@ -1051,7 +1051,7 @@
 	reporter = im->reporter;
 	igmp_stop_timer(im);
 
-	if (in_dev->dev->flags & IFF_UP) {
+	if (!in_dev->dead) {
 		if (IGMP_V1_SEEN(in_dev))
 			goto done;
 		if (IGMP_V2_SEEN(in_dev)) {
@@ -1082,6 +1082,8 @@
 	if (im->multiaddr == IGMP_ALL_HOSTS)
 		return;
 
+	if (in_dev->dead)
+		return;
 	if (IGMP_V1_SEEN(in_dev) || IGMP_V2_SEEN(in_dev)) {
 		spin_lock_bh(&im->lock);
 		igmp_start_timer(im, IGMP_Initial_Report_Delay);
@@ -1155,7 +1157,7 @@
 	igmpv3_del_delrec(in_dev, im->multiaddr);
 #endif
 	igmp_group_added(im);
-	if (in_dev->dev->flags & IFF_UP)
+	if (!in_dev->dead)
 		ip_rt_multicast_event(in_dev);
 out:
 	return;
@@ -1179,7 +1181,7 @@
 				write_unlock_bh(&in_dev->lock);
 				igmp_group_dropped(i);
 
-				if (in_dev->dev->flags & IFF_UP)
+				if (!in_dev->dead)
 					ip_rt_multicast_event(in_dev);
 
 				ip_ma_put(i);
@@ -1255,6 +1257,9 @@
 
 	ASSERT_RTNL();
 
+	/* Deactivate timers */
+	ip_mc_down(in_dev);
+
 	write_lock_bh(&in_dev->lock);
 	while ((i = in_dev->mc_list) != NULL) {
 		in_dev->mc_list = i->next;

Re: Deadlock problem with dev->refcnt somewhere in netlink/multicast... [PATCH]

[David S. Miller]

On Mon, 2 Feb 2004 16:41:11 -0700
David Stevens <dlstevens@us.ibm.com> wrote:

> Below is a patch for the reference count problem you ran into.
> 
> The problem is that the IGMP code was using "IFF_UP" to determine
> if a report should be sent or a timer should be started. But it is not
> necessarily cleared during a destroy_dev.
> 
> The IPv4 code was also missing an ip_mc_down() call added in the
> IPv6 code for a similar case by Jan Oravec.

Applied to both 2.4.x and 2.6.x, thanks David.

Re: Deadlock problem with dev->refcnt somewhere in netlink/multicast... [PATCH]

[Willy Tarreau]

Hi David,

On Mon, Feb 02, 2004 at 04:41:11PM -0700, David Stevens wrote:
> Below is a patch for the reference count problem you ran into.
> 
> The problem is that the IGMP code was using "IFF_UP" to determine
> if a report should be sent or a timer should be started. But it is not
> necessarily cleared during a destroy_dev.
> 
> The IPv4 code was also missing an ip_mc_down() call added in the
> IPv6 code for a similar case by Jan Oravec.

I would never have found this myself !

> The patch is for 2.4.x but also applies to 2.6.x. In-line for
> whitespace-mangled
> viewing and attached for applying.
> 
> Thanks for reporting the problem, Willy, and let me know if you have any
> problems with the patch.

Thanks a lot, I will try ASAP and will report if I still can break it.

Cheers,
Willy

Re: Deadlock problem with dev->refcnt somewhere in netlink/multicast... [PATCH]

[Willy Tarreau]

Hi David,

On Mon, Feb 02, 2004 at 04:41:11PM -0700, David Stevens wrote:
> Below is a patch for the reference count problem you ran into.

I have just tested 2.4.25-rc1 (which includes your patch) right now, and I
cannot reproduce the problem anymore. So to me, the problem is closed.

Thanks for the fix !
Willy