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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@xxxxxxxxxx]
> Sent: 30 May 2017 15:04
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Subject: [PATCH 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        | 82
> +++++++++++++++++++++++++++++++++++++++-------
>  src/xenbus/store.c         | 11 +++++++
>  4 files changed, 128 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..e39a7a7 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)Channel->Count - (LONG64)Count <= 0)

Oops... that logic is backwards. I'll send v2.

  Paul

>              break;
> 
>          if (Timeout != NULL) {
> @@ -950,7 +960,6 @@ EvtchnWait(
>          }
> 
>          _mm_pause();
> -        KeMemoryBarrier();
>      }
> 
>      KeLowerIrql(Irql);
> @@ -958,6 +967,23 @@ EvtchnWait(
>      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 +1395,8 @@ EvtchnDebugCallback(
> 
>              XENBUS_DEBUG(Printf,
>                           &Context->DebugInterface,
> -                         "Events = %lu\n",
> -                         Channel->Events);
> +                         "Count = %lu\n",
> +                         Channel->Count);
>          }
>      }
>  }
> @@ -1655,7 +1681,7 @@ static struct _XENBUS_EVTCHN_INTERFACE_V5
> EvtchnInterfaceVersion5 = {
>      EvtchnUnmask,
>      EvtchnSendVersion1,
>      EvtchnTrigger,
> -    EvtchnWait,
> +    EvtchnWaitVersion5,
>      EvtchnGetPort,
>      EvtchnClose,
>  };
> @@ -1669,6 +1695,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 +1874,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