diff mbox

net: net_assign_generic() fix

Message ID 4A6EF0BF.2050801@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet July 28, 2009, 12:36 p.m. UTC 
Eric Dumazet a écrit :
> Pavel Emelyanov a écrit :
>> Eric Dumazet wrote:
>>> Igor M Podlesny a écrit :
>>>> [...]
>>>>> Could have been a problem in net core, perhaps.
>>>>>
>>>>> Below is a ppp fix from 2.6.31, but it seems unlikely to fix your problem.
>>>>>
>>>>> It would help if we could see that trace, please.  A digital photo
>>>>> would suit.
>>>> 	Here it is:
>>>>
>>>> 		http://bugzilla.kernel.org/attachment.cgi?id=22516
>>>>
>>>> 	(It's 2.6.30.3)
>>>> 	
>>> Looking at this, I believe net_assign_generic() is not safe.
>>>
>>> Two cpus could try to expand/update the array at same time, one update could be lost.
>>>
>>> register_pernet_gen_device() has a mutex to guard against concurrent
>>> calls, but net_assign_generic() has no locking at all.
>>>
>>> I doubt this is the reason of the crash, still worth to mention it...
>>>
>>> [PATCH] net: net_assign_generic() is not SMP safe
>>>
>>> Two cpus could try to expand/update the array at same time, one update
>>> could be lost during the copy of old array.
>> How can this happen? The array is updated only during ->init routines
>> of the pernet_operations, which are called from under the net_mutex.
>>
>> Do I miss anything?
>>
> 
> Oops, I missed the obvious "BUG_ON(!mutex_is_locked(&net_mutex));"
> 
> Sorry for the noise and untested patch as well :)
>

Hmm...

Real bug may be fixed by followed patch ? (yet untested, sorry...)

[PATCH] net: net_assign_generic() fix 

memcpy() should take into account size of pointers,
not only number of pointers to copy.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
--
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

Comments

Pavel Emelyanov July 28, 2009, 1:03 p.m. UTC | #1 
> Hmm...
> 
> Real bug may be fixed by followed patch ? (yet untested, sorry...)
> 
> [PATCH] net: net_assign_generic() fix 
> 
> memcpy() should take into account size of pointers,
> not only number of pointers to copy.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Acked-by: Pavel Emelyanov <xemul@openvz.org>

> ---
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index b7292a2..1972830 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -488,7 +488,7 @@ int net_assign_generic(struct net *net, int id, void *data)
>  	 */
>  
>  	ng->len = id;
> -	memcpy(&ng->ptr, &old_ng->ptr, old_ng->len);
> +	memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*));
>  
>  	rcu_assign_pointer(net->gen, ng);
>  	call_rcu(&old_ng->rcu, net_generic_release);
> 
> 

--
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
Eric Dumazet July 28, 2009, 1:16 p.m. UTC | #2 
Pavel Emelyanov a écrit :
>> Hmm...
>>
>> Real bug may be fixed by followed patch ? (yet untested, sorry...)
>>
>> [PATCH] net: net_assign_generic() fix 
>>
>> memcpy() should take into account size of pointers,
>> not only number of pointers to copy.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Acked-by: Pavel Emelyanov <xemul@openvz.org>

Thanks.

Still this doesnt explain the crash, because initial number of pointers is 13
(INITIAL_NET_GEN_PTRS)

We probably never realloc this array, unless a module forgets to
unregister_pernet_gen_device() and we load/unload it many times ?

--
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
Eric Dumazet July 28, 2009, 1:22 p.m. UTC | #3 
Eric Dumazet a écrit :
> Pavel Emelyanov a écrit :
>>> Hmm...
>>>
>>> Real bug may be fixed by followed patch ? (yet untested, sorry...)
>>>
>>> [PATCH] net: net_assign_generic() fix 
>>>
>>> memcpy() should take into account size of pointers,
>>> not only number of pointers to copy.
>>>
>>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Acked-by: Pavel Emelyanov <xemul@openvz.org>
> 
> Thanks.
> 
> Still this doesnt explain the crash, because initial number of pointers is 13
> (INITIAL_NET_GEN_PTRS)
> 
> We probably never realloc this array, unless a module forgets to
> unregister_pernet_gen_device() and we load/unload it many times ?
> 

Seems drivers/net/pppol2tp.c is a suspect...

It uses register_pernet_gen_device() from pppol2tp_init()
but doesnt call unregister_pernet_gen_device()
--
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
David Miller Aug. 2, 2009, 7:27 p.m. UTC | #4 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 28 Jul 2009 15:16:37 +0200

> Pavel Emelyanov a écrit :
>>> Hmm...
>>>
>>> Real bug may be fixed by followed patch ? (yet untested, sorry...)
>>>
>>> [PATCH] net: net_assign_generic() fix 
>>>
>>> memcpy() should take into account size of pointers,
>>> not only number of pointers to copy.
>>>
>>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> 
>> Acked-by: Pavel Emelyanov <xemul@openvz.org>
> 
> Thanks.

Applied, 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
diff mbox

Patch

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index b7292a2..1972830 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -488,7 +488,7 @@  int net_assign_generic(struct net *net, int id, void *data)
 	 */
 
 	ng->len = id;
-	memcpy(&ng->ptr, &old_ng->ptr, old_ng->len);
+	memcpy(&ng->ptr, &old_ng->ptr, old_ng->len * sizeof(void*));
 
 	rcu_assign_pointer(net->gen, ng);
 	call_rcu(&old_ng->rcu, net_generic_release);