* [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON
@ 2016-02-20 1:27 Gonglei
2016-02-20 4:36 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Gonglei @ 2016-02-20 1:27 UTC (permalink / raw)
To: netdev, xen-devel, linux-kernel
Cc: peter.huangpeng, Gonglei, David S . Miller
It's possible for a race condition to exist between xennet_open() and
talk_to_netback(). After invoking netfront_probe() then other
threads or processes invoke xennet_open (such as NetworkManager)
immediately may trigger BUG_ON(). Besides, we also should reset
real_num_tx_queues in xennet_destroy_queues().
[ 3324.658057] kernel BUG at include/linux/netdevice.h:508!
[ 3324.658057] invalid opcode: 0000 [#1] SMP
[ 3324.658057] CPU: 0 PID: 662 Comm: NetworkManager Tainted: G
[<ffffffff810bc646>] ? raw_notifier_call_chain+0x16/0x20
[<ffffffff8166e1be>] __dev_open+0xce/0x150
[<ffffffff8166e501>] __dev_change_flags+0xa1/0x170
[<ffffffff8166e5f9>] dev_change_flags+0x29/0x70
[<ffffffff8167c49f>] do_setlink+0x39f/0xb40
[<ffffffff813c9ce2>] ? nla_parse+0x32/0x120
[<ffffffff8167d544>] rtnl_newlink+0x604/0x900
[<ffffffff8169f453>] ? netlink_unicast+0x193/0x1c0
[<ffffffff81324808>] ? security_capable+0x18/0x20
[<ffffffff810a4e9d>] ? ns_capable+0x2d/0x60
[<ffffffff8167b955>] rtnetlink_rcv_msg+0xf5/0x270
[<ffffffff813b32bd>] ? rhashtable_lookup_compare+0x5d/0xa0
[<ffffffff8167b860>] ? rtnetlink_rcv+0x40/0x40
[<ffffffff8169fc89>] netlink_rcv_skb+0xb9/0xe0
[<ffffffff8167b84c>] rtnetlink_rcv+0x2c/0x40
[<ffffffff8169f3ed>] netlink_unicast+0x12d/0x1c0
[<ffffffff8169f953>] netlink_sendmsg+0x4d3/0x630
[<ffffffff813280a2>] ? sock_has_perm+0x72/0x90
[<ffffffff8164d34f>] do_sock_sendmsg+0x9f/0xc0
[ 3324.703482] RIP [<ffffffffa0065a50>] xennet_open+0x180/0x182 [xen_netfront]
CC: David S. Miller <davem@davemloft.net>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
drivers/net/xen-netfront.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index d6abf19..da25555 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -340,7 +340,7 @@ static int xennet_open(struct net_device *dev)
unsigned int i = 0;
struct netfront_queue *queue = NULL;
- for (i = 0; i < num_queues; ++i) {
+ for (i = 0; i < num_queues && np->queues; ++i) {
queue = &np->queues[i];
napi_enable(&queue->napi);
@@ -1296,6 +1296,10 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)
np = netdev_priv(netdev);
np->xbdev = dev;
+ /* No need to use rtnl_lock() before the call below as it
+ * happens before register_netdev().
+ */
+ netdev->real_num_tx_queues = 0;
np->queues = NULL;
err = -ENOMEM;
@@ -1748,7 +1752,7 @@ static void xennet_destroy_queues(struct netfront_info *info)
del_timer_sync(&queue->rx_refill_timer);
netif_napi_del(&queue->napi);
}
-
+ info->netdev->real_num_tx_queues = 0;
rtnl_unlock();
kfree(info->queues);
@@ -1951,6 +1955,9 @@ abort_transaction_no_dev_fatal:
xennet_disconnect_backend(info);
kfree(info->queues);
info->queues = NULL;
+ rtnl_lock();
+ info->netdev->real_num_tx_queues = 0;
+ rtnl_unlock();
out:
return err;
}
--
1.8.5.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON
2016-02-20 1:27 [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON Gonglei
@ 2016-02-20 4:36 ` David Miller
2016-02-20 6:00 ` Gonglei (Arei)
2016-02-20 15:50 ` Sergei Shtylyov
2016-02-22 14:10 ` [Xen-devel] " David Vrabel
2 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2016-02-20 4:36 UTC (permalink / raw)
To: arei.gonglei; +Cc: netdev, xen-devel, linux-kernel, peter.huangpeng
From: Gonglei <arei.gonglei@huawei.com>
Date: Sat, 20 Feb 2016 09:27:26 +0800
> It's possible for a race condition to exist between xennet_open() and
> talk_to_netback(). After invoking netfront_probe() then other
> threads or processes invoke xennet_open (such as NetworkManager)
> immediately may trigger BUG_ON(). Besides, we also should reset
> real_num_tx_queues in xennet_destroy_queues().
One should really never invoke register_netdev() until the device is
%100 fully initialized.
This means you cannot call register_netdev() until it is completely
legal to invoke your ->open() method.
And I think that is what the real problem is here.
If you follow the correct rules for ordering wrt. register_netdev()
there are no "races". Because ->open() must be legally invokable
from the exact moment you call register_netdev().
I'm not applying this, as it really sounds like the fundamental issue
is the order in which the xen-netfront private data is initialized
or setup before being registered.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON
2016-02-20 4:36 ` David Miller
@ 2016-02-20 6:00 ` Gonglei (Arei)
2016-02-22 13:33 ` [Xen-devel] " David Vrabel
0 siblings, 1 reply; 6+ messages in thread
From: Gonglei (Arei) @ 2016-02-20 6:00 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, xen-devel@lists.xenproject.org,
linux-kernel@vger.kernel.org, Huangpeng (Peter)
Hi,
Thanks for rapid feedback :)
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, February 20, 2016 12:37 PM
>
> From: Gonglei <arei.gonglei@huawei.com>
> Date: Sat, 20 Feb 2016 09:27:26 +0800
>
> > It's possible for a race condition to exist between xennet_open() and
> > talk_to_netback(). After invoking netfront_probe() then other
> > threads or processes invoke xennet_open (such as NetworkManager)
> > immediately may trigger BUG_ON(). Besides, we also should reset
> > real_num_tx_queues in xennet_destroy_queues().
>
> One should really never invoke register_netdev() until the device is
> %100 fully initialized.
>
> This means you cannot call register_netdev() until it is completely
> legal to invoke your ->open() method.
>
> And I think that is what the real problem is here.
>
> If you follow the correct rules for ordering wrt. register_netdev()
> there are no "races". Because ->open() must be legally invokable
> from the exact moment you call register_netdev().
>
Yes, I agree. Though that's the historic legacy problem. ;)
> I'm not applying this, as it really sounds like the fundamental issue
> is the order in which the xen-netfront private data is initialized
> or setup before being registered.
That means register_netdev() should be invoked after xennet_connect(), right?
Regards,
-Gonglei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON
2016-02-20 1:27 [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON Gonglei
2016-02-20 4:36 ` David Miller
@ 2016-02-20 15:50 ` Sergei Shtylyov
2016-02-22 14:10 ` [Xen-devel] " David Vrabel
2 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2016-02-20 15:50 UTC (permalink / raw)
To: Gonglei, netdev, xen-devel, linux-kernel
Cc: peter.huangpeng, David S . Miller
On 02/20/2016 04:27 AM, Gonglei wrote:
> It's possible for a race condition to exist between xennet_open() and
> talk_to_netback(). After invoking netfront_probe() then other
> threads or processes invoke xennet_open (such as NetworkManager)
> immediately may trigger BUG_ON(). Besides, we also should reset
> real_num_tx_queues in xennet_destroy_queues().
>
> [ 3324.658057] kernel BUG at include/linux/netdevice.h:508!
> [ 3324.658057] invalid opcode: 0000 [#1] SMP
> [ 3324.658057] CPU: 0 PID: 662 Comm: NetworkManager Tainted: G
> [<ffffffff810bc646>] ? raw_notifier_call_chain+0x16/0x20
> [<ffffffff8166e1be>] __dev_open+0xce/0x150
> [<ffffffff8166e501>] __dev_change_flags+0xa1/0x170
> [<ffffffff8166e5f9>] dev_change_flags+0x29/0x70
> [<ffffffff8167c49f>] do_setlink+0x39f/0xb40
> [<ffffffff813c9ce2>] ? nla_parse+0x32/0x120
> [<ffffffff8167d544>] rtnl_newlink+0x604/0x900
> [<ffffffff8169f453>] ? netlink_unicast+0x193/0x1c0
> [<ffffffff81324808>] ? security_capable+0x18/0x20
> [<ffffffff810a4e9d>] ? ns_capable+0x2d/0x60
> [<ffffffff8167b955>] rtnetlink_rcv_msg+0xf5/0x270
> [<ffffffff813b32bd>] ? rhashtable_lookup_compare+0x5d/0xa0
> [<ffffffff8167b860>] ? rtnetlink_rcv+0x40/0x40
> [<ffffffff8169fc89>] netlink_rcv_skb+0xb9/0xe0
> [<ffffffff8167b84c>] rtnetlink_rcv+0x2c/0x40
> [<ffffffff8169f3ed>] netlink_unicast+0x12d/0x1c0
> [<ffffffff8169f953>] netlink_sendmsg+0x4d3/0x630
> [<ffffffff813280a2>] ? sock_has_perm+0x72/0x90
> [<ffffffff8164d34f>] do_sock_sendmsg+0x9f/0xc0
> [ 3324.703482] RIP [<ffffffffa0065a50>] xennet_open+0x180/0x182 [xen_netfront]
>
> CC: David S. Miller <davem@davemloft.net>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Full name required for this tag.
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON
2016-02-20 6:00 ` Gonglei (Arei)
@ 2016-02-22 13:33 ` David Vrabel
0 siblings, 0 replies; 6+ messages in thread
From: David Vrabel @ 2016-02-22 13:33 UTC (permalink / raw)
To: Gonglei (Arei), David Miller
Cc: netdev@vger.kernel.org, Huangpeng (Peter),
linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
On 20/02/16 06:00, Gonglei (Arei) wrote:
> Hi,
>
> Thanks for rapid feedback :)
>
>> From: David Miller [mailto:davem@davemloft.net]
>> Sent: Saturday, February 20, 2016 12:37 PM
>>
>> From: Gonglei <arei.gonglei@huawei.com>
>> Date: Sat, 20 Feb 2016 09:27:26 +0800
>>
>>> It's possible for a race condition to exist between xennet_open() and
>>> talk_to_netback(). After invoking netfront_probe() then other
>>> threads or processes invoke xennet_open (such as NetworkManager)
>>> immediately may trigger BUG_ON(). Besides, we also should reset
>>> real_num_tx_queues in xennet_destroy_queues().
>>
>> One should really never invoke register_netdev() until the device is
>> %100 fully initialized.
>>
>> This means you cannot call register_netdev() until it is completely
>> legal to invoke your ->open() method.
>>
>> And I think that is what the real problem is here.
>>
>> If you follow the correct rules for ordering wrt. register_netdev()
>> there are no "races". Because ->open() must be legally invokable
>> from the exact moment you call register_netdev().
>>
>
> Yes, I agree. Though that's the historic legacy problem. ;)
>
>> I'm not applying this, as it really sounds like the fundamental issue
>> is the order in which the xen-netfront private data is initialized
>> or setup before being registered.
>
> That means register_netdev() should be invoked after xennet_connect(), right?
No. This would mean that the network device is removed and re-added
when a guest is migrated which at best would result in considerably more
downtime (e.g., the IP address has to be renegotiated with DHCP).
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON
2016-02-20 1:27 [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON Gonglei
2016-02-20 4:36 ` David Miller
2016-02-20 15:50 ` Sergei Shtylyov
@ 2016-02-22 14:10 ` David Vrabel
2 siblings, 0 replies; 6+ messages in thread
From: David Vrabel @ 2016-02-22 14:10 UTC (permalink / raw)
To: Gonglei, netdev, xen-devel, linux-kernel
Cc: peter.huangpeng, David S . Miller
On 20/02/16 01:27, Gonglei wrote:
> It's possible for a race condition to exist between xennet_open() and
> talk_to_netback(). After invoking netfront_probe() then other
> threads or processes invoke xennet_open (such as NetworkManager)
> immediately may trigger BUG_ON(). Besides, we also should reset
> real_num_tx_queues in xennet_destroy_queues().
I think you want something like the following. We serialize the backend
changing and the netdev open with the device mutex.
(I've not even tried to compile this so it might not even build.)
David
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index d6abf19..7e2791a 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -340,22 +340,28 @@ static int xennet_open(struct net_device *dev)
unsigned int i = 0;
struct netfront_queue *queue = NULL;
+ device_lock(&np->xbdev->dev);
+
+ if (!netif_carrier_ok(dev))
+ goto unlock;
+
for (i = 0; i < num_queues; ++i) {
queue = &np->queues[i];
napi_enable(&queue->napi);
spin_lock_bh(&queue->rx_lock);
- if (netif_carrier_ok(dev)) {
- xennet_alloc_rx_buffers(queue);
- queue->rx.sring->rsp_event = queue->rx.rsp_cons + 1;
- if (RING_HAS_UNCONSUMED_RESPONSES(&queue->rx))
- napi_schedule(&queue->napi);
- }
+ xennet_alloc_rx_buffers(queue);
+ queue->rx.sring->rsp_event = queue->rx.rsp_cons + 1;
+ if (RING_HAS_UNCONSUMED_RESPONSES(&queue->rx))
+ napi_schedule(&queue->napi);
spin_unlock_bh(&queue->rx_lock);
}
netif_tx_start_all_queues(dev);
+ unlock:
+ device_unlock(&np->xbdev->dev);
+
return 0;
}
@@ -1983,8 +1989,8 @@ static int xennet_connect(struct net_device *dev)
num_queues = dev->real_num_tx_queues;
rtnl_lock();
+
netdev_update_features(dev);
- rtnl_unlock();
/*
* All public and private state should now be sane. Get
@@ -1992,7 +1998,6 @@ static int xennet_connect(struct net_device *dev)
* domain a kick because we've probably just requeued some
* packets.
*/
- netif_carrier_on(np->netdev);
for (j = 0; j < num_queues; ++j) {
queue = &np->queues[j];
@@ -2006,9 +2011,17 @@ static int xennet_connect(struct net_device *dev)
spin_lock_bh(&queue->rx_lock);
xennet_alloc_rx_buffers(queue);
+ if (netif_running(dev)) {
+ if (RING_HAS_UNCONSUMED_RESPONSES(&queue->rx))
+ napi_schedule(&queue->napi);
+ }
spin_unlock_bh(&queue->rx_lock);
}
+ netif_carrier_on(np->netdev);
+
+ rtnl_unlock();
+
return 0;
}
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index 33a31cf..57cd20d 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -204,8 +204,11 @@ void xenbus_otherend_changed(struct xenbus_watch
*watch,
return;
}
- if (drv->otherend_changed)
+ if (drv->otherend_changed) {
+ device_lock(&dev->dev);
drv->otherend_changed(dev, state);
+ device_unlock(&dev->dev);
+ }
}
EXPORT_SYMBOL_GPL(xenbus_otherend_changed);
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-22 14:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-20 1:27 [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON Gonglei
2016-02-20 4:36 ` David Miller
2016-02-20 6:00 ` Gonglei (Arei)
2016-02-22 13:33 ` [Xen-devel] " David Vrabel
2016-02-20 15:50 ` Sergei Shtylyov
2016-02-22 14:10 ` [Xen-devel] " David Vrabel
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).