[RFC, PATCH] net: suspicious test in dev_change_name()

[RFC, PATCH] net: suspicious test in dev_change_name()

[Eric Dumazet]

While preparing a patch for net-next-2.6, I noticed following code in dev_change_name()

int err = 0;
...
        ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
        ret = notifier_to_errno(ret);

        if (ret) {
<< HERE >>      if (err) {
                        printk(KERN_ERR
                               "%s: name change rollback failed: %d.\n",
                               dev->name, ret);
                } else {
                        err = ret;
                        memcpy(dev->name, oldname, IFNAMSIZ);
                        goto rollback;
                }
        }


It seems intent was to test if notifier_to_errno() was null ?

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---

diff --git a/net/core/dev.c b/net/core/dev.c
index b8f74cf..029cd41 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -939,9 +939,9 @@ rollback:
 	write_unlock_bh(&dev_base_lock);
 
 	ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
-	ret = notifier_to_errno(ret);
 
 	if (ret) {
+		err = notifier_to_errno(ret);
 		if (err) {
 			printk(KERN_ERR
 			       "%s: name change rollback failed: %d.\n",

Re: [RFC, PATCH] net: suspicious test in dev_change_name()

[Jarek Poplawski]

Eric Dumazet wrote, On 10/30/2009 10:20 AM:

> While preparing a patch for net-next-2.6, I noticed following code in dev_change_name()
> 
> int err = 0;
> ...
>         ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
>         ret = notifier_to_errno(ret);
> 
>         if (ret) {
> << HERE >>      if (err) {
>                         printk(KERN_ERR
>                                "%s: name change rollback failed: %d.\n",
>                                dev->name, ret);
>                 } else {
>                         err = ret;
>                         memcpy(dev->name, oldname, IFNAMSIZ);
>                         goto rollback;
>                 }
>         }
> 
> 
> It seems intent was to test if notifier_to_errno() was null ?


I don't think so: err stores the previous ret meaning rollback and
is checked for this later. But somebody forgot err can store previous
(positive) value here, so IMHO you're right: there is a bug in this
place ;-)

Jarek P.

> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b8f74cf..029cd41 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -939,9 +939,9 @@ rollback:
>  	write_unlock_bh(&dev_base_lock);
>  
>  	ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
> -	ret = notifier_to_errno(ret);
>  
>  	if (ret) {
> +		err = notifier_to_errno(ret);
>  		if (err) {
>  			printk(KERN_ERR
>  			       "%s: name change rollback failed: %d.\n",
> --
> 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
> 

Re: [RFC, PATCH] net: suspicious test in dev_change_name()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sat, 31 Oct 2009 00:50:09 +0100

> I don't think so: err stores the previous ret meaning rollback and
> is checked for this later. But somebody forgot err can store previous
> (positive) value here, so IMHO you're right: there is a bug in this
> place ;-)

Just not the one Eric is specifically fixing :-)

[PATCH net] net: Fix the rollback test in dev_change_name()

[Jarek Poplawski]

On Mon, Nov 02, 2009 at 12:05:49AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Sat, 31 Oct 2009 00:50:09 +0100
> 
> > I don't think so: err stores the previous ret meaning rollback and
> > is checked for this later. But somebody forgot err can store previous
> > (positive) value here, so IMHO you're right: there is a bug in this
> > place ;-)
> 
> Just not the one Eric is specifically fixing :-)

Since this bug looks quite serious (consider -stable), here is a
proposal in case we forget what is Eric specifically fixing. ;-)

Thanks,
Jarek P.

---------------->
From: Eric Dumazet <eric.dumazet@gmail.com>

net: Fix the rollback test in dev_change_name()

In dev_change_name() an err variable is used for storing the original
call_netdevice_notifiers() errno (negative) and testing for a rollback
error later, but the test for non-zero is wrong, because the err might
have positive value as well - from dev_alloc_name(). It means the
rollback for a netdevice with a number > 0 will never happen. (The err
test is reordered btw. to make it more readable.)

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---

 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b8f74cf..fe10551 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -942,14 +942,15 @@ rollback:
 	ret = notifier_to_errno(ret);
 
 	if (ret) {
-		if (err) {
-			printk(KERN_ERR
-			       "%s: name change rollback failed: %d.\n",
-			       dev->name, ret);
-		} else {
+		/* err >= 0 after dev_alloc_name() or stores the first errno */
+		if (err >= 0) {
 			err = ret;
 			memcpy(dev->name, oldname, IFNAMSIZ);
 			goto rollback;
+		} else {
+			printk(KERN_ERR
+			       "%s: name change rollback failed: %d.\n",
+			       dev->name, ret);
 		}
 	}
 

Re: [PATCH net] net: Fix the rollback test in dev_change_name()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 16 Nov 2009 09:30:24 +0000

> From: Eric Dumazet <eric.dumazet@gmail.com>
> 
> net: Fix the rollback test in dev_change_name()
> 
> In dev_change_name() an err variable is used for storing the original
> call_netdevice_notifiers() errno (negative) and testing for a rollback
> error later, but the test for non-zero is wrong, because the err might
> have positive value as well - from dev_alloc_name(). It means the
> rollback for a netdevice with a number > 0 will never happen. (The err
> test is reordered btw. to make it more readable.)
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>

Ok.

Eric please give your signoff and I'll put this where it needs
to go.

Thanks.

Re: [PATCH net] net: Fix the rollback test in dev_change_name()

[Eric Dumazet]

David Miller a écrit :
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 16 Nov 2009 09:30:24 +0000
> 
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>
>> net: Fix the rollback test in dev_change_name()
>>
>> In dev_change_name() an err variable is used for storing the original
>> call_netdevice_notifiers() errno (negative) and testing for a rollback
>> error later, but the test for non-zero is wrong, because the err might
>> have positive value as well - from dev_alloc_name(). It means the
>> rollback for a netdevice with a number > 0 will never happen. (The err
>> test is reordered btw. to make it more readable.)
>>
>> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Ok.
> 
> Eric please give your signoff and I'll put this where it needs
> to go.
> 

I finaly understood why I got stuck on this one : I thought dev_alloc_name()
was returning 0 in case of success. The test was thus realy obscure for me.

So Jarek patch is pretty clear now I saw the light !

Thanks

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH net] net: Fix the rollback test in dev_change_name()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 16 Nov 2009 12:08:22 +0100

> David Miller a écrit :
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Mon, 16 Nov 2009 09:30:24 +0000
>> 
>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>>
>>> net: Fix the rollback test in dev_change_name()
>>>
>>> In dev_change_name() an err variable is used for storing the original
>>> call_netdevice_notifiers() errno (negative) and testing for a rollback
>>> error later, but the test for non-zero is wrong, because the err might
>>> have positive value as well - from dev_alloc_name(). It means the
>>> rollback for a netdevice with a number > 0 will never happen. (The err
>>> test is reordered btw. to make it more readable.)
>>>
>>> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
 ...
>> Eric please give your signoff and I'll put this where it needs
>> to go.
 ...
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied to net-2.6 and queued for -stable, thanks everyone!