[NET]: Messed multicast lists after dev_mc_sync/unsync

[NET]: Messed multicast lists after dev_mc_sync/unsync

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1058 bytes --]

commit 6548b91f39381b2c5f02f99c14734546354bff89
Author: Jorge Boncompte [DTI2] <jorge@dti2.net>
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] <jorge@dti2.net>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/core/dev.c b/net/core/dev.c
index 6cfc123..9516105 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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);

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 19 Feb 2008 15:56:59 +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] <jorge@dti2.net>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

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

Thanks!

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[Pekka Savola]

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

On Tue, 19 Feb 2008, David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Tue, 19 Feb 2008 15:56:59 +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] <jorge@dti2.net>
>>     Signed-off-by: Patrick McHardy <kaber@trash.net>
>
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[David Miller]

From: Pekka Savola <pekkas@netcore.fi>
Date: Wed, 27 Feb 2008 09:38:34 +0200 (EET)

> 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.

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

    
>     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.
>  
> -	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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[Patrick McHardy]

Johannes Berg wrote:
>     
>   
>>     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.
>>  
>> -	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?

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]


> > 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[Patrick McHardy]

Johannes Berg wrote:
>>> 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.

Re: [NET]: Messed multicast lists after dev_mc_sync/unsync

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]


> >> 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.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]