* [PATCH] net: init ingress queue
@ 2010-12-04 5:45 Changli Gao
2010-12-04 8:47 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Changli Gao @ 2010-12-04 5:45 UTC (permalink / raw)
To: David S. Miller
Cc: Eric Dumazet, Tom Herbert, Jiri Pirko, netdev, Changli Gao
The dev field of ingress queue is forgot to initialized, then NULL
pointer dereference happens in qdisc_alloc().
[ 303.019348] BUG: unable to handle kernel NULL pointer dereference at 0000000000000398
[ 303.020068] IP: [<ffffffff81472aab>] qdisc_alloc+0x9b/0xc0
[ 303.020068] PGD 3d637067 PUD 3d03f067 PMD 0
[ 303.020068] Oops: 0000 [#1] SMP
[ 303.020068] last sysfs file: /sys/kernel/uevent_seqnum
[ 303.020068] CPU 0
[ 303.020068] Modules linked in: sch_ingress ipv6
[ 303.020068]
[ 303.020068] Pid: 3138, comm: tc Not tainted 2.6.37-rc1+ #90 /VirtualBox
[ 303.020068] RIP: 0010:[<ffffffff81472aab>] [<ffffffff81472aab>] qdisc_alloc+0x9b/0xc0
[ 303.020068] RSP: 0018:ffff88003daf1938 EFLAGS: 00010246
[ 303.020068] RAX: ffff88003db01400 RBX: ffffffffa00612a0 RCX: 0000000000000000
[ 303.020068] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88003db01600
[ 303.020068] RBP: ffff88003daf1948 R08: 0000000000000000 R09: ffff88003db01400
[ 303.020068] R10: ffff88003fbe9fe8 R11: dead000000200200 R12: ffff88003d37c600
[ 303.020068] R13: 00000000fffffff1 R14: ffffffffa00612a0 R15: 00000000fffffff1
[ 303.020068] FS: 00007f1b3575a700(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
[ 303.020068] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 303.020068] CR2: 0000000000000398 CR3: 000000003d2a9000 CR4: 00000000000006f0
[ 303.020068] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 303.020068] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 303.020068] Process tc (pid: 3138, threadinfo ffff88003daf0000, task ffff88003d407500)
[ 303.020068] Stack:
[ 303.020068] ffff88003e231000 ffff88003d6e1024 ffff88003daf19b8 ffffffff81474d68
[ 303.020068] 0000000000000001 0000000000000000 ffff88003d37c600 ffff88003daf1a08
[ 303.020068] 0073736572676e69 0000000000000000 ffff88003daf19b8 ffff88003d6e1010
[ 303.020068] Call Trace:
[ 303.020068] [<ffffffff81474d68>] qdisc_create+0x78/0x350
[ 303.020068] [<ffffffff81476849>] tc_modify_qdisc+0x339/0x590
[ 303.020068] [<ffffffff8146622f>] rtnetlink_rcv_msg+0x16f/0x280
[ 303.020068] [<ffffffff814660c0>] ? rtnetlink_rcv_msg+0x0/0x280
[ 303.020068] [<ffffffff8147d6d9>] netlink_rcv_skb+0xa9/0xd0
[ 303.020068] [<ffffffff814660b0>] rtnetlink_rcv+0x20/0x30
[ 303.020068] [<ffffffff8147ce85>] netlink_unicast+0x2c5/0x2e0
[ 303.020068] [<ffffffff8147e195>] netlink_sendmsg+0x245/0x360
[ 303.020068] [<ffffffff81444874>] sock_sendmsg+0xe4/0x110
[ 303.020068] [<ffffffff810c5e19>] ? find_get_page+0x19/0x90
[ 303.020068] [<ffffffff810c716a>] ? filemap_fault+0xca/0x4b0
[ 303.020068] [<ffffffff810c6cd5>] ? unlock_page+0x25/0x30
[ 303.020068] [<ffffffff81444b6d>] ? move_addr_to_kernel+0x5d/0x60
[ 303.020068] [<ffffffff814500bd>] ? verify_iovec+0x7d/0xf0
[ 303.020068] [<ffffffff814465b5>] sys_sendmsg+0x1e5/0x330
[ 303.020068] [<ffffffff8147b5c8>] ? netlink_table_ungrab+0x28/0x30
[ 303.020068] [<ffffffff81516fac>] ? do_page_fault+0x1dc/0x4c0
[ 303.020068] [<ffffffff81444abb>] ? move_addr_to_user+0x9b/0xb0
[ 303.020068] [<ffffffff8112463b>] ? alloc_fd+0x4b/0x140
[ 303.020068] [<ffffffff81446394>] ? sys_recvmsg+0x44/0x80
[ 303.020068] [<ffffffff81002eab>] system_call_fastpath+0x16/0x1b
[ 303.020068] Code: 8d 90 88 00 00 00 48 89 90 88 00 00 00 48 89 90 90 00 00 00 48 8b 53 28 48 89 10 48 8b 53 30 4c 89 60 68 48 89 50 08 49 8b 14 24 <48> 8b 92 98 03 00 00 65 ff 02 c7 40 40 01 00 00 00 5b 41 5c c9
[ 303.020068] RIP [<ffffffff81472aab>] qdisc_alloc+0x9b/0xc0
[ 303.020068] RSP <ffff88003daf1938>
[ 303.020068] CR2: 0000000000000398
[ 303.135737] ---[ end trace 5b6e09a3328c82e4 ]---
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
net/core/dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index cd24374..8083c68 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5577,6 +5577,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
queue = kzalloc(sizeof(*queue), GFP_KERNEL);
if (!queue)
return NULL;
+ netdev_queue_numa_node_write(queue, -1);
+ queue->dev = dev;
netdev_init_one_queue(dev, queue, NULL);
queue->qdisc = &noop_qdisc;
queue->qdisc_sleeping = &noop_qdisc;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: init ingress queue
2010-12-04 5:45 [PATCH] net: init ingress queue Changli Gao
@ 2010-12-04 8:47 ` Eric Dumazet
2010-12-04 8:55 ` Changli Gao
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2010-12-04 8:47 UTC (permalink / raw)
To: Changli Gao; +Cc: David S. Miller, Tom Herbert, Jiri Pirko, netdev
Le samedi 04 décembre 2010 à 13:45 +0800, Changli Gao a écrit :
> The dev field of ingress queue is forgot to initialized, then NULL
> pointer dereference happens in qdisc_alloc().
< deleted oops >
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> net/core/dev.c | 2 ++
> 1 file changed, 2 insertions(+)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cd24374..8083c68 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5577,6 +5577,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
> queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> if (!queue)
> return NULL;
> + netdev_queue_numa_node_write(queue, -1);
> + queue->dev = dev;
> netdev_init_one_queue(dev, queue, NULL);
> queue->qdisc = &noop_qdisc;
> queue->qdisc_sleeping = &noop_qdisc;
Hi Changli, thanks for bug report and patch.
IMHO there is no point to include this long Oops report in Changelog for
a net-next-2.6 temporary problem, there wont be any bugzilla report.
Instead, you could say it is a followup patch for commits 1d24eb4815d1e0
and f2cd2d3e9b3ef960. Two or three lines with relevant information.
I suggest you submit following patch instead, as this seems cleaner to
me ?
(factorize the two inits in netdev_init_one_queue())
diff --git a/net/core/dev.c b/net/core/dev.c
index cd24374..36e10b4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5113,7 +5113,6 @@ static int netif_alloc_netdev_queues(struct net_device *dev)
{
unsigned int count = dev->num_tx_queues;
struct netdev_queue *tx;
- int i;
BUG_ON(count < 1);
@@ -5125,10 +5124,6 @@ static int netif_alloc_netdev_queues(struct net_device *dev)
}
dev->_tx = tx;
- for (i = 0; i < count; i++) {
- netdev_queue_numa_node_write(&tx[i], -1);
- tx[i].dev = dev;
- }
return 0;
}
@@ -5140,6 +5135,8 @@ static void netdev_init_one_queue(struct net_device *dev,
spin_lock_init(&queue->_xmit_lock);
netdev_set_xmit_lockdep_class(&queue->_xmit_lock, dev->type);
queue->xmit_lock_owner = -1;
+ netdev_queue_numa_node_write(queue, -1);
+ queue->dev = dev;
}
static void netdev_init_queues(struct net_device *dev)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: init ingress queue
2010-12-04 8:47 ` Eric Dumazet
@ 2010-12-04 8:55 ` Changli Gao
0 siblings, 0 replies; 6+ messages in thread
From: Changli Gao @ 2010-12-04 8:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Tom Herbert, Jiri Pirko, netdev
On Sat, Dec 4, 2010 at 4:47 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le samedi 04 décembre 2010 à 13:45 +0800, Changli Gao a écrit :
>> The dev field of ingress queue is forgot to initialized, then NULL
>> pointer dereference happens in qdisc_alloc().
>
> < deleted oops >
>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>> net/core/dev.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index cd24374..8083c68 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5577,6 +5577,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
>> queue = kzalloc(sizeof(*queue), GFP_KERNEL);
>> if (!queue)
>> return NULL;
>> + netdev_queue_numa_node_write(queue, -1);
>> + queue->dev = dev;
>> netdev_init_one_queue(dev, queue, NULL);
>> queue->qdisc = &noop_qdisc;
>> queue->qdisc_sleeping = &noop_qdisc;
>
> Hi Changli, thanks for bug report and patch.
>
> IMHO there is no point to include this long Oops report in Changelog for
> a net-next-2.6 temporary problem, there wont be any bugzilla report.
>
OK, Thanks.
> Instead, you could say it is a followup patch for commits 1d24eb4815d1e0
> and f2cd2d3e9b3ef960. Two or three lines with relevant information.
>
> I suggest you submit following patch instead, as this seems cleaner to
> me ?
>
> (factorize the two inits in netdev_init_one_queue())
>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cd24374..36e10b4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5113,7 +5113,6 @@ static int netif_alloc_netdev_queues(struct net_device *dev)
> {
> unsigned int count = dev->num_tx_queues;
> struct netdev_queue *tx;
> - int i;
>
> BUG_ON(count < 1);
>
> @@ -5125,10 +5124,6 @@ static int netif_alloc_netdev_queues(struct net_device *dev)
> }
> dev->_tx = tx;
>
> - for (i = 0; i < count; i++) {
> - netdev_queue_numa_node_write(&tx[i], -1);
> - tx[i].dev = dev;
> - }
> return 0;
> }
>
> @@ -5140,6 +5135,8 @@ static void netdev_init_one_queue(struct net_device *dev,
> spin_lock_init(&queue->_xmit_lock);
> netdev_set_xmit_lockdep_class(&queue->_xmit_lock, dev->type);
> queue->xmit_lock_owner = -1;
> + netdev_queue_numa_node_write(queue, -1);
> + queue->dev = dev;
> }
>
> static void netdev_init_queues(struct net_device *dev)
>
>
>
I thought about it before submitting. I am not sure if there are some
users of txq->dev before netdev_init_one_queue().
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] net: init ingress queue
@ 2010-12-04 12:31 Changli Gao
2010-12-04 13:36 ` Jarek Poplawski
2010-12-05 21:05 ` Eric Dumazet
0 siblings, 2 replies; 6+ messages in thread
From: Changli Gao @ 2010-12-04 12:31 UTC (permalink / raw)
To: David S. Miller
Cc: Eric Dumazet, Tom Herbert, Jiri Pirko, netdev, Changli Gao
The dev field of ingress queue is forgot to initialized, then NULL
pointer dereference happens in qdisc_alloc().
Move inits of tx queues to netif_alloc_netdev_queues().
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
v2: factorize the two inits in netdev_init_queues() and move inits of
tx queues to netif_alloc_netdev_queues().
net/core/dev.c | 35 +++++++++++++----------------------
1 file changed, 13 insertions(+), 22 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index cd24374..4a7fc32 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5109,11 +5109,21 @@ static int netif_alloc_rx_queues(struct net_device *dev)
}
#endif
+static void netdev_init_one_queue(struct net_device *dev,
+ struct netdev_queue *queue, void *_unused)
+{
+ /* Initialize queue lock */
+ spin_lock_init(&queue->_xmit_lock);
+ netdev_set_xmit_lockdep_class(&queue->_xmit_lock, dev->type);
+ queue->xmit_lock_owner = -1;
+ netdev_queue_numa_node_write(queue, -1);
+ queue->dev = dev;
+}
+
static int netif_alloc_netdev_queues(struct net_device *dev)
{
unsigned int count = dev->num_tx_queues;
struct netdev_queue *tx;
- int i;
BUG_ON(count < 1);
@@ -5125,27 +5135,10 @@ static int netif_alloc_netdev_queues(struct net_device *dev)
}
dev->_tx = tx;
- for (i = 0; i < count; i++) {
- netdev_queue_numa_node_write(&tx[i], -1);
- tx[i].dev = dev;
- }
- return 0;
-}
-
-static void netdev_init_one_queue(struct net_device *dev,
- struct netdev_queue *queue,
- void *_unused)
-{
- /* Initialize queue lock */
- spin_lock_init(&queue->_xmit_lock);
- netdev_set_xmit_lockdep_class(&queue->_xmit_lock, dev->type);
- queue->xmit_lock_owner = -1;
-}
-
-static void netdev_init_queues(struct net_device *dev)
-{
netdev_for_each_tx_queue(dev, netdev_init_one_queue, NULL);
spin_lock_init(&dev->tx_global_lock);
+
+ return 0;
}
/**
@@ -5184,8 +5177,6 @@ int register_netdevice(struct net_device *dev)
dev->iflink = -1;
- netdev_init_queues(dev);
-
/* Init, if this function is available */
if (dev->netdev_ops->ndo_init) {
ret = dev->netdev_ops->ndo_init(dev);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: init ingress queue
2010-12-04 12:31 Changli Gao
@ 2010-12-04 13:36 ` Jarek Poplawski
2010-12-05 21:05 ` Eric Dumazet
1 sibling, 0 replies; 6+ messages in thread
From: Jarek Poplawski @ 2010-12-04 13:36 UTC (permalink / raw)
To: Changli Gao
Cc: David S. Miller, Eric Dumazet, Tom Herbert, Jiri Pirko, netdev
Changli Gao wrote:
> The dev field of ingress queue is forgot to initialized, then NULL
> pointer dereference happens in qdisc_alloc().
>
> Move inits of tx queues to netif_alloc_netdev_queues().
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> v2: factorize the two inits in netdev_init_queues() and move inits of
> tx queues to netif_alloc_netdev_queues().
> net/core/dev.c | 35 +++++++++++++----------------------
> 1 file changed, 13 insertions(+), 22 deletions(-)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cd24374..4a7fc32 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
I guess you should mention it's net-next problem, because some people
might be unnecessarily concerned (why they don't have such an oops ;-)
Jarek P.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: init ingress queue
2010-12-04 12:31 Changli Gao
2010-12-04 13:36 ` Jarek Poplawski
@ 2010-12-05 21:05 ` Eric Dumazet
1 sibling, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2010-12-05 21:05 UTC (permalink / raw)
To: Changli Gao; +Cc: David S. Miller, Tom Herbert, Jiri Pirko, netdev
Le samedi 04 décembre 2010 à 20:31 +0800, Changli Gao a écrit :
> The dev field of ingress queue is forgot to initialized, then NULL
> pointer dereference happens in qdisc_alloc().
>
> Move inits of tx queues to netif_alloc_netdev_queues().
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> v2: factorize the two inits in netdev_init_queues() and move inits of
> tx queues to netif_alloc_netdev_queues().
This is a net-next-2.6 patch, you forgot to mention it.
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-12-05 21:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-04 5:45 [PATCH] net: init ingress queue Changli Gao
2010-12-04 8:47 ` Eric Dumazet
2010-12-04 8:55 ` Changli Gao
-- strict thread matches above, loose matches on Subject: below --
2010-12-04 12:31 Changli Gao
2010-12-04 13:36 ` Jarek Poplawski
2010-12-05 21:05 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).