[Xen-devel] [GIT] xen: netback: take net_schedule_list_lock when removing entry from net_schedule_list

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

The following changes since commit adb236eb839264255954076dbc4fc40f5738f08e:
  Ian Campbell (1):
        xen: netback: increase size of rx_meta array.

are available in the git repository at:

  git://xenbits.xen.org/people/ianc/linux-2.6.git for-jeremy/netback

Ian Campbell (1):
      xen: netback: take net_schedule_list_lock when removing entry from 
net_schedule_list

 drivers/xen/netback/netback.c |   35 ++++++++++++++++++++++++++++-------
 1 files changed, 28 insertions(+), 7 deletions(-)

Ian.

>From 3af2f5d05b1236d11e952152ac1f505e6b0e8935 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@xxxxxxxxxx>
Date: Fri, 15 Oct 2010 13:41:44 +0100
Subject: [PATCH] xen: netback: take net_schedule_list_lock when removing entry 
from net_schedule_list

There is a race in net_tx_build_mops between checking if
net_schedule_list is empty and actually dequeuing the first entry on
the list. If another thread dequeues the only entry on the list during
this window we crash because list_first_entry expects a non-empty
list, like so:

[ 0.133127] BUG: unable to handle kernel NULL pointer dereference at 00000008
[ 0.133132] IP: [<c12aae71>] net_tx_build_mops+0x91/0xa70
[ 0.133142] *pdpt = 0000000000000000 *pde = 000000000000000f
[ 0.133147] Oops: 0002 1 SMP
[ 0.133150] last sysfs file:
[ 0.133152] Modules linked in:
[ 0.133154]
[ 0.133156] Pid: 55, comm: netback/1 Not tainted (2.6.32.12-0.7.1 #1) Latitude 
E4310
[ 0.133158] EIP: 0061:[<c12aae71>] EFLAGS: 00010202 CPU: 1
[ 0.133161] EIP is at net_tx_build_mops+0x91/0xa70
[ 0.133163] EAX: 00000012 EBX: 00000008 ECX: e112b734 EDX: e112b76c
[ 0.133165] ESI: ffffff30 EDI: 00000000 EBP: e112b734 ESP: dfe85d98
[ 0.133167] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0069
[ 0.133169] Process netback/1 (pid: 55, ti=dfe84000 task=dfe83340 
task.ti=dfe84000)
[ 0.133170] Stack:
[ 0.133172] 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
[ 0.133177] <0> 00000000 e112b734 e112ec08 e112b7f8 e112ec08 ffffff30 00000000 
00000000
[ 0.133186] <0> 00000000 00000000 00000000 e112b76c dfe85df4 00000001 00000000 
aaaaaaaa
[ 0.133193] Call Trace:
[ 0.133202] [<c12abc7f>] net_tx_action+0x42f/0xac0
[ 0.133206] [<c12ac37a>] netbk_action_thread+0x6a/0x1b0
[ 0.133212] [<c1057444>] kthread+0x74/0x80
[ 0.133218] [<c10049d7>] kernel_thread_helper+0x7/0x10
[ 0.133220] Code: c4 00 00 00 89 74 24 58 39 74 24 2c 0f 84 c7 06 00 00 8b 74 
24 \
                  58 8b 5c 24 58 81 ee d0 00 00 00 83 c3 08 89 74 24 34 8b 7c 
24 \
             58 <f0> ff 47 08 89 f0 e8 b4 f9 ff ff 8b 46 2c 8b 56 34 89 44 24 5c
[ 0.133261] EIP: [<c12aae71>] net_tx_build_mops+0x91/0xa70 SS:ESP 0069:dfe85d98
[ 0.133265] CR2: 0000000000000008
[ 0.133274] --[ end trace e2c5c15f54bd9d93 ]--

Therefore after the initial lock free check for an empty list check
again with the lock held before dequeueing the entry.

Based on a patch by Tomasz Wroblewski.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: Tomasz Wroblewski <tomasz.wroblewski@xxxxxxxxxx>
---
 drivers/xen/netback/netback.c |   35 ++++++++++++++++++++++++++++-------
 1 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index 9052895..c448675 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -785,15 +785,34 @@ static int __on_net_schedule_list(struct xen_netif *netif)
        return !list_empty(&netif->list);
 }
 
+/* Must be called with net_schedule_list_lock held */
 static void remove_from_net_schedule_list(struct xen_netif *netif)
 {
-       struct xen_netbk *netbk = &xen_netbk[netif->group];
-       spin_lock_irq(&netbk->net_schedule_list_lock);
        if (likely(__on_net_schedule_list(netif))) {
                list_del_init(&netif->list);
                netif_put(netif);
        }
+}
+
+static struct xen_netif *poll_net_schedule_list(struct xen_netbk *netbk)
+{
+       struct xen_netif *netif = NULL;
+
+       spin_lock_irq(&netbk->net_schedule_list_lock);
+       if (list_empty(&netbk->net_schedule_list))
+               goto out;
+
+       netif = list_first_entry(&netbk->net_schedule_list,
+                                struct xen_netif, list);
+       if (!netif)
+               goto out;
+
+       netif_get(netif);
+
+       remove_from_net_schedule_list(netif);
+out:
        spin_unlock_irq(&netbk->net_schedule_list_lock);
+       return netif;
 }
 
 static void add_to_net_schedule_list_tail(struct xen_netif *netif)
@@ -828,7 +847,10 @@ void netif_schedule_work(struct xen_netif *netif)
 
 void netif_deschedule_work(struct xen_netif *netif)
 {
+       struct xen_netbk *netbk = &xen_netbk[netif->group];
+       spin_lock_irq(&netbk->net_schedule_list_lock);
        remove_from_net_schedule_list(netif);
+       spin_unlock_irq(&netbk->net_schedule_list_lock);
 }
 
 
@@ -1312,12 +1334,11 @@ static unsigned net_tx_build_mops(struct xen_netbk 
*netbk)
                int work_to_do;
                unsigned int data_len;
                pending_ring_idx_t index;
-       
+
                /* Get a netif from the list with work to do. */
-               netif = list_first_entry(&netbk->net_schedule_list,
-                               struct xen_netif, list);
-               netif_get(netif);
-               remove_from_net_schedule_list(netif);
+               netif = poll_net_schedule_list(netbk);
+               if (!netif)
+                       continue;
 
                RING_FINAL_CHECK_FOR_REQUESTS(&netif->tx, work_to_do);
                if (!work_to_do) {
-- 
1.5.6.5




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel