Re: dev_get_valid_name buggy with hash collision

[Daniel Lezcano <daniel.lezcano@free.fr>]

On 05/18/2010 02:29 PM, Octavian Purdila wrote:
> On Tuesday 18 May 2010 13:17:10 you wrote:
>
>    
>> the commit:
>>
>> commit d90310243fd750240755e217c5faa13e24f41536
>> Author: Octavian Purdila<opurdila@ixiacom.com>
>> Date:   Wed Nov 18 02:36:59 2009 +0000
>>
>>       net: device name allocation cleanups
>>
>> introduced a bug when there is a hash collision making impossible to
>> rename a device with eth%d
>>
>>      

[ ... ]

>> --- net-2.6.orig/net/core/dev.c
>> +++ net-2.6/net/core/dev.c
>> @@ -936,18 +936,22 @@ int dev_alloc_name(struct net_device *de
>> }
>> EXPORT_SYMBOL(dev_alloc_name);
>>
>> -static int dev_get_valid_name(struct net *net, const char *name, char *buf,
>> -                             bool fmt)
>> +static int dev_get_valid_name(struct net_device *dev, const char *name, bool
>>      
> fmt)
>    
>> {
>> +       struct net *net;
>> +
>> +       BUG_ON(!dev_net(dev));
>> +       net = dev_net(dev);
>> +
>>         if (!dev_valid_name(name))
>>                 return -EINVAL;
>>
>>         if (fmt&&  strchr(name, '%'))
>> -               return __dev_alloc_name(net, name, buf);
>> +               return dev_alloc_name(dev, name);
>>         else if (__dev_get_by_name(net, name))
>>                 return -EEXIST;
>> -       else if (buf != name)
>> -               strlcpy(buf, name, IFNAMSIZ);
>> +       else if (strncmp(dev->name, name, IFNAMSIZ))
>> +                strlcpy(dev->name, name, IFNAMSIZ);
>>
>>      
> Why do the strncmp, can't we preserve the (buf != name) condition

The 'buf' parameter is no longer passed to the function. We have the 
'dev'  and the 'newname' parameters.
The pointer test was just to check 'dev_get_valid_name' was called from 
the 'register_netdevice' function context with 'dev_get_valid_name(net, 
dev->name, dev->name, 0)'. Comparing the strings is valid in this case.

Otherwise dev_get_valid_name is called from:

  *  "dev_change_net_namespace" with "dev%d" or "ifname" specified 
within the netlink message. Both are different pointers, the first will 
fall in the "if (fmt && strchr(name, '%'))".

  * "dev_change_name", where the pointers are different and the strings 
are different.

I think it is safe to do the string comparison here. But maybe there are 
a few simplifications (eg. remove fmt) to do.
If you agree, I will send this patch against net-2.6 and the 
simplifications against net-next-2.6.

Thanks
   -- Daniel