[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[win-pv-devel] [PATCH v2 2/2] Update XENBUS_EVTCHN Wait method to avoid races



Use of the XENBUS_EVTCHN method is prone to races in that a client
usually wants to check that something that should be triggered by an
event and, if it has not occurred, wait for the next event.
The problem is that the client may makes its check, the event then occurs,
and then the client waits. Thus the event is missed and the client only
wakes up when the timeout expires.

This patch changes the Wait method to take an explicit event count to wait
for, and adds a method to sample the current event count. A client can
then avoid a race as described above by sampling the event count first,
making its check and then waiting for the event count to increment by one.
If the event occurred between the check and the wait, the wait will now
return immediately.

Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
---
 include/evtchn_interface.h | 49 +++++++++++++++++++++++---
 include/revision.h         |  3 +-
 src/xenbus/evtchn.c        | 87 +++++++++++++++++++++++++++++++++++++++-------
 src/xenbus/store.c         | 11 ++++++
 4 files changed, 133 insertions(+), 17 deletions(-)

diff --git a/include/evtchn_interface.h b/include/evtchn_interface.h
index 4e696c8..310850b 100644
--- a/include/evtchn_interface.h
+++ b/include/evtchn_interface.h
@@ -175,17 +175,39 @@ typedef VOID
     IN  PXENBUS_EVTCHN_CHANNEL  Channel
     );
 
+/*! \typedef XENBUS_EVTCHN_GET_COUNT
+    \brief Get the number of events received by the channel since it was opened
+
+    \param Interface The interface header
+    \param Channel The channel handle
+    \return The number of events
+*/
+typedef ULONG
+(*XENBUS_EVTCHN_GET_COUNT)(
+    IN  PINTERFACE              Interface,
+    IN  PXENBUS_EVTCHN_CHANNEL  Channel
+    );
+
+typedef NTSTATUS
+(*XENBUS_EVTCHN_WAIT_V5)(
+    IN  PINTERFACE              Interface,
+    IN  PXENBUS_EVTCHN_CHANNEL  Channel,
+    IN  PLARGE_INTEGER          Timeout OPTIONAL
+    );
+
 /*! \typedef XENBUS_EVTCHN_WAIT
-    \brief Wait for an event to the local end of the channel
+    \brief Wait for events to the local end of the channel
 
     \param Interface The interface header
     \param Channel The channel handle
+    \param Count The event count to wait for
     \param Timeout An optional timeout value (similar to 
KeWaitForSingleObject(), but non-zero values are allowed at DISPATCH_LEVEL).
 */
 typedef NTSTATUS
 (*XENBUS_EVTCHN_WAIT)(
     IN  PINTERFACE              Interface,
     IN  PXENBUS_EVTCHN_CHANNEL  Channel,
+    IN  ULONG                   Count,
     IN  PLARGE_INTEGER          Timeout OPTIONAL
     );
 
@@ -248,7 +270,7 @@ struct _XENBUS_EVTCHN_INTERFACE_V5 {
     XENBUS_EVTCHN_UNMASK    EvtchnUnmask;
     XENBUS_EVTCHN_SEND_V1   EvtchnSendVersion1;
     XENBUS_EVTCHN_TRIGGER   EvtchnTrigger;
-    XENBUS_EVTCHN_WAIT      EvtchnWait;
+    XENBUS_EVTCHN_WAIT_V5   EvtchnWaitVersion5;
     XENBUS_EVTCHN_GET_PORT  EvtchnGetPort;
     XENBUS_EVTCHN_CLOSE     EvtchnClose;
 };
@@ -266,12 +288,31 @@ struct _XENBUS_EVTCHN_INTERFACE_V6 {
     XENBUS_EVTCHN_UNMASK    EvtchnUnmask;
     XENBUS_EVTCHN_SEND      EvtchnSend;
     XENBUS_EVTCHN_TRIGGER   EvtchnTrigger;
+    XENBUS_EVTCHN_WAIT_V5   EvtchnWaitVersion5;
+    XENBUS_EVTCHN_GET_PORT  EvtchnGetPort;
+    XENBUS_EVTCHN_CLOSE     EvtchnClose;
+};
+
+/*! \struct _XENBUS_EVTCHN_INTERFACE_V7
+    \brief EVTCHN interface version 7
+    \ingroup interfaces
+*/
+struct _XENBUS_EVTCHN_INTERFACE_V7 {
+    INTERFACE               Interface;
+    XENBUS_EVTCHN_ACQUIRE   EvtchnAcquire;
+    XENBUS_EVTCHN_RELEASE   EvtchnRelease;
+    XENBUS_EVTCHN_OPEN      EvtchnOpen;
+    XENBUS_EVTCHN_BIND      EvtchnBind;
+    XENBUS_EVTCHN_UNMASK    EvtchnUnmask;
+    XENBUS_EVTCHN_SEND      EvtchnSend;
+    XENBUS_EVTCHN_TRIGGER   EvtchnTrigger;
+    XENBUS_EVTCHN_GET_COUNT EvtchnGetCount;
     XENBUS_EVTCHN_WAIT      EvtchnWait;
     XENBUS_EVTCHN_GET_PORT  EvtchnGetPort;
     XENBUS_EVTCHN_CLOSE     EvtchnClose;
 };
 
-typedef struct _XENBUS_EVTCHN_INTERFACE_V6 XENBUS_EVTCHN_INTERFACE, 
*PXENBUS_EVTCHN_INTERFACE;
+typedef struct _XENBUS_EVTCHN_INTERFACE_V7 XENBUS_EVTCHN_INTERFACE, 
*PXENBUS_EVTCHN_INTERFACE;
 
 /*! \def XENBUS_EVTCHN
     \brief Macro at assist in method invocation
@@ -282,7 +323,7 @@ typedef struct _XENBUS_EVTCHN_INTERFACE_V6 
XENBUS_EVTCHN_INTERFACE, *PXENBUS_EVT
 #endif  // _WINDLL
 
 #define XENBUS_EVTCHN_INTERFACE_VERSION_MIN 4
-#define XENBUS_EVTCHN_INTERFACE_VERSION_MAX 6
+#define XENBUS_EVTCHN_INTERFACE_VERSION_MAX 7
 
 #endif  // _XENBUS_EVTCHN_INTERFACE_H
 
diff --git a/include/revision.h b/include/revision.h
index 2aa7dc5..bc73be2 100644
--- a/include/revision.h
+++ b/include/revision.h
@@ -51,6 +51,7 @@
     DEFINE_REVISION(0x0800000A,  1,  2,  5,  1,  1,  1,  1,  1,  1,  0,  1), \
     DEFINE_REVISION(0x0800000B,  1,  2,  5,  1,  2,  1,  1,  2,  1,  0,  1), \
     DEFINE_REVISION(0x09000000,  1,  2,  5,  1,  2,  1,  1,  2,  1,  0,  1), \
-    DEFINE_REVISION(0x09000001,  1,  2,  6,  1,  2,  1,  1,  2,  1,  1,  1)
+    DEFINE_REVISION(0x09000001,  1,  2,  6,  1,  2,  1,  1,  2,  1,  1,  1), \
+    DEFINE_REVISION(0x09000002,  1,  2,  7,  1,  2,  1,  1,  2,  1,  1,  1)
 
 #endif  // _REVISION_H
diff --git a/src/xenbus/evtchn.c b/src/xenbus/evtchn.c
index 7546f19..f9f95c9 100644
--- a/src/xenbus/evtchn.c
+++ b/src/xenbus/evtchn.c
@@ -81,7 +81,7 @@ struct _XENBUS_EVTCHN_CHANNEL {
     PKSERVICE_ROUTINE           Callback;
     PVOID                       Argument;
     BOOLEAN                     Active; // Must be tested at >= DISPATCH_LEVEL
-    ULONG                       Events;
+    ULONG                       Count;
     XENBUS_EVTCHN_TYPE          Type;
     XENBUS_EVTCHN_PARAMETERS    Parameters;
     BOOLEAN                     Mask;
@@ -401,7 +401,7 @@ EvtchnReap(
 
     Trace("%u\n", LocalPort);
 
-    Channel->Events = 0;
+    Channel->Count = 0;
 
     ASSERT(Channel->Closed);
     Channel->Closed = FALSE;
@@ -507,7 +507,7 @@ EvtchnPoll(
 
         KeMemoryBarrier();
         if (!Channel->Closed) {
-            Channel->Events++;
+            Channel->Count++;
 
             RemoveEntryList(&Channel->PendingListEntry);
             InitializeListHead(&Channel->PendingListEntry);
@@ -898,15 +898,26 @@ EvtchnGetPort(
     return Channel->LocalPort;
 }
 
+static ULONG
+EvtchnGetCount(
+    IN  PINTERFACE              Interface,
+    IN  PXENBUS_EVTCHN_CHANNEL  Channel
+    )
+{
+    UNREFERENCED_PARAMETER(Interface);
+
+    return Channel->Count;
+}
+
 static NTSTATUS
 EvtchnWait(
     IN  PINTERFACE              Interface,
     IN  PXENBUS_EVTCHN_CHANNEL  Channel,
+    IN  ULONG                   Count,
     IN  PLARGE_INTEGER          Timeout
     )
 {
     KIRQL                       Irql;
-    ULONG                       Events;
     LARGE_INTEGER               Start;
     NTSTATUS                    status;
 
@@ -915,14 +926,13 @@ EvtchnWait(
     ASSERT3U(KeGetCurrentIrql(), <=, DISPATCH_LEVEL);
     KeRaiseIrql(DISPATCH_LEVEL, &Irql); // Prevent suspend
 
-    Events = Channel->Events;
-    KeMemoryBarrier();
-
     KeQuerySystemTime(&Start);
 
     for (;;) {
+        KeMemoryBarrier();
+
         status = STATUS_SUCCESS;
-        if (Channel->Events != Events)
+        if ((LONG64)Count - (LONG64)Channel->Count <= 0)
             break;
 
         if (Timeout != NULL) {
@@ -950,14 +960,35 @@ EvtchnWait(
         }
 
         _mm_pause();
-        KeMemoryBarrier();
     }
 
+    if (status == STATUS_TIMEOUT)
+        Info("TIMED OUT: Count = %08x Channel->Count = %08x\n",
+             Count,
+             Channel->Count);
+
     KeLowerIrql(Irql);
 
     return status;
 }
 
+static NTSTATUS
+EvtchnWaitVersion5(
+    IN  PINTERFACE              Interface,
+    IN  PXENBUS_EVTCHN_CHANNEL  Channel,
+    IN  PLARGE_INTEGER          Timeout
+    )
+{
+    ULONG                       Count;
+
+    Count = EvtchnGetCount(Interface, Channel);
+
+    return EvtchnWait(Interface,
+                      Channel,
+                      Count + 1,
+                      Timeout);
+}
+
 static
 _Function_class_(KSERVICE_ROUTINE)
 __drv_requiresIRQL(HIGH_LEVEL)
@@ -1369,8 +1400,8 @@ EvtchnDebugCallback(
 
             XENBUS_DEBUG(Printf,
                          &Context->DebugInterface,
-                         "Events = %lu\n",
-                         Channel->Events);
+                         "Count = %lu\n",
+                         Channel->Count);
         }
     }
 }
@@ -1655,7 +1686,7 @@ static struct _XENBUS_EVTCHN_INTERFACE_V5 
EvtchnInterfaceVersion5 = {
     EvtchnUnmask,
     EvtchnSendVersion1,
     EvtchnTrigger,
-    EvtchnWait,
+    EvtchnWaitVersion5,
     EvtchnGetPort,
     EvtchnClose,
 };
@@ -1669,6 +1700,21 @@ static struct _XENBUS_EVTCHN_INTERFACE_V6 
EvtchnInterfaceVersion6 = {
     EvtchnUnmask,
     EvtchnSend,
     EvtchnTrigger,
+    EvtchnWaitVersion5,
+    EvtchnGetPort,
+    EvtchnClose,
+};
+
+static struct _XENBUS_EVTCHN_INTERFACE_V7 EvtchnInterfaceVersion7 = {
+    { sizeof (struct _XENBUS_EVTCHN_INTERFACE_V7), 7, NULL, NULL, NULL },
+    EvtchnAcquire,
+    EvtchnRelease,
+    EvtchnOpen,
+    EvtchnBind,
+    EvtchnUnmask,
+    EvtchnSend,
+    EvtchnTrigger,
+    EvtchnGetCount,
     EvtchnWait,
     EvtchnGetPort,
     EvtchnClose,
@@ -1833,6 +1879,23 @@ EvtchnGetInterface(
         status = STATUS_SUCCESS;
         break;
     }
+    case 7: {
+        struct _XENBUS_EVTCHN_INTERFACE_V7  *EvtchnInterface;
+
+        EvtchnInterface = (struct _XENBUS_EVTCHN_INTERFACE_V7 *)Interface;
+
+        status = STATUS_BUFFER_OVERFLOW;
+        if (Size < sizeof (struct _XENBUS_EVTCHN_INTERFACE_V7))
+            break;
+
+        *EvtchnInterface = EvtchnInterfaceVersion7;
+
+        ASSERT3U(Interface->Version, ==, Version);
+        Interface->Context = Context;
+
+        status = STATUS_SUCCESS;
+        break;
+    }
     default:
         status = STATUS_NOT_SUPPORTED;
         break;
diff --git a/src/xenbus/store.c b/src/xenbus/store.c
index 86bbcb8..101dc40 100644
--- a/src/xenbus/store.c
+++ b/src/xenbus/store.c
@@ -900,6 +900,7 @@ StoreSubmitRequest(
 {
     PXENBUS_STORE_RESPONSE      Response;
     KIRQL                       Irql;
+    ULONG                       Count;
     LARGE_INTEGER               Timeout;
 
     ASSERT3U(Request->State, ==, XENBUS_STORE_REQUEST_PREPARED);
@@ -913,6 +914,11 @@ StoreSubmitRequest(
     InsertTailList(&Context->SubmittedList, &Request->ListEntry);
 
     Request->State = XENBUS_STORE_REQUEST_SUBMITTED;
+
+    Count = XENBUS_EVTCHN(GetCount,
+                          &Context->EvtchnInterface,
+                          Context->Channel);
+
     StorePollLocked(Context);
     KeMemoryBarrier();
 
@@ -924,10 +930,15 @@ StoreSubmitRequest(
         status = XENBUS_EVTCHN(Wait,
                                &Context->EvtchnInterface,
                                Context->Channel,
+                               Count + 1,
                                &Timeout);
         if (status == STATUS_TIMEOUT)
             Warning("TIMED OUT\n");
 
+        Count = XENBUS_EVTCHN(GetCount,
+                              &Context->EvtchnInterface,
+                              Context->Channel);
+
         StorePollLocked(Context);
         KeMemoryBarrier();
     }
-- 
2.5.3


_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.