| Message ID | 1500392574-4462-2-git-send-email-gvrose8192@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Joe Stringer |
| Headers | show |
On 18 July 2017 at 08:42, Greg Rose <gvrose8192@gmail.com> wrote: > Upstream commit: > commit 8b97ac5bda17cfaa257bcab6180af0f43a2e87e0 > Author: Greg Rose <gvrose8192@gmail.com> > Date: Fri Jul 14 12:42:49 2017 -0700 > > openvswitch: Fix for force/commit action failures > > 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 <pshelar@nicira.com> > CC: dev@openvswitch.org > Signed-off-by: Joe Stringer <joe@ovn.org> > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Co-authored-by: Joe Stringer <joe@ovn.org> > Signed-off-by: Joe Stringer <joe@ovn.org> > Signed-off-by: Greg Rose <gvrose8192@gmail.com> Thanks for the backport, if you don't mind I'd like to hold off until we can assemble the full series to sync with upstream, so we get the commits in the same order. I believe you're working on that at the moment, so I'll keep an eye out for when that series is available. Cheers, Joe
On 07/18/2017 04:29 PM, Joe Stringer wrote: > On 18 July 2017 at 08:42, Greg Rose <gvrose8192@gmail.com> wrote: > > Upstream commit: > > commit 8b97ac5bda17cfaa257bcab6180af0f43a2e87e0 > > Author: Greg Rose <gvrose8192@gmail.com> > > Date: Fri Jul 14 12:42:49 2017 -0700 > > > > openvswitch: Fix for force/commit action failures > > > > 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 <pshelar@nicira.com> > > CC: dev@openvswitch.org > > Signed-off-by: Joe Stringer <joe@ovn.org> > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > > Co-authored-by: Joe Stringer <joe@ovn.org> > > Signed-off-by: Joe Stringer <joe@ovn.org> > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > > Thanks for the backport, if you don't mind I'd like to hold off until > we can assemble the full series to sync with upstream, so we get the > commits in the same order. I believe you're working on that at the > moment, so I'll keep an eye out for when that series is available. > > Cheers, > Joe > Sure, sounds good. Thanks! - Greg
On 19 July 2017 at 06:47, Greg Rose <gvrose8192@gmail.com> wrote: > On 07/18/2017 04:29 PM, Joe Stringer wrote: >> >> On 18 July 2017 at 08:42, Greg Rose <gvrose8192@gmail.com> wrote: >> > Upstream commit: >> > commit 8b97ac5bda17cfaa257bcab6180af0f43a2e87e0 >> > Author: Greg Rose <gvrose8192@gmail.com> >> > Date: Fri Jul 14 12:42:49 2017 -0700 >> > >> > openvswitch: Fix for force/commit action failures >> > >> > 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 <pshelar@nicira.com> >> > CC: dev@openvswitch.org >> > Signed-off-by: Joe Stringer <joe@ovn.org> >> > Signed-off-by: Greg Rose <gvrose8192@gmail.com> >> > Signed-off-by: David S. Miller <davem@davemloft.net> >> > >> > Co-authored-by: Joe Stringer <joe@ovn.org> >> > Signed-off-by: Joe Stringer <joe@ovn.org> >> > Signed-off-by: Greg Rose <gvrose8192@gmail.com> >> >> Thanks for the backport, if you don't mind I'd like to hold off until >> we can assemble the full series to sync with upstream, so we get the >> commits in the same order. I believe you're working on that at the >> moment, so I'll keep an eye out for when that series is available. >> >> Cheers, >> Joe >> > Sure, sounds good. > > Thanks! Thanks, I applied this patch to master.
diff --git a/datapath/conntrack.c b/datapath/conntrack.c index bf28fc0..30d2add 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -663,6 +663,34 @@ ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone, return ct; } +static +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) +{ + struct nf_conn *ct = NULL; + + /* 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, 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. + */ + *ct_executed = (key->ct_state & OVS_CS_F_TRACKED) && + !(key->ct_state & OVS_CS_F_INVALID) && + (key->ct_zone == info->zone.id); + + if (*ct_executed || (!key->ct_state && info->force)) { + ct = ovs_ct_find_existing(net, &info->zone, info->family, skb, + !!(key->ct_state & + OVS_CS_F_NAT_MASK)); + } + + return ct; +} + /* Determine whether skb->_nfct is equal to the result of conntrack lookup. */ static bool skb_nfct_cached(struct net *net, const struct sw_flow_key *key, @@ -671,24 +699,17 @@ static bool skb_nfct_cached(struct net *net, { enum ip_conntrack_info ctinfo; struct nf_conn *ct; + bool ct_executed = true; 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. - */ - if (!ct && key->ct_state & OVS_CS_F_TRACKED && - !(key->ct_state & OVS_CS_F_INVALID) && - key->ct_zone == info->zone.id) { - 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); - } if (!ct) + ct = ovs_ct_executed(net, key, info, skb, &ct_executed); + + if (ct) + nf_ct_get(skb, &ctinfo); + else return false; + if (!net_eq(net, read_pnet(&ct->ct_net))) return false; if (!nf_ct_zone_equal_any(info->ct, nf_ct_zone(ct))) @@ -713,7 +734,7 @@ static bool skb_nfct_cached(struct net *net, return false; } - return true; + return ct_executed; } #ifdef CONFIG_NF_NAT_NEEDED
Upstream commit: commit 8b97ac5bda17cfaa257bcab6180af0f43a2e87e0 Author: Greg Rose <gvrose8192@gmail.com> Date: Fri Jul 14 12:42:49 2017 -0700 openvswitch: Fix for force/commit action failures 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 <pshelar@nicira.com> CC: dev@openvswitch.org Signed-off-by: Joe Stringer <joe@ovn.org> Signed-off-by: Greg Rose <gvrose8192@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> Co-authored-by: Joe Stringer <joe@ovn.org> Signed-off-by: Joe Stringer <joe@ovn.org> Signed-off-by: Greg Rose <gvrose8192@gmail.com> --- V2 - Reformat commit message to conform to policy for backports of upstream patches --- datapath/conntrack.c | 51 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 15 deletions(-)