On 7 February 2017 at 17:03, Jarno Rajahalme <ja...@ovn.org> wrote: > > On Feb 7, 2017, at 2:15 PM, Joe Stringer <j...@ovn.org> wrote: > > On 2 February 2017 at 17:10, Jarno Rajahalme <ja...@ovn.org> wrote: > > Stateful network admission policy may allow connections to one > direction and reject connections initiated in the other direction. > After policy change it is possible that for a new connection an > overlapping conntrack entry already exist, where the connection > original direction is opposed to the new connection's initial packet. > > Most importantly, conntrack state relating to the current packet gets > the "reply" designation based on whether the original direction tuple > or the reply direction tuple matched. If this "directionality" is > wrong w.r.t. to the stateful network admission policy it may happen > that packets in neither direction are correctly admitted. > > This patch adds a new "force commit" option to the OVS conntrack > action that checks the original direction of an existing conntrack > entry. If that direction is opposed to the current packet, the > existing conntrack entry is deleted and a new one is subsequently > created in the correct direction. > > Signed-off-by: Jarno Rajahalme <ja...@ovn.org> > --- > include/uapi/linux/openvswitch.h | 10 ++++++++++ > net/openvswitch/conntrack.c | 27 +++++++++++++++++++++++++-- > 2 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/openvswitch.h > b/include/uapi/linux/openvswitch.h > index 90af8b8..d5ba9a9 100644 > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -674,6 +674,10 @@ struct ovs_action_hash { > * @OVS_CT_ATTR_HELPER: variable length string defining conntrack ALG. > * @OVS_CT_ATTR_NAT: Nested OVS_NAT_ATTR_* for performing L3 network address > * translation (NAT) on the packet. > + * @OVS_CT_ATTR_FORCE_COMMIT: Like %OVS_CT_ATTR_COMMIT, but instead of > doing > + * nothing if the connection is already committed will check that the > current > + * packet is in conntrack entry's original direction. If directionality > does > + * not match, will delete the existing conntrack entry and commit a new > one. > */ > enum ovs_ct_attr { > OVS_CT_ATTR_UNSPEC, > @@ -684,6 +688,12 @@ enum ovs_ct_attr { > OVS_CT_ATTR_HELPER, /* netlink helper to assist detection of > related connections. */ > OVS_CT_ATTR_NAT, /* Nested OVS_NAT_ATTR_* */ > + OVS_CT_ATTR_FORCE_COMMIT, /* No argument, commits connection. If > the > + * conntrack entry original direction > tuple > + * does not match the current packet > header > + * values, will delete the current > conntrack > + * entry and create a new one. > + */ > > > We only need one copy of the explanation, keep it above the enum, then > the inline comment can be /* No argument */. > > > OK. > > __OVS_CT_ATTR_MAX > }; > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index 1afe153..1f27f44 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -65,6 +65,7 @@ struct ovs_conntrack_info { > struct nf_conn *ct; > u8 commit : 1; > u8 nat : 3; /* enum ovs_ct_nat */ > + u8 force : 1; > u16 family; > struct md_mark mark; > struct md_labels labels; > @@ -631,10 +632,13 @@ static bool skb_nfct_cached(struct net *net, > */ > 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) { > 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 ctinfo is only used with the new call below, we can unconditionally > fetch this just before it's used... > > if (!ct) > return false; > if (!net_eq(net, read_pnet(&ct->ct_net))) > @@ -648,6 +652,19 @@ static bool skb_nfct_cached(struct net *net, > if (help && rcu_access_pointer(help->helper) != info->helper) > return false; > } > + /* Force conntrack entry direction to the current packet? */ > > > Here. > > > But then we would be executing nf_ct_get() twice in the common case?
Ah, fair enough. It's fine here.