| Message ID | 20160713233838.47648-5-vsairam@vmware.com |
|---|---|
| State | Superseded |
| Delegated to: | Guru Shetty |
| Headers | show |
Thanks Sai for fixing the bugs. A few questions: 1. There is also an unrelease lock in line 359, function OvsWaitEventIoctl. 2. Do we need the lock before OvsGetOpenInstance? If so, please release the lock inside "if (instance == NULL)"; if not, you should do the same thing on line 359. On Wed, Jul 13, 2016 at 4:38 PM, Sairam Venugopal <vsairam@vmware.com> wrote: > 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. > > Signed-off-by: Sairam Venugopal <vsairam@vmware.com> > --- > datapath-windows/ovsext/Event.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > 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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Hi Yin, Thanks for reviewing this. (1) has been addressed in a different patch. (2) We don¹t need the lock for OvsGetOpenInstance(). Thanks, Sairam On 7/18/16, 11:07 AM, "Yin Lin" <yinlin10@gmail.com> wrote: >Thanks Sai for fixing the bugs. A few questions: > >1. There is also an unrelease lock in line 359, function >OvsWaitEventIoctl. >2. Do we need the lock before OvsGetOpenInstance? If so, please release >the >lock inside "if (instance == NULL)"; if not, you should do the same thing >on line 359. > > > > >On Wed, Jul 13, 2016 at 4:38 PM, Sairam Venugopal <vsairam@vmware.com> >wrote: > >> 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. >> >> Signed-off-by: Sairam Venugopal <vsairam@vmware.com> >> --- >> datapath-windows/ovsext/Event.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> 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@openvswitch.org >> >>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm >>an_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r= >>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJN >>u3r80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e= >> >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma >n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc >ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJNu3r >80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e=
> -----Mesaj original----- > De la: dev [mailto:dev-bounces@openvswitch.org] În numele Sairam > Venugopal > Trimis: Monday, July 18, 2016 9:27 PM > Către: Yin Lin <yinlin10@gmail.com> > Cc: dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in Event.c > around subscribe and lock > > Hi Yin, > > Thanks for reviewing this. (1) has been addressed in a different patch. > (2) We don¹t need the lock for OvsGetOpenInstance(). [Alin Gabriel Serdean: ] we need a lock in OvsGetOpenInstance because: https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Datapath.c#L527
Hi Alin, gOvsSwitchContext->dpNo and gOvsSwitchContext in general is currently read in a lot of places without any locks being taken out. Eg: - https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc80963465b9 a5475/datapath-windows/ovsext/Datapath.c#L973 https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc80963465b9 a5475/datapath-windows/ovsext/Datapath.c#L1420 The function OvsGetOpenInstance() is currently defined in Datapath.c and only used by 2 methods in Event.c. If gOvsSwitchContext->dpNo was meant to be read after acquiring a lock, then we need to define a lock specific to gOvsSwitchContext inside OvsGetOpenInstance() and not rely on EventQueue lock to enforce it. Thanks, Sairam On 7/21/16, 6:58 AM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote: >> -----Mesaj original----- > >> De la: dev [mailto:dev-bounces@openvswitch.org] În numele Sairam > >> Venugopal > >> Trimis: Monday, July 18, 2016 9:27 PM > >> Către: Yin Lin <yinlin10@gmail.com> > >> Cc: dev@openvswitch.org > >> Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in Event.c > >> around subscribe and lock > >> > >> Hi Yin, > >> > >> Thanks for reviewing this. (1) has been addressed in a different patch. > >> (2) We don¹t need the lock for OvsGetOpenInstance(). > >[Alin Gabriel Serdean: ] we need a lock in OvsGetOpenInstance because: > >https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitc >h_ovs_blob_master_datapath-2Dwindows_ovsext_Datapath.c-23L527&d=CwIGaQ&c=S >qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dcruz40PROJ40ROzSpxyQSLw6fcrO >WpJgEcEmNR3JEQ&m=TIfmnsdkLsgajJWRZp6fyWcW_1eXVWsfuqHqHzvVhiM&s=Lb5uwUvekGK >ck_ubpcUaZVZkJMFEEgaBwQtbZVsD8vw&e= > > > >gOvsSwitchContext->dpNo
Can you refer me to the patch you fix the lock in line 359? I don't see why function OvsWaitEventIoctl is so special that we need to fix it in another patch. It is the same thing as other places you fixed so it's better to address them together.. On Mon, Jul 18, 2016 at 11:27 AM, Sairam Venugopal <vsairam@vmware.com> wrote: > Hi Yin, > > Thanks for reviewing this. (1) has been addressed in a different patch. > (2) We don¹t need the lock for OvsGetOpenInstance(). > > Thanks, > Sairam > > On 7/18/16, 11:07 AM, "Yin Lin" <yinlin10@gmail.com> wrote: > > >Thanks Sai for fixing the bugs. A few questions: > > > >1. There is also an unrelease lock in line 359, function > >OvsWaitEventIoctl. > >2. Do we need the lock before OvsGetOpenInstance? If so, please release > >the > >lock inside "if (instance == NULL)"; if not, you should do the same thing > >on line 359. > > > > > > > > > >On Wed, Jul 13, 2016 at 4:38 PM, Sairam Venugopal <vsairam@vmware.com> > >wrote: > > > >> 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. > >> > >> Signed-off-by: Sairam Venugopal <vsairam@vmware.com> > >> --- > >> datapath-windows/ovsext/Event.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> 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@openvswitch.org > >> > >> > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm > >>an_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r= > >>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJN > >>u3r80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e= > >> > >_______________________________________________ > >dev mailing list > >dev@openvswitch.org > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma > >n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc > >ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJNu3r > >80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e= > >
Hi Yin, I think you are confused about what this patch does. Or I may not be understanding this correctly. As it stands, Event.c in OVS master has 2 bugs that I identified while looking at it. 1. OvsSubscribeEventIoctl - Subscribe function doesn’t set the right status parameter when queue is subscribed 2. OvsWaitEventIoctl - Releasing an acquired lock instead of simply returning back. The 2nd bug in question is on line 359 in Event.c on master - https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc80963465b9a5475/datapath-windows/ovsext/Event.c#L359 And that’s exactly what this patch addresses. The only thing special about OvsWaitEventIoctl is that it has a bug that needed to be fixed before I modified it further with my original changes. I kept the bug fixes separate from the intended changes mainly because we can use this patch to address issues on ToT without being blocked on my other changes. Thanks, Sairam From: Yin Lin <yinlin10@gmail.com<mailto:yinlin10@gmail.com>> Date: Thursday, July 21, 2016 at 1:12 PM To: Sairam Venugopal <vsairam@vmware.com<mailto:vsairam@vmware.com>> Cc: "dev@openvswitch.org<mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org>> Subject: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in Event.c around subscribe and lock Can you refer me to the patch you fix the lock in line 359? I don't see why function OvsWaitEventIoctl is so special that we need to fix it in another patch. It is the same thing as other places you fixed so it's better to address them together.. On Mon, Jul 18, 2016 at 11:27 AM, Sairam Venugopal <vsairam@vmware.com<mailto:vsairam@vmware.com>> wrote: Hi Yin, Thanks for reviewing this. (1) has been addressed in a different patch. (2) We don¹t need the lock for OvsGetOpenInstance(). Thanks, Sairam On 7/18/16, 11:07 AM, "Yin Lin" <yinlin10@gmail.com<mailto:yinlin10@gmail.com>> wrote: >Thanks Sai for fixing the bugs. A few questions: > >1. There is also an unrelease lock in line 359, function >OvsWaitEventIoctl. >2. Do we need the lock before OvsGetOpenInstance? If so, please release >the >lock inside "if (instance == NULL)"; if not, you should do the same thing >on line 359. > > > > >On Wed, Jul 13, 2016 at 4:38 PM, Sairam Venugopal <vsairam@vmware.com<mailto:vsairam@vmware.com>> >wrote: > >> 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. >> >> Signed-off-by: Sairam Venugopal <vsairam@vmware.com<mailto:vsairam@vmware.com>> >> --- >> datapath-windows/ovsext/Event.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> 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@openvswitch.org<mailto:dev@openvswitch.org> >> >>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm >>an_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r= >>Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJN >>u3r80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e= >> >_______________________________________________ >dev mailing list >dev@openvswitch.org<mailto:dev@openvswitch.org> >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma >n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc >ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=_qUqkq3AbylbGTt0FRPDctPJYJNu3r >80SqmGYDgiNA4&s=HbFssXsaXxSrxCuEo36BQmPdk4BlTDTMpnC_OQkp_Ok&e=
The idea is dpNo could change in odd circumstances. We could use gOvsSwitchContext->dispatchLock and make OvsGetOpenInstance safe by itself. Thanks, Alin. > -----Mesaj original----- > De la: Sairam Venugopal [mailto:vsairam@vmware.com] > Trimis: Thursday, July 21, 2016 9:52 PM > Către: Alin Serdean <aserdean@cloudbasesolutions.com>; Yin Lin > <yinlin10@gmail.com> > Cc: dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in Event.c > around subscribe and lock > > Hi Alin, > > gOvsSwitchContext->dpNo and gOvsSwitchContext in general is currently > gOvsSwitchContext->read > in a lot of places without any locks being taken out. > > Eg: - > https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc8096 > 3465b9 > a5475/datapath-windows/ovsext/Datapath.c#L973 > https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc8096 > 3465b9 > a5475/datapath-windows/ovsext/Datapath.c#L1420 > > The function OvsGetOpenInstance() is currently defined in Datapath.c and > only used by 2 methods in Event.c. If gOvsSwitchContext->dpNo was meant > to be read after acquiring a lock, then we need to define a lock specific to > gOvsSwitchContext inside OvsGetOpenInstance() and not rely on > EventQueue lock to enforce it. > > Thanks, > Sairam > > > > > On 7/21/16, 6:58 AM, "Alin Serdean" <aserdean@cloudbasesolutions.com> > wrote: > > >> -----Mesaj original----- > > > >> De la: dev [mailto:dev-bounces@openvswitch.org] În numele Sairam > > > >> Venugopal > > > >> Trimis: Monday, July 18, 2016 9:27 PM > > > >> Către: Yin Lin <yinlin10@gmail.com> > > > >> Cc: dev@openvswitch.org > > > >> Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in > >> Event.c > > > >> around subscribe and lock > > > >> > > > >> Hi Yin, > > > >> > > > >> Thanks for reviewing this. (1) has been addressed in a different patch. > > > >> (2) We don¹t need the lock for OvsGetOpenInstance(). > > > >[Alin Gabriel Serdean: ] we need a lock in OvsGetOpenInstance because: > > > >https://urldefense.proofpoint.com/v2/url?u=https- > 3A__github.com_openvsw > >itc > >h_ovs_blob_master_datapath-2Dwindows_ovsext_Datapath.c- > 23L527&d=CwIGaQ& > >c=S > >qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt- > uEs&r=Dcruz40PROJ40ROzSpxyQSLw6f > >crO > >WpJgEcEmNR3JEQ&m=TIfmnsdkLsgajJWRZp6fyWcW_1eXVWsfuqHqHzvVhi > M&s=Lb5uwUve > >kGK > >ck_ubpcUaZVZkJMFEEgaBwQtbZVsD8vw&e= > > > > > > > >gOvsSwitchContext->dpNo
Yes, I was also thinking of using the gOvsSwitchContext->dispatchLock to keep it safe. That said, we would want to do the following: 1) Add this to OvsGetOpenInstance() 2) Update all unsafe usages of gOvsSwitchContext to either use OvsGetOpenInstance() or acquire gOvsSwitchContext->dispatchLock. I will file a bug for this and follow it up. Thanks, Sairam On 7/22/16, 9:57 AM, "Alin Serdean" <aserdean@cloudbasesolutions.com> wrote: >The idea is dpNo could change in odd circumstances. > > > >We could use gOvsSwitchContext->dispatchLock and make OvsGetOpenInstance >safe by itself. > > > >Thanks, > >Alin. > > > >> -----Mesaj original----- > >> De la: Sairam Venugopal [mailto:vsairam@vmware.com] > >> Trimis: Thursday, July 21, 2016 9:52 PM > >> Către: Alin Serdean <aserdean@cloudbasesolutions.com>; Yin Lin > >> <yinlin10@gmail.com> > >> Cc: dev@openvswitch.org > >> Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in Event.c > >> around subscribe and lock > >> > >> Hi Alin, > >> > >> gOvsSwitchContext->dpNo and gOvsSwitchContext in general is currently > >> gOvsSwitchContext->read > >> in a lot of places without any locks being taken out. > >> > >> Eg: - > >> >>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswit >>ch_ovs_blob_15850211ce88d540e57a6f2fc8096&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKI >>iDJAXVeAw-YihVMNtXt-uEs&r=Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=3 >>u3GI2uiqwVsds-qnZQrKxEq0hIgf5QgkFs4Fgq_9a4&s=HXq_96fjf6h_3PgWBQMt342vsEcd >>J4gwQL12akeRGao&e= > >> 3465b9 > >> a5475/datapath-windows/ovsext/Datapath.c#L973 > >> >>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswit >>ch_ovs_blob_15850211ce88d540e57a6f2fc8096&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKI >>iDJAXVeAw-YihVMNtXt-uEs&r=Dcruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=3 >>u3GI2uiqwVsds-qnZQrKxEq0hIgf5QgkFs4Fgq_9a4&s=HXq_96fjf6h_3PgWBQMt342vsEcd >>J4gwQL12akeRGao&e= > >> 3465b9 > >> a5475/datapath-windows/ovsext/Datapath.c#L1420 > >> > >> The function OvsGetOpenInstance() is currently defined in Datapath.c and > >> only used by 2 methods in Event.c. If gOvsSwitchContext->dpNo was meant > >> to be read after acquiring a lock, then we need to define a lock >>specific to > >> gOvsSwitchContext inside OvsGetOpenInstance() and not rely on > >> EventQueue lock to enforce it. > >> > >> Thanks, > >> Sairam > >> > >> > >> > >> > >> On 7/21/16, 6:58 AM, "Alin Serdean" <aserdean@cloudbasesolutions.com> > >> wrote: > >> > >> >> -----Mesaj original----- > >> > > >> >> De la: dev [mailto:dev-bounces@openvswitch.org] În numele Sairam > >> > > >> >> Venugopal > >> > > >> >> Trimis: Monday, July 18, 2016 9:27 PM > >> > > >> >> Către: Yin Lin <yinlin10@gmail.com> > >> > > >> >> Cc: dev@openvswitch.org > >> > > >> >> Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in > >> >> Event.c > >> > > >> >> around subscribe and lock > >> > > >> >> > >> > > >> >> Hi Yin, > >> > > >> >> > >> > > >> >> Thanks for reviewing this. (1) has been addressed in a different >>patch. > >> > > >> >> (2) We don¹t need the lock for OvsGetOpenInstance(). > >> > > >> >[Alin Gabriel Serdean: ] we need a lock in OvsGetOpenInstance because: > >> > > >> >https://urldefense.proofpoint.com/v2/url?u=https- > >> 3A__github.com_openvsw > >> >itc > >> >h_ovs_blob_master_datapath-2Dwindows_ovsext_Datapath.c- > >> 23L527&d=CwIGaQ& > >> >c=S > >> >qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt- > >> uEs&r=Dcruz40PROJ40ROzSpxyQSLw6f > >> >crO > >> >WpJgEcEmNR3JEQ&m=TIfmnsdkLsgajJWRZp6fyWcW_1eXVWsfuqHqHzvVhi > >> M&s=Lb5uwUve > >> >kGK > >> >ck_ubpcUaZVZkJMFEEgaBwQtbZVsD8vw&e= > >> > > >> > > >> > > >> >gOvsSwitchContext->dpNo > > >
> -----Mesaj original----- > De la: Sairam Venugopal [mailto:vsairam@vmware.com] > Trimis: Thursday, July 21, 2016 9:52 PM > Către: Alin Serdean <aserdean@cloudbasesolutions.com>; Yin Lin > <yinlin10@gmail.com> > Cc: dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in Event.c > around subscribe and lock > > Hi Alin, > > gOvsSwitchContext->dpNo and gOvsSwitchContext in general is currently > gOvsSwitchContext->read > in a lot of places without any locks being taken out. [Alin Gabriel Serdean: ] which ones? > > Eg: - > https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc8096 > 3465b9 > a5475/datapath-windows/ovsext/Datapath.c#L973 > https://github.com/openvswitch/ovs/blob/15850211ce88d540e57a6f2fc8096 > 3465b9 > a5475/datapath-windows/ovsext/Datapath.c#L1420 [Alin Gabriel Serdean: ] Those are guarded by reference counting (OvsAcquireSwitchContext/OvsReleaseSwitchContext) > > The function OvsGetOpenInstance() is currently defined in Datapath.c and > only used by 2 methods in Event.c. If gOvsSwitchContext->dpNo was meant > to be read after acquiring a lock, then we need to define a lock specific to > gOvsSwitchContext inside OvsGetOpenInstance() and not rely on > EventQueue lock to enforce it. > > Thanks, > Sairam > > > > > On 7/21/16, 6:58 AM, "Alin Serdean" <aserdean@cloudbasesolutions.com> > wrote: > > >> -----Mesaj original----- > > > >> De la: dev [mailto:dev-bounces@openvswitch.org] În numele Sairam > > > >> Venugopal > > > >> Trimis: Monday, July 18, 2016 9:27 PM > > > >> Către: Yin Lin <yinlin10@gmail.com> > > > >> Cc: dev@openvswitch.org > > > >> Subiect: Re: [ovs-dev] [PATCH 4/9] datapath-windows: Fix bugs in > >> Event.c > > > >> around subscribe and lock > > > >> > > > >> Hi Yin, > > > >> > > > >> Thanks for reviewing this. (1) has been addressed in a different patch. > > > >> (2) We don¹t need the lock for OvsGetOpenInstance(). > > > >[Alin Gabriel Serdean: ] we need a lock in OvsGetOpenInstance because: > > > >https://urldefense.proofpoint.com/v2/url?u=https- > 3A__github.com_openvsw > >itc > >h_ovs_blob_master_datapath-2Dwindows_ovsext_Datapath.c- > 23L527&d=CwIGaQ& > >c=S > >qcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt- > uEs&r=Dcruz40PROJ40ROzSpxyQSLw6f > >crO > >WpJgEcEmNR3JEQ&m=TIfmnsdkLsgajJWRZp6fyWcW_1eXVWsfuqHqHzvVhi > M&s=Lb5uwUve > >kGK > >ck_ubpcUaZVZkJMFEEgaBwQtbZVsD8vw&e= > > > > > > > >gOvsSwitchContext->dpNo
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");
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. Signed-off-by: Sairam Venugopal <vsairam@vmware.com> --- datapath-windows/ovsext/Event.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)