Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1338608 > unrolled thread

[PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON

Back to article view | Back to linux.kernel

Contents

  [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON Gonglei <arei.gonglei@huawei.com> - 2016-02-20 02:30 +0100
    Re: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to  trigger BUG_ON David Miller <davem@davemloft.net> - 2016-02-20 05:40 +0100
      RE: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to  trigger BUG_ON "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-02-20 07:10 +0100
        Re: [Xen-devel] [PATCH] xen-netfront: set real_num_tx_queues to zreo  avoid to trigger BUG_ON David Vrabel <david.vrabel@citrix.com> - 2016-02-22 14:40 +0100
    Re: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to  trigger BUG_ON Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-02-20 17:00 +0100
    Re: [Xen-devel] [PATCH] xen-netfront: set real_num_tx_queues to zreo  avoid to trigger BUG_ON David Vrabel <david.vrabel@citrix.com> - 2016-02-22 15:20 +0100

#1338608 — [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON

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

[toc] | [next] | [standalone]

#1338631 — Re: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON

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.

[toc] | [prev] | [next] | [standalone]

#1338644 — RE: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON

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

[toc] | [prev] | [next] | [standalone]

#1339474 — Re: [Xen-devel] [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON

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

[toc] | [prev] | [next] | [standalone]

#1338744 — Re: [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON

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

[toc] | [prev] | [next] | [standalone]

#1339502 — Re: [Xen-devel] [PATCH] xen-netfront: set real_num_tx_queues to zreo avoid to trigger BUG_ON

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web