| Message ID | 20190227140303.57652-1-aserdean@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] datapath-windows: Guard vport usage in user.c | expand |
Do we need to keep the dispatchLock for reading vport structures? If that's the case, then we will need to fix it in other areas of the code too. It would be better to move the locking inside the relevant vport.c functions instead of taking out global ones.
Thanks,
Sairam
On 2/27/19, 6:10 AM, "ovs-dev-bounces@openvswitch.org on behalf of Alin Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of aserdean@ovn.org> wrote:
When using a vport we need to guard its usage with the dispatch lock.
Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org>
---
datapath-windows/ovsext/User.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
index b43d7cc04..ed1fcbea8 100644
--- a/datapath-windows/ovsext/User.c
+++ b/datapath-windows/ovsext/User.c
@@ -452,14 +452,6 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
}
fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl);
- vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort);
- if (vport) {
- fwdDetail->SourcePortId = vport->portId;
- fwdDetail->SourceNicIndex = vport->nicIndex;
- } else {
- fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID;
- fwdDetail->SourceNicIndex = 0;
- }
// XXX: Figure out if any of the other members of fwdDetail need to be set.
status = OvsGetFlowMetadata(&key, execute->keyAttrs);
@@ -502,6 +494,14 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
if (ndisStatus == NDIS_STATUS_SUCCESS) {
NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0);
+ vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort);
+ if (vport) {
+ fwdDetail->SourcePortId = vport->portId;
+ fwdDetail->SourceNicIndex = vport->nicIndex;
+ } else {
+ fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID;
+ fwdDetail->SourceNicIndex = 0;
+ }
ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl,
vport ? vport->portNo :
OVS_DPPORT_NUMBER_INVALID,
--
2.16.1.windows.1
_______________________________________________
dev mailing list
dev@openvswitch.org
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cvsairam%40vmware.com%7C0bd42693441d43f12bfa08d69cbd589d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636868734400091268&sdata=ShTAK8Qyi4hL3EG8PgLzbIxzSMril0w%2BHEE8VO6qWlU%3D&reserved=0
> Do we need to keep the dispatchLock for reading vport structures? [Alin Serdean] We need to make sure that port is not deleted since we use it for processing. >If that's > the case, then we will need to fix it in other areas of the code too. [Alin Serdean] I sent out https://patchwork.ozlabs.org/patch/1049043/ so we can easily see where we need to change it. > It would > be better to move the locking inside the relevant vport.c functions instead of > taking out global ones. [Alin Serdean] That would be ideal, but unfortunately if the vport is used afterwards it needs to be guarded. > > Thanks, > Sairam > > On 2/27/19, 6:10 AM, "ovs-dev-bounces@openvswitch.org on behalf of Alin > Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of > aserdean@ovn.org> wrote: > > When using a vport we need to guard its usage with the dispatch lock. > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > ---
Acked-by: Anand Kumar <kumaranand@vmware.com> Thanks, Anand Kumar On 2/27/19, 6:10 AM, "ovs-dev-bounces@openvswitch.org on behalf of Alin Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of aserdean@ovn.org> wrote: When using a vport we need to guard its usage with the dispatch lock. Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- datapath-windows/ovsext/User.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c index b43d7cc04..ed1fcbea8 100644 --- a/datapath-windows/ovsext/User.c +++ b/datapath-windows/ovsext/User.c @@ -452,14 +452,6 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) } fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl); - vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); - if (vport) { - fwdDetail->SourcePortId = vport->portId; - fwdDetail->SourceNicIndex = vport->nicIndex; - } else { - fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; - fwdDetail->SourceNicIndex = 0; - } // XXX: Figure out if any of the other members of fwdDetail need to be set. status = OvsGetFlowMetadata(&key, execute->keyAttrs); @@ -502,6 +494,14 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) if (ndisStatus == NDIS_STATUS_SUCCESS) { NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0); + vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); + if (vport) { + fwdDetail->SourcePortId = vport->portId; + fwdDetail->SourceNicIndex = vport->nicIndex; + } else { + fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; + fwdDetail->SourceNicIndex = 0; + } ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl, vport ? vport->portNo : OVS_DPPORT_NUMBER_INVALID, -- 2.16.1.windows.1 _______________________________________________ dev mailing list dev@openvswitch.org https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Ckumaranand%40vmware.com%7C0bd42693441d43f12bfa08d69cbd589d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636868734386921985&sdata=KCb%2FeQOwJIyXj6C7ZS9QePNUW6CZp7CwBWJ1obSHMvA%3D&reserved=0
Acked-by: Sairam Venugopal <vsairam@vmware.com> On 2/27/19, 6:10 AM, "ovs-dev-bounces@openvswitch.org on behalf of Alin Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of aserdean@ovn.org> wrote: When using a vport we need to guard its usage with the dispatch lock. Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- datapath-windows/ovsext/User.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c index b43d7cc04..ed1fcbea8 100644 --- a/datapath-windows/ovsext/User.c +++ b/datapath-windows/ovsext/User.c @@ -452,14 +452,6 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) } fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl); - vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); - if (vport) { - fwdDetail->SourcePortId = vport->portId; - fwdDetail->SourceNicIndex = vport->nicIndex; - } else { - fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; - fwdDetail->SourceNicIndex = 0; - } // XXX: Figure out if any of the other members of fwdDetail need to be set. status = OvsGetFlowMetadata(&key, execute->keyAttrs); @@ -502,6 +494,14 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) if (ndisStatus == NDIS_STATUS_SUCCESS) { NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0); + vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); + if (vport) { + fwdDetail->SourcePortId = vport->portId; + fwdDetail->SourceNicIndex = vport->nicIndex; + } else { + fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; + fwdDetail->SourceNicIndex = 0; + } ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl, vport ? vport->portNo : OVS_DPPORT_NUMBER_INVALID, -- 2.16.1.windows.1 _______________________________________________ dev mailing list dev@openvswitch.org https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cvsairam%40vmware.com%7C0bd42693441d43f12bfa08d69cbd589d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636868734400091268&sdata=ShTAK8Qyi4hL3EG8PgLzbIxzSMril0w%2BHEE8VO6qWlU%3D&reserved=0
Applied on master and branch-2.11 > -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele Sairam Venugopal via dev > Trimis: Tuesday, March 12, 2019 8:20 PM > Către: Alin Gabriel Serdean <aserdean@ovn.org>; dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Guard vport usage in > user.c > > Acked-by: Sairam Venugopal <vsairam@vmware.com> > > On 2/27/19, 6:10 AM, "ovs-dev-bounces@openvswitch.org on behalf of Alin > Gabriel Serdean" <ovs-dev-bounces@openvswitch.org on behalf of > aserdean@ovn.org> wrote: > > When using a vport we need to guard its usage with the dispatch lock. > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > --- > datapath-windows/ovsext/User.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/datapath-windows/ovsext/User.c b/datapath- > windows/ovsext/User.c > index b43d7cc04..ed1fcbea8 100644 > --- a/datapath-windows/ovsext/User.c > +++ b/datapath-windows/ovsext/User.c > @@ -452,14 +452,6 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) > } > > fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl); > - vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); > - if (vport) { > - fwdDetail->SourcePortId = vport->portId; > - fwdDetail->SourceNicIndex = vport->nicIndex; > - } else { > - fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; > - fwdDetail->SourceNicIndex = 0; > - } > // XXX: Figure out if any of the other members of fwdDetail need to be > set. > > status = OvsGetFlowMetadata(&key, execute->keyAttrs); > @@ -502,6 +494,14 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) > > if (ndisStatus == NDIS_STATUS_SUCCESS) { > NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, > &lockState, 0); > + vport = OvsFindVportByPortNo(gOvsSwitchContext, execute- > >inPort); > + if (vport) { > + fwdDetail->SourcePortId = vport->portId; > + fwdDetail->SourceNicIndex = vport->nicIndex; > + } else { > + fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; > + fwdDetail->SourceNicIndex = 0; > + } > ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl, > vport ? vport->portNo : > OVS_DPPORT_NUMBER_INVALID, > -- > 2.16.1.windows.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.o > penvswitch.org%2Fmailman%2Flistinfo%2Fovs- > dev&data=02%7C01%7Cvsairam%40vmware.com%7C0bd42693441d43f1 > 2bfa08d69cbd589d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C6 > 36868734400091268&sdata=ShTAK8Qyi4hL3EG8PgLzbIxzSMril0w%2BHEE > 8VO6qWlU%3D&reserved=0 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c index b43d7cc04..ed1fcbea8 100644 --- a/datapath-windows/ovsext/User.c +++ b/datapath-windows/ovsext/User.c @@ -452,14 +452,6 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) } fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl); - vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); - if (vport) { - fwdDetail->SourcePortId = vport->portId; - fwdDetail->SourceNicIndex = vport->nicIndex; - } else { - fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; - fwdDetail->SourceNicIndex = 0; - } // XXX: Figure out if any of the other members of fwdDetail need to be set. status = OvsGetFlowMetadata(&key, execute->keyAttrs); @@ -502,6 +494,14 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute) if (ndisStatus == NDIS_STATUS_SUCCESS) { NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0); + vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); + if (vport) { + fwdDetail->SourcePortId = vport->portId; + fwdDetail->SourceNicIndex = vport->nicIndex; + } else { + fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; + fwdDetail->SourceNicIndex = 0; + } ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl, vport ? vport->portNo : OVS_DPPORT_NUMBER_INVALID,
When using a vport we need to guard its usage with the dispatch lock. Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- datapath-windows/ovsext/User.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)