: Messed multicast lists after dev_mc_sync/unsync

NET Patrick McHardy 4 participants 9 messages Feb 19, 2008 – Feb 27, 2008

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Patrick McHardy

author

2008-02-19 14:57 UTC #0 lore raw

commit 6548b91f39381b2c5f02f99c14734546354bff89

Author: Jorge Boncompte [DTI2] <[email protected]>
Date:   Mon Feb 18 16:02:01 2008 +0100

[NET]: Messed multicast lists after dev_mc_sync/unsync

Commit a0a400d79e3dd7843e7e81baa3ef2957bdc292d0 from you introduced a new field "da_synced" to struct dev_addr_list that is not properly initialized to 0. So when any of the current users (8021q, macvlan, mac80211) calls dev_mc_sync/unsync they mess the address list for both devices.

The attached patch fixed it for me and avoid future problems.

Signed-off-by Jorge Boncompte [DTI2] <[email protected]>

Signed-off-by Patrick McHardy <[email protected]>

net/core/dev.c +1 -1

index 6cfc123..9516105 100644
@@ -2900,7 +2900,7 @@ int __dev_addr_add(struct dev_addr_list **list, int *count,
 		}
 	}
 
-	da = kmalloc(sizeof(*da), GFP_ATOMIC);
+	da = kzalloc(sizeof(*da), GFP_ATOMIC);
 	if (da == NULL)
 		return -ENOMEM;
 	memcpy(da->da_addr, addr, alen);

David Miller

maintainer

2008-02-19 22:17 UTC #1 lore raw

From: Patrick McHardy <[email protected]>
Date: Tue, 19 Feb 2008 15:56:59 +0100

Quoted 8 lines

[NET]: Messed multicast lists after dev_mc_sync/unsync
Commit a0a400d79e3dd7843e7e81baa3ef2957bdc292d0 from you introduced a new field "da_synced" to struct dev_addr_list that is not properly initialized to 0. So when any of the current users (8021q, macvlan, mac80211) calls dev_mc_sync/unsync they mess the address list for both devices.

Quoted 4 lines

The attached patch fixed it for me and avoid future problems.
Signed-off-by Jorge Boncompte [DTI2] <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>

I've added the headline description of the referenced commit to this commit message, and will queue this up to -stable.

Thanks!

Pekka Savola

2008-02-27 07:39 UTC #2 lore raw

This seems to have missed 2.6.24.3 (also 2.6.23.17), hopefully the next stable will have it.

Quoted 25 lines · David Miller

From: Patrick McHardy <[email protected]>
Date: Tue, 19 Feb 2008 15:56:59 +0100
Quoted 8 lines
[NET]: Messed multicast lists after dev_mc_sync/unsync
Commit a0a400d79e3dd7843e7e81baa3ef2957bdc292d0 from you introduced a new field "da_synced" to struct dev_addr_list that is not properly initialized to 0. So when any of the current users (8021q, macvlan, mac80211) calls dev_mc_sync/unsync they mess the address list for both devices.
Quoted 4 lines
The attached patch fixed it for me and avoid future problems.
Signed-off-by Jorge Boncompte [DTI2] <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>
I've added the headline description of the referenced commit to this commit message, and will queue this up to -stable.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Pekka Savola                 "You each name yourselves king, yet the
Netcore Oy                    kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings

David Miller

maintainer

2008-02-27 07:52 UTC #3 lore raw

From: Pekka Savola <[email protected]>
Date: Wed, 27 Feb 2008 09:38:34 +0200 (EET)

Quoted 2 lines

This seems to have missed 2.6.24.3 (also 2.6.23.17), hopefully the next stable will have it.

I will make sure it does.

Johannes Berg

2008-02-27 11:37 UTC #4 lore raw

In reply to Patrick McHardy

net/core/dev.c int __dev_addr_add(struct dev_addr_list **list, int *count, · line 2903

Quoted 9 lines

Commit a0a400d79e3dd7843e7e81baa3ef2957bdc292d0 from you introduced a new field "da_synced" to struct dev_addr_list that is not properly initialized to 0. So when any of the current users (8021q, macvlan, mac80211) calls dev_mc_sync/unsync they mess the address list for both devices.
net/core/dev.c +1 -1
-	da = kmalloc(sizeof(*da), GFP_ATOMIC);
+	da = kzalloc(sizeof(*da), GFP_ATOMIC);

Could this be the reason for us seeing "multicast address leakage" warnings?

johannes

Patrick McHardy

author

2008-02-27 11:47 UTC #5 lore raw

Quoted 8 lines · Johannes Berg

Quoted 8 lines
Commit a0a400d79e3dd7843e7e81baa3ef2957bdc292d0 from you introduced a new field "da_synced" to struct dev_addr_list that is not properly initialized to 0. So when any of the current users (8021q, macvlan, mac80211) calls dev_mc_sync/unsync they mess the address list for both devices.

Quoted 6 lines

net/core/dev.c int __dev_addr_add(struct dev_addr_list **list, int *count, · line 2903
Quoted 3 lines
net/core/dev.c +1 -1
-	da = kmalloc(sizeof(*da), GFP_ATOMIC);
+	da = kzalloc(sizeof(*da), GFP_ATOMIC);
Could this be the reason for us seeing "multicast address leakage" warnings?

I don't think so, the users field was already initialized properly before. Is wireless using dev_unicast_sync or dev_mc_sync?

Johannes Berg

2008-02-27 12:34 UTC #6 lore raw

Quoted 7 lines

Quoted 3 lines
Could this be the reason for us seeing "multicast address leakage" warnings?
I don't think so, the users field was already initialized properly before. Is wireless using dev_unicast_sync or dev_mc_sync?

dev_mc_sync(), we sync all multicast addresses of all interfaces to the master netdev to have a single consistent list. There used to be a custom implementation of that but I replaced it.

johannes

Patrick McHardy

author

2008-02-27 12:42 UTC #7 lore raw

Quoted 9 lines · Johannes Berg

Quoted 5 lines
Quoted 2 lines
Could this be the reason for us seeing "multicast address leakage" warnings?
I don't think so, the users field was already initialized properly before. Is wireless using dev_unicast_sync or dev_mc_sync?
dev_mc_sync(), we sync all multicast addresses of all interfaces to the master netdev to have a single consistent list. There used to be a custom implementation of that but I replaced it.

I can't see how this bug could be responsible, assuming da_synced intially contained a non-zero value it might forget to sync an address, but still try to delete it when unsyncing. __dev_addr_delete will simply do nothing in this case. But I guess the patch is still worth trying.

Johannes Berg

2008-02-27 12:46 UTC #8 lore raw

Quoted 9 lines

Quoted 7 lines
Quoted 3 lines
I don't think so, the users field was already initialized properly before. Is wireless using dev_unicast_sync or dev_mc_sync?
dev_mc_sync(), we sync all multicast addresses of all interfaces to the master netdev to have a single consistent list. There used to be a custom implementation of that but I replaced it.

Quoted 5 lines

I can't see how this bug could be responsible, assuming da_synced intially contained a non-zero value it might forget to sync an address, but still try to delete it when unsyncing. __dev_addr_delete will simply do nothing in this case. But I guess the patch is still worth trying.

I don't know how it would cause it either, but seeing that I can't even reliably reproduce the problem at least that seems like uninitialised memory could cause it. I guess I'll see if I the problem ever turns up again after this patch goes in...

johannes