From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Rose Subject: Re: [PATCH net] openvswitch: Fix for force/commit action failures Date: Fri, 14 Jul 2017 11:44:07 -0700 Message-ID: References: <1500048639-24169-1-git-send-email-gvrose8192@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Cc: ovs dev , netdev To: Joe Stringer Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces-yBygre7rU0TnMu66kgdUjQ@public.gmane.org Errors-To: ovs-dev-bounces-yBygre7rU0TnMu66kgdUjQ@public.gmane.org List-Id: netdev.vger.kernel.org On 07/14/2017 11:42 AM, Joe Stringer wrote: > On 14 July 2017 at 09:10, 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. > > > > Fixes: dd41d330b03 ("openvswitch: Add force commit.") > > CC: Pravin Shelar > > CC: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org > > Signed-off-by: Joe Stringer > > Signed-off-by: Greg Rose > > --- > > net/openvswitch/conntrack.c | 50 +++++++++++++++++++++++++++++++-------------- > > 1 file changed, 35 insertions(+), 15 deletions(-) > > > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > > index 08679eb..1260f2b 100644 > > --- a/net/openvswitch/conntrack.c > > +++ b/net/openvswitch/conntrack.c > > @@ -629,6 +629,33 @@ static int handle_fragments(struct net *net, struct sw_flow_key *key, > > return ct; > > } > > > > +struct nf_conn *ovs_ct_executed(struct net *net, > > + const struct sw_flow_key *key, > > + const struct ovs_conntrack_info *info, > > + struct sk_buff *skb, > > + bool *ct_executed) > > Actually, some compilers will report this new warning: > net/openvswitch/conntrack.c:632:16: warning: symbol 'ovs_ct_executed' > was not declared. Should it be static? > > Could you make this function static and repost? > > Thanks. > Yes, I just saw that too. Need to update my compiler for my primary build machine. I'll send a V2... Thanks! - Greg