Re: [ovs-dev] [PATCH] datapath: Fix for force/commit action failures

2017-07-13 Thread Greg Rose 

On 07/13/2017 10:08 AM, Darrell Ball wrote:



On 7/13/17, 9:25 AM, "ovs-dev-boun...@openvswitch.org on behalf of Greg Rose" 
 wrote:

 When there is an established connection in direction A->B, it is
 possible to receive a packet on port B which then executes
 ct(commit,force) without first performing ct() - ie, a lookup.
 In this case, we would expect that this packet can delete the existing
 entry so that we can commit a connection with direction B->A. However,
 currently we only perform a check in skb_nfct_cached() for whether
 OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
 lookup previously occurred. In the above scenario, a lookup has not
 occurred but we should still be able to statelessly look up the
 existing entry and potentially delete the entry if it is in the
 opposite direction.
 
 This patch extends the check to also hint that if the action has the

 force flag set, then we will lookup the existing entry so that the
 force check at the end of skb_nfct_cached has the ability to delete
 the connection.
 
 CC: d...@openvswitch.org

 CC: Pravin Shalar 
 Signed-off-by: Joe Stringer 
 Signed-off-by: Greg Rose 
 ---
  net/openvswitch/conntrack.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)
 
 diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c

 index 08679eb..9041cf8 100644
 --- a/net/openvswitch/conntrack.c
 +++ b/net/openvswitch/conntrack.c
 @@ -641,17 +641,21 @@ static bool skb_nfct_cached(struct net *net,
ct = nf_ct_get(skb, &ctinfo);
/* If no ct, check if we have evidence that an existing conntrack entry
 * might be found for this skb.  This happens when we lose a skb->_nfct
 -   * due to an upcall.  If the connection was not confirmed, it is not
 -   * cached and needs to be run through conntrack again.
 +   * due to an upcall, or if the direction is being forced.  If the
 +   * connection was not confirmed, it is not cached and needs to be run
 +   * through conntrack again.
 */
 -  if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
 +  if ((!ct && (key->ct_state & OVS_CS_F_TRACKED) &&
!(key->ct_state & OVS_CS_F_INVALID) &&
 -  key->ct_zone == info->zone.id) {
 +   key->ct_zone == info->zone.id) ||
 +   (!key->ct_state && info->force)) {
ct = ovs_ct_find_existing(net, &info->zone, info->family, skb,
  !!(key->ct_state
 & OVS_CS_F_NAT_MASK));
if (ct)
nf_ct_get(skb, &ctinfo);
 +  else
 +  return false;

the above else case looks redundant since it maps to the following check
if (!ct)
return false;
which services a superset of the code flow.


Sure, we can let it fall through... missed that.

After waiting for more review I'll send a V2 patch.

Thanks for the review Darrell

- Greg



}
if (!ct)
return false;
 --
 1.8.3.1
 
 ___

 dev mailing list
 d...@openvswitch.org
 
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=5da6ykiSQJBoJXmpq6iVknmPAc5JDzlVngmp8j_xcXA&s=PsX-njQ_hFqy8P77KNEyX0i7u165p2Wrbg4uAYqfbGs&e=

Re: [ovs-dev] [PATCH] datapath: Fix for force/commit action failures

2017-07-13 Thread Darrell Ball 


On 7/13/17, 9:25 AM, "ovs-dev-boun...@openvswitch.org on behalf of Greg Rose" 
 wrote:

When there is an established connection in direction A->B, it is
possible to receive a packet on port B which then executes
ct(commit,force) without first performing ct() - ie, a lookup.
In this case, we would expect that this packet can delete the existing
entry so that we can commit a connection with direction B->A. However,
currently we only perform a check in skb_nfct_cached() for whether
OVS_CS_F_TRACKED is set and OVS_CS_F_INVALID is not set, ie that a
lookup previously occurred. In the above scenario, a lookup has not
occurred but we should still be able to statelessly look up the
existing entry and potentially delete the entry if it is in the
opposite direction.

This patch extends the check to also hint that if the action has the
force flag set, then we will lookup the existing entry so that the
force check at the end of skb_nfct_cached has the ability to delete
the connection.

CC: d...@openvswitch.org
CC: Pravin Shalar 
Signed-off-by: Joe Stringer 
Signed-off-by: Greg Rose 
---
 net/openvswitch/conntrack.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 08679eb..9041cf8 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -641,17 +641,21 @@ static bool skb_nfct_cached(struct net *net,
ct = nf_ct_get(skb, &ctinfo);
/* If no ct, check if we have evidence that an existing conntrack entry
 * might be found for this skb.  This happens when we lose a skb->_nfct
-* due to an upcall.  If the connection was not confirmed, it is not
-* cached and needs to be run through conntrack again.
+* due to an upcall, or if the direction is being forced.  If the
+* connection was not confirmed, it is not cached and needs to be run
+* through conntrack again.
 */
-   if (!ct && key->ct_state & OVS_CS_F_TRACKED &&
+   if ((!ct && (key->ct_state & OVS_CS_F_TRACKED) &&
!(key->ct_state & OVS_CS_F_INVALID) &&
-   key->ct_zone == info->zone.id) {
+key->ct_zone == info->zone.id) ||
+(!key->ct_state && info->force)) {
ct = ovs_ct_find_existing(net, &info->zone, info->family, skb,
  !!(key->ct_state
 & OVS_CS_F_NAT_MASK));
if (ct)
nf_ct_get(skb, &ctinfo);
+   else
+   return false;

the above else case looks redundant since it maps to the following check
if (!ct)
return false;
which services a superset of the code flow.

}
if (!ct)
return false;
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=5da6ykiSQJBoJXmpq6iVknmPAc5JDzlVngmp8j_xcXA&s=PsX-njQ_hFqy8P77KNEyX0i7u165p2Wrbg4uAYqfbGs&e=