On 12 July 2017 at 11:05, Greg Rose <[email protected]> wrote: > When the direction is being forced key->ct_state may not have been > set. Check for this condition and take action to set the state > correctly so that the force direction occurs. > > Co-authored-by: Joe Stringer <[email protected]> > Signed-off-by: Joe Stringer <[email protected]> > Signed-off-by: Greg Rose <[email protected]> > ---
Hi Greg, a few bits of high level feedback before I get into the patch itself: Usually, patches to datapath/* just use the "datapath: " prefix in the subject. Typically, the "conntrack:" prefix in the OVS tree indicates changes to lib/conntrack.[ch], which is used only for the userspace datapath. Patches to datapath/*.[ch], ie the Linux kernel module code, need to be accepted into upstream Linux trees before applying to the OVS tree. For bugfixes, this means the 'net' tree; for features, 'net-next'. That said, if you're looking for feedback from the OVS community before posting upstream then it's still quite reasonable to post the patch here on ovs-dev first. For what it's worth, there's some documentation about this policy here: http://docs.openvswitch.org/en/latest/internals/contributing/backporting-patches/ I happen to have some of the context here, so it's a little easier for me to understand what's going on but the patch description is a little hard to follow. Given that the upstream Linux openvswitch maintainer will be the primary person to provide feedback on the patch, the clearer we can explain the better :-) I've given an attempt at providing more context for the patch description below, but feel free to take or leave any/all of it if it helps to describe the solution: "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. " I ran this on my tester box across a variety of kernels and platforms, and everything was green. I think that you've been using a patch to tests/system-traffic.at to validate this behaviour, would you be able to send that to the ovs-dev list as a separate patch as well? When we get this into the OVS tree, it would be nice to have the test to validate that behaviour, to ensure that the userspace datapath has the same behaviour, and to help to stop regressions in future. > datapath/conntrack.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/datapath/conntrack.c b/datapath/conntrack.c > index bf28fc0..2da0321 100644 > --- a/datapath/conntrack.c > +++ b/datapath/conntrack.c > @@ -665,7 +665,7 @@ ovs_ct_find_existing(struct net *net, const struct > nf_conntrack_zone *zone, > > /* 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, > + struct sw_flow_key *key, > const struct ovs_conntrack_info *info, > struct sk_buff *skb) > { > @@ -675,12 +675,22 @@ 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)) { > + if (!key->ct_state && info->force && !info->ct) { > + int result = nf_conntrack_in(net, info->family, > + NF_INET_PRE_ROUTING, > skb); > + if (result != NF_ACCEPT) > + return false; I suspect that in this scenario, the extra nf_conntrack_in() here is not strictly necessary; skb_nfct_cached() is actually trying to avoid doing nf_conntrack_in() in __ovs_ct_lookup(), so to put an nf_conntrack_in() call here is a bit difficult to fully reason about. This function is always hit for every packet that executes ct() action, but it's primarily here to deal with losing state upon upcall. Can you try removing it and just keeping the if condition changes? Cheers, Joe _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
