Skip to contentSkip to conversation

net: suspicious test in dev_change_name()

RFC PATCH In review Eric Dumazet 3 participants 7 messages Oct 30, 2009 – Nov 16, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

RFC PATCH 1/1 net: suspicious test in dev_change_name()

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 <[email protected]>

---

net/core/dev.c +1 -1
index b8f74cf..029cd41 100644
@@ -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",
#1 lore raw

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

Quoted 20 lines

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;
                }
        }
Quoted 1 line

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.

Quoted 24 lines
Signed-off-by Eric Dumazet <[email protected]>

---

net/core/dev.c +1 -2
index b8f74cf..029cd41 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#2 lore raw

From: Jarek Poplawski <[email protected]>
Date: Sat, 31 Oct 2009 00:50:09 +0100

Quoted 4 lines

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

#3 lore raw

PATCH 1/1 net: Fix the rollback test in dev_change_name()

Quoted 9 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Sat, 31 Oct 2009 00:50:09 +0100

Quoted 4 lines

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 <[email protected]>

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 <[email protected]>
Cc Eric Dumazet <[email protected]>

---

 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
net/core/dev.c +6 -5
index b8f74cf..fe10551 100644
@@ -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);
 		}
 	}
maintainer
#4 lore raw

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

From: Jarek Poplawski <[email protected]>
Date: Mon, 16 Nov 2009 09:30:24 +0000

Quoted 11 lines

From: Eric Dumazet <[email protected]>

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

Quoted 2 lines
Signed-off-by Jarek Poplawski <[email protected]>
Cc Eric Dumazet <[email protected]>

Ok.

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

Thanks.

author
#5 lore raw

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

Quoted 14 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Mon, 16 Nov 2009 09:30:24 +0000

Quoted 11 lines

From: Eric Dumazet <[email protected]>

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

Quoted 8 lines
Quoted 2 lines
Signed-off-by Jarek Poplawski <[email protected]>
Cc Eric Dumazet <[email protected]>

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 <[email protected]>
maintainer Applied
#6 lore raw

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

From: Eric Dumazet <[email protected]>
Date: Mon, 16 Nov 2009 12:08:22 +0100

Quoted 17 lines
Quoted 16 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Mon, 16 Nov 2009 09:30:24 +0000

Quoted 13 lines

From: Eric Dumazet <[email protected]>

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 <[email protected]>
Cc Eric Dumazet <[email protected]>

...

Quoted 2 lines
Quoted 2 lines

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

...

Quoted 1 line
Signed-off-by Eric Dumazet <[email protected]>

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