net: add checks if dev->netdev_ops is NULL in register_netdevice()

PATCH In review dingtianhong 5 participants 5 messages Apr 8, 2013 – Apr 10, 2013

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Eric Dumazet

2013-04-08 15:16 UTC #1 lore raw

net/core/dev.c int register_netdevice(struct net_device *dev)

Quoted 11 lines · dingtianhong

In some cases netdev->netdev_ops could be NULL in the register_netdevice(), thus a NULL point deference happens and lead to oops:
net/core/dev.c +1 -2
 	/* Init, if this function is available */
-	if (dev->netdev_ops->ndo_init) {
+	if (dev->netdev_ops && dev->netdev_ops->ndo_init) {
 		ret = dev->netdev_ops->ndo_init(dev);
 		if (ret) {
 			if (ret > 0)
-- 1.8.0

Seriously ?

This patch is not needed, and not complete anyway.

Fix the driver instead, we don't want this to happen.

David Miller

maintainer

2013-04-08 15:34 UTC #2 lore raw

From: Eric Dumazet <[email protected]>
Date: Mon, 08 Apr 2013 08:16:34 -0700

Quoted 15 lines · dingtianhong

net/core/dev.c int register_netdevice(struct net_device *dev)
Quoted 11 lines
In some cases netdev->netdev_ops could be NULL in the register_netdevice(), thus a NULL point deference happens and lead to oops:
net/core/dev.c +1 -2
 	/* Init, if this function is available */
-	if (dev->netdev_ops->ndo_init) {
+	if (dev->netdev_ops && dev->netdev_ops->ndo_init) {
 		ret = dev->netdev_ops->ndo_init(dev);
 		if (ret) {
 			if (ret > 0)
-- 1.8.0
Seriously ?

Quoted 3 lines

This patch is not needed, and not complete anyway.
Fix the driver instead, we don't want this to happen.

Agreed, I'm not seriously considering patches like this at all.

Alexander Duyck

2013-04-08 15:49 UTC #3 lore raw

In reply to dingtianhong

Quoted 39 lines · dingtianhong

In some cases netdev->netdev_ops could be NULL in the register_netdevice(), thus a NULL point deference happens and lead to oops:

[ 8260.836400] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 8260.955306] IP: [<ffffffff81388333>] register_netdevice+0x73/0x2d0
[ 8261.054888] PGD 2f4dcd067 PUD 2f4de0067 PMD 0
[ 8261.134244] Oops: 0000 [#1] SMP
[ 8261.198938] CPU 0
[ 8262.605182] RIP: 0010:[<ffffffff81388333>]  [<ffffffff81388333>] register_netdevice+0x73/0x2d0
[ 8262.741859] RSP: 0018:ffff8802f489f9f8  EFLAGS: 00010246
[ 8262.839463] RAX: 0000000000000000 RBX: ffff8802f4e73000 RCX: ffff8802f4e73006
[ 8262.959484] RDX: 0000000000000000 RSI: 00000000daa92527 RDI: ffff8802f4e73006
[ 8263.079352] RBP: 0000000000000000 R08: 0000000000000000 R09: ffff880327d898c0
[ 8263.198987] R10: 00000000000000c1 R11: 0000000000000000 R12: ffffffff81fed080
[ 8263.318401] R13: ffffffff81fed080 R14: ffff8802f4e73000 R15: 0000000000000000
[ 8263.438014] FS:  00007f01dfa81700(0000) GS:ffff88033f200000(0000) knlGS:0000000000000000
[ 8263.569778] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 8263.673859] CR2: 0000000000000000 CR3: 000000032893f000 CR4: 00000000000006f0
[ 8263.794922] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 8263.915734] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 8264.036251] Process rmcli (pid: 14089, threadinfo ffff8802f489e000, task ffff880324708080)
[ 8264.170961] Stack:
[ 8264.231328]  ffff88032a3c0000 0000000000000003 0000000000000003 ffff88032a3c0000
[ 8264.356967]  0000000000000003 ffffffffa039dbcb 000000003ec216c0 0000000000000246
[ 8264.483081]  382e30363238205b 00205d3900000006 ff00305f63696e76 0000000000000003

We should check the point to avoid the oops.

Signed-off-by Ding Tianhong <[email protected]>

---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/core/dev.c +0 -0

index e7d68ed..596a11e 100644

net/core/dev.c int register_netdevice(struct net_device *dev)

Quoted 9 lines

net/core/dev.c +1 -2

@@ -5159,7 +5159,7 @@ int register_netdevice(struct net_device *dev)
 		goto out;
 
 	/* Init, if this function is available */
-	if (dev->netdev_ops->ndo_init) {
+	if (dev->netdev_ops && dev->netdev_ops->ndo_init) {
 		ret = dev->netdev_ops->ndo_init(dev);
 		if (ret) {
 			if (ret > 0)
-- 1.8.0

The correct fix for this is to not allow the registration of devices with a null netdev_ops pointer. The subtle approach to resolving this is adding a check for the null pointer near the start of the function and returning -EVINAL since the dev structure passed was configured incorrectly. The blunt approach is to just add a BUG_ON with a comment on the line before it explaining that you should always set the netdev_ops pointer before trying to register a netdev. That way if someone else makes the mistake then they will hit the BUG_ON and realize what they are doing wrong.

Thanks,

Alex

Stephen Hemminger

2013-04-10 20:45 UTC #4 lore raw

In reply to dingtianhong

Quoted 26 lines · On Mon, 8 Apr 2013 17:43:53 +0800 dingtianhong <[email protected]> wrote:

In some cases netdev->netdev_ops could be NULL in the register_netdevice(), thus a NULL point deference happens and lead to oops:

[ 8260.836400] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 8260.955306] IP: [<ffffffff81388333>] register_netdevice+0x73/0x2d0
[ 8261.054888] PGD 2f4dcd067 PUD 2f4de0067 PMD 0
[ 8261.134244] Oops: 0000 [#1] SMP
[ 8261.198938] CPU 0
[ 8262.605182] RIP: 0010:[<ffffffff81388333>]  [<ffffffff81388333>] register_netdevice+0x73/0x2d0
[ 8262.741859] RSP: 0018:ffff8802f489f9f8  EFLAGS: 00010246
[ 8262.839463] RAX: 0000000000000000 RBX: ffff8802f4e73000 RCX: ffff8802f4e73006
[ 8262.959484] RDX: 0000000000000000 RSI: 00000000daa92527 RDI: ffff8802f4e73006
[ 8263.079352] RBP: 0000000000000000 R08: 0000000000000000 R09: ffff880327d898c0
[ 8263.198987] R10: 00000000000000c1 R11: 0000000000000000 R12: ffffffff81fed080
[ 8263.318401] R13: ffffffff81fed080 R14: ffff8802f4e73000 R15: 0000000000000000
[ 8263.438014] FS:  00007f01dfa81700(0000) GS:ffff88033f200000(0000) knlGS:0000000000000000
[ 8263.569778] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 8263.673859] CR2: 0000000000000000 CR3: 000000032893f000 CR4: 00000000000006f0
[ 8263.794922] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 8263.915734] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 8264.036251] Process rmcli (pid: 14089, threadinfo ffff8802f489e000, task ffff880324708080)
[ 8264.170961] Stack:
[ 8264.231328]  ffff88032a3c0000 0000000000000003 0000000000000003 ffff88032a3c0000
[ 8264.356967]  0000000000000003 ffffffffa039dbcb 000000003ec216c0 0000000000000246
[ 8264.483081]  382e30363238205b 00205d3900000006 ff00305f63696e76 0000000000000003

Quoted 3 lines

We should check the point to avoid the oops.
Signed-off-by Ding Tianhong <[email protected]>

I notice you truncated the back trace. You should include the whole thing so the buggy driver can be found and shot. If it is your own code fix it.

Adding a BUG_ON() is a waste, the NULL trap is the same effect.