[ovs-dev] [PATCH v2 2/7] datapath-windows: Fix bugs in Event.c around subscribe and lock

[Alin Serdean]

I wanted to ask you about OvsWaitEventIoctl lock but you fixed in 5/7

Acked-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com>

> -----Mesaj original-----
> De la: dev [mailto:dev-bounces at openvswitch.org] În numele Sairam
> Venugopal
> Trimis: Tuesday, July 26, 2016 3:05 AM
> Către: dev at openvswitch.org
> Subiect: [ovs-dev] [PATCH v2 2/7] datapath-windows: Fix bugs in Event.c
> around subscribe and lock
> 
> When userspace tries to resubscribe to an existing queue, return
> STATUS_INVALID_PARAMETER since it's not supported. The current bug
> overwrites status to STATUS_SUCCESS.
> 
> The second bug fix is around releasing the EventQueue lock if an open
> instance couldn't be found. The current version returns back without
> releasing the lock. Moving the OvsAcquireEventQueueLock() after the
> instance is verified.
> 
> OvsGetOpenInstance does not enforce a safe read for
> gOvsSwitchContext->dpNo. Use the gOvsSwitchContext->dispatchLock for
> accessing the parameter.
> 
> v2: Address review comments from Alin Serdean and Yin Lin (around keeping
> OvsGetOpenInstance safe).
> 
> Signed-off-by: Sairam Venugopal <vsairam at vmware.com>
> ---
>  datapath-windows/ovsext/Datapath.c | 7 ++++++-
>  datapath-windows/ovsext/Event.c    | 6 ++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-
> windows/ovsext/Datapath.c
> index e4d6ab1..75f133a 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -521,12 +521,17 @@ POVS_OPEN_INSTANCE
> OvsGetOpenInstance(PFILE_OBJECT fileObject,
>                     UINT32 dpNo)
>  {
> +    LOCK_STATE_EX lockState;
>      POVS_OPEN_INSTANCE instance = (POVS_OPEN_INSTANCE)fileObject-
> >FsContext;
>      ASSERT(instance);
>      ASSERT(instance->fileObject == fileObject);
> +    NdisAcquireRWLockWrite(gOvsSwitchContext->dispatchLock, &lockState,
> + 0);
> +
>      if (gOvsSwitchContext->dpNo != dpNo) {
> -        return NULL;
> +        instance = NULL;
>      }
> +
> +    NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
>      return instance;
>  }
> 
> diff --git a/datapath-windows/ovsext/Event.c b/datapath-
> windows/ovsext/Event.c index 8c7c3ec..8ff0322 100644
> --- a/datapath-windows/ovsext/Event.c
> +++ b/datapath-windows/ovsext/Event.c
> @@ -217,9 +217,8 @@ OvsSubscribeEventIoctl(PFILE_OBJECT fileObject,
>          if (queue->mask != request->mask) {
>              status = STATUS_INVALID_PARAMETER;
>              OVS_LOG_WARN("Can not chnage mask when the queue is
> subscribed");
> +            goto done_event_subscribe;
>          }
> -        status = STATUS_SUCCESS;
> -        goto done_event_subscribe;
>      } else if (!request->subscribe && queue == NULL) {
>          status = STATUS_SUCCESS;
>          goto done_event_subscribe;
> @@ -356,8 +355,6 @@ OvsWaitEventIoctl(PIRP irp,
>      }
>      poll = (POVS_EVENT_POLL)inputBuffer;
> 
> -    OvsAcquireEventQueueLock();
> -
>      instance = OvsGetOpenInstance(fileObject, poll->dpNo);
>      if (instance == NULL) {
>          OVS_LOG_TRACE("Exit: Can not find open instance, dpNo: %d", @@ -
> 365,6 +362,7 @@ OvsWaitEventIoctl(PIRP irp,
>          return STATUS_INVALID_PARAMETER;
>      }
> 
> +    OvsAcquireEventQueueLock();
>      queue = (POVS_EVENT_QUEUE)instance->eventQueue;
>      if (queue == NULL) {
>          OVS_LOG_TRACE("Exit: Event queue does not exist");
> --
> 2.9.0.windows.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev