[patch net] net: correct check in dev_addr_del()

[patch net] net: correct check in dev_addr_del()

[Jiri Pirko]

Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
sets this. Correct the check to behave properly on addr removal.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/core/dev_addr_lists.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 87cc17d..b079c7b 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -319,7 +319,8 @@ int dev_addr_del(struct net_device *dev, const unsigned char *addr,
 	 */
 	ha = list_first_entry(&dev->dev_addrs.list,
 			      struct netdev_hw_addr, list);
-	if (ha->addr == dev->dev_addr && ha->refcount == 1)
+	if (!memcmp(ha->addr, addr, dev->addr_len) &&
+	    ha->type == addr_type && ha->refcount == 1)
 		return -ENOENT;
 
 	err = __hw_addr_del(&dev->dev_addrs, addr, dev->addr_len,
-- 
1.8.0

Re: [patch net] net: correct check in dev_addr_del()

[Eric Dumazet]

On Wed, 2012-11-14 at 13:51 +0100, Jiri Pirko wrote:
> Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
> sets this. Correct the check to behave properly on addr removal.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---

Please add in the changelog which commit added the bug, to ease
stable teams work.

Thanks

Re: [patch net] net: correct check in dev_addr_del()

[Jiri Pirko]

Wed, Nov 14, 2012 at 03:18:35PM CET, eric.dumazet@gmail.com wrote:
>On Wed, 2012-11-14 at 13:51 +0100, Jiri Pirko wrote:
>> Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
>> sets this. Correct the check to behave properly on addr removal.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>
>Please add in the changelog which commit added the bug, to ease
>stable teams work.

bug introduced by:

commit a748ee2426817a95b1f03012d8f339c45c722ae1
Author: Jiri Pirko <jpirko@redhat.com>
Date:   Thu Apr 1 21:22:09 2010 +0000

    net: move address list functions to a separate file


>
>Thanks
>
>

Re: [patch net] net: correct check in dev_addr_del()

[Eric Dumazet]

On Wed, 2012-11-14 at 15:29 +0100, Jiri Pirko wrote:
> Wed, Nov 14, 2012 at 03:18:35PM CET, eric.dumazet@gmail.com wrote:
> >On Wed, 2012-11-14 at 13:51 +0100, Jiri Pirko wrote:
> >> Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
> >> sets this. Correct the check to behave properly on addr removal.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> ---
> >
> >Please add in the changelog which commit added the bug, to ease
> >stable teams work.
> 
> bug introduced by:
> 
> commit a748ee2426817a95b1f03012d8f339c45c722ae1
> Author: Jiri Pirko <jpirko@redhat.com>
> Date:   Thu Apr 1 21:22:09 2010 +0000
> 
>     net: move address list functions to a separate file

Good, the usual way is then to add in the changelog :

Bug added in commit a748ee242681
(net: move address list functions to a separate file)


(No need to give the author/date, and we can shorten the SHA1 to 10 or
12 digits)

Thanks

Re: [patch net] net: correct check in dev_addr_del()

[Jiri Pirko]

Wed, Nov 14, 2012 at 03:38:59PM CET, eric.dumazet@gmail.com wrote:
>On Wed, 2012-11-14 at 15:29 +0100, Jiri Pirko wrote:
>> Wed, Nov 14, 2012 at 03:18:35PM CET, eric.dumazet@gmail.com wrote:
>> >On Wed, 2012-11-14 at 13:51 +0100, Jiri Pirko wrote:
>> >> Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
>> >> sets this. Correct the check to behave properly on addr removal.
>> >> 
>> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> >> ---
>> >
>> >Please add in the changelog which commit added the bug, to ease
>> >stable teams work.
>> 
>> bug introduced by:
>> 
>> commit a748ee2426817a95b1f03012d8f339c45c722ae1
>> Author: Jiri Pirko <jpirko@redhat.com>
>> Date:   Thu Apr 1 21:22:09 2010 +0000
>> 
>>     net: move address list functions to a separate file
>
>Good, the usual way is then to add in the changelog :
>
>Bug added in commit a748ee242681
>(net: move address list functions to a separate file)
>
>
>(No need to give the author/date, and we can shorten the SHA1 to 10 or
>12 digits)

Mind noted. Thanks Eric.

>
>Thanks
>
>

Re: [patch net] net: correct check in dev_addr_del()

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 14 Nov 2012 13:51:04 +0100

> Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
> sets this. Correct the check to behave properly on addr removal.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

I'm pretty sure this is very intentional.

It's trying to prevent deletion of the implicit dev->dev_addr
entry.  But it will allow decementing the reference count to
1, but no further.

I'm not applying this.

Re: [patch net] net: correct check in dev_addr_del()

[Jiri Pirko]

Thu, Nov 15, 2012 at 03:52:54AM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Wed, 14 Nov 2012 13:51:04 +0100
>
>> Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
>> sets this. Correct the check to behave properly on addr removal.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
>I'm pretty sure this is very intentional.
>
>It's trying to prevent deletion of the implicit dev->dev_addr
>entry.  But it will allow decementing the reference count to
>1, but no further.
>
>I'm not applying this.

Please look at dev_addr_init(), line 266:
dev->dev_addr = ha->addr;

and that is never changed.
Therefore check (ha->addr == dev->dev_addr) in dev_addr_del() is always
true and has no meaning.

dev_addr_del() should check if the address passed in "const unsigned
char *addr" function arg is the same as in ha->addr (dev->dev_addr) and
prevent to remove it in that case. And that is what this patch does.

Re: [patch net] net: correct check in dev_addr_del()

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 15 Nov 2012 11:12:38 +0100

> Thu, Nov 15, 2012 at 03:52:54AM CET, davem@davemloft.net wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Date: Wed, 14 Nov 2012 13:51:04 +0100
>>
>>> Check (ha->addr == dev->dev_addr) is always true because dev_addr_init()
>>> sets this. Correct the check to behave properly on addr removal.
>>> 
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>
>>I'm pretty sure this is very intentional.
>>
>>It's trying to prevent deletion of the implicit dev->dev_addr
>>entry.  But it will allow decementing the reference count to
>>1, but no further.
>>
>>I'm not applying this.
> 
> Please look at dev_addr_init(), line 266:
> dev->dev_addr = ha->addr;
> 
> and that is never changed.
> Therefore check (ha->addr == dev->dev_addr) in dev_addr_del() is always
> true and has no meaning.
> 
> dev_addr_del() should check if the address passed in "const unsigned
> char *addr" function arg is the same as in ha->addr (dev->dev_addr) and
> prevent to remove it in that case. And that is what this patch does.

Thanks for explaining.  I misread the code and thought that 'ha'
was already the object looked up using 'addr' as the key.

Applied and queued up for -stable, thanks.

Re: [patch net] net: correct check in dev_addr_del()

[Jan Engelhardt]

On Wednesday 2012-11-14 15:38, Eric Dumazet wrote:
>> 
>> bug introduced by:
>> 
>> commit a748ee2426817a95b1f03012d8f339c45c722ae1
>> Author: Jiri Pirko <jpirko@redhat.com>
>> Date:   Thu Apr 1 21:22:09 2010 +0000
>> 
>>     net: move address list functions to a separate file
>
>Good, the usual way is then to add in the changelog :
>
>Bug added in commit a748ee242681
>(net: move address list functions to a separate file)
>
>(No need to give the author/date, and we can shorten the SHA1 to 10 or
>12 digits)

A shortened hash value is however not truly future-proof. I recommend
using `git describe` or `git describe --contains`, as that is
unambiguous into the future and shows a related version. In case of
a748ee, that would be v2.6.35-rc1~473^2~602, which immediately tells
us that the bug has been in tarballs since 2.6.35-rc1.