Re: [PATCH net-next] openvswitch: add ct_clear action
From: Pravin Shelar <hidden>
Date: 2017-10-10 04:41:56
On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver [off-list ref] wrote:
This adds a ct_clear action for clearing conntrack state. ct_clear is currently implemented in OVS userspace, but is not backed by an action in the kernel datapath. This is useful for flows that may modify a packet tuple after a ct lookup has already occurred. Signed-off-by: Eric Garver <redacted>
Patch mostly looks good. I have following comments.
quoted hunk ↗ jump to hunk
--- include/uapi/linux/openvswitch.h | 2 ++ net/openvswitch/actions.c | 5 +++++ net/openvswitch/conntrack.c | 12 ++++++++++++ net/openvswitch/conntrack.h | 7 +++++++ net/openvswitch/flow_netlink.c | 5 +++++ 5 files changed, 31 insertions(+)diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index 156ee4cab82e..1b6e510e2cc6 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h@@ -806,6 +806,7 @@ struct ovs_action_push_eth { * packet. * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the * packet. + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet. * * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all * fields within a header are modifiable, e.g. the IPv4 protocol and fragment@@ -835,6 +836,7 @@ enum ovs_action_attr { OVS_ACTION_ATTR_TRUNC, /* u32 struct ovs_action_trunc. */ OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */ OVS_ACTION_ATTR_POP_ETH, /* No argument. */ + OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */ __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted * from userspace. */diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index a54a556fcdb5..db9c7f2e662b 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c@@ -1203,6 +1203,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, return err == -EINPROGRESS ? 0 : err; break; + case OVS_ACTION_ATTR_CT_CLEAR: + err = ovs_ct_clear(skb, key); + break; + case OVS_ACTION_ATTR_PUSH_ETH: err = push_eth(skb, key, nla_data(a)); break;@@ -1210,6 +1214,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, case OVS_ACTION_ATTR_POP_ETH: err = pop_eth(skb, key); break; + }
Unrelated change.
quoted hunk ↗ jump to hunk
if (unlikely(err)) {diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index d558e882ca0c..f9b73c726ad7 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c@@ -1129,6 +1129,18 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb, return err; } +int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key) +{ + if (skb_nfct(skb)) { + nf_conntrack_put(skb_nfct(skb)); + nf_ct_set(skb, NULL, 0);
Can the new conntract state be appropriate? may be IP_CT_UNTRACKED?
+ } + + ovs_ct_fill_key(skb, key); +
I do not see need to refill the key if there is no skb-nf-ct.
+ return 0;
+}
+
static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
const struct sw_flow_key *key, bool log)
{