| Message ID | 20180122191005.14376-3-e@erig.me |
|---|---|
| State | Accepted |
| Headers | show |
| Series | datapath: backport ct_clear action | expand |
On Mon, Jan 22, 2018 at 11:10 AM, Eric Garver <e@erig.me> wrote: > Upstream commit: > commit b8226962b1c49c784aeddb9d2fafbf53dfdc2190 > Author: Eric Garver <e@erig.me> > Date: Tue Oct 10 16:54:44 2017 -0400 > > openvswitch: add ct_clear action > > 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 <e@erig.me> > Acked-by: Pravin B Shelar <pshelar@ovn.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Notes: > - hunk from include/uapi/linux/openvswitch.h is missing because it > was added with userspace support in 1fe178d251c8 ("dpif: Add support > for OVS_ACTION_ATTR_CT_CLEAR") > - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set() > calls do. Since we're setting ct to NULL this is okay. > > Signed-off-by: Eric Garver <e@erig.me> LGTM. Acked-by: Pravin B Shelar <pshelar@ovn.org> Thanks.
On Mon, Jan 22, 2018 at 11:10 AM, Eric Garver <e@erig.me> wrote: > Upstream commit: > commit b8226962b1c49c784aeddb9d2fafbf53dfdc2190 > Author: Eric Garver <e@erig.me> > Date: Tue Oct 10 16:54:44 2017 -0400 > > openvswitch: add ct_clear action > > 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 <e@erig.me> > Acked-by: Pravin B Shelar <pshelar@ovn.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Notes: > - hunk from include/uapi/linux/openvswitch.h is missing because it > was added with userspace support in 1fe178d251c8 ("dpif: Add support > for OVS_ACTION_ATTR_CT_CLEAR") > - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set() > calls do. Since we're setting ct to NULL this is okay. > > Signed-off-by: Eric Garver <e@erig.me> > --- > datapath/actions.c | 4 ++++ > datapath/conntrack.c | 15 +++++++++++++++ > datapath/conntrack.h | 7 +++++++ > datapath/flow_netlink.c | 5 +++++ > 4 files changed, 31 insertions(+) > > diff --git a/datapath/actions.c b/datapath/actions.c > index ad18c2cc768a..1840fe556baf 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c > @@ -1214,6 +1214,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; > diff --git a/datapath/conntrack.c b/datapath/conntrack.c > index d517a87b0474..3f7943370fb3 100644 > --- a/datapath/conntrack.c > +++ b/datapath/conntrack.c > @@ -1170,6 +1170,21 @@ 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)); > +#ifdef HAVE_IP_CT_UNTRACKED I just noticed, there is no code to define symbol HAVE_IP_CT_UNTRACKED. Can you add it to acinclude.m4 ?
On Mon, Jan 22, 2018 at 02:38:07PM -0800, Pravin Shelar wrote: > On Mon, Jan 22, 2018 at 11:10 AM, Eric Garver <e@erig.me> wrote: > > Upstream commit: > > commit b8226962b1c49c784aeddb9d2fafbf53dfdc2190 > > Author: Eric Garver <e@erig.me> > > Date: Tue Oct 10 16:54:44 2017 -0400 > > > > openvswitch: add ct_clear action > > > > 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 <e@erig.me> > > Acked-by: Pravin B Shelar <pshelar@ovn.org> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > > Notes: > > - hunk from include/uapi/linux/openvswitch.h is missing because it > > was added with userspace support in 1fe178d251c8 ("dpif: Add support > > for OVS_ACTION_ATTR_CT_CLEAR") > > - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set() > > calls do. Since we're setting ct to NULL this is okay. > > > > Signed-off-by: Eric Garver <e@erig.me> > > --- > > datapath/actions.c | 4 ++++ > > datapath/conntrack.c | 15 +++++++++++++++ > > datapath/conntrack.h | 7 +++++++ > > datapath/flow_netlink.c | 5 +++++ > > 4 files changed, 31 insertions(+) > > > > diff --git a/datapath/actions.c b/datapath/actions.c > > index ad18c2cc768a..1840fe556baf 100644 > > --- a/datapath/actions.c > > +++ b/datapath/actions.c > > @@ -1214,6 +1214,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; > > diff --git a/datapath/conntrack.c b/datapath/conntrack.c > > index d517a87b0474..3f7943370fb3 100644 > > --- a/datapath/conntrack.c > > +++ b/datapath/conntrack.c > > @@ -1170,6 +1170,21 @@ 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)); > > +#ifdef HAVE_IP_CT_UNTRACKED > > I just noticed, there is no code to define symbol > HAVE_IP_CT_UNTRACKED. Can you add it to acinclude.m4 ? That's patch 01. Macro OVS_GREP_IFELSE() creates the define by default if $3 is empty. acinclude.m4: [..snip..] dnl OVS_GREP_IFELSE(FILE, REGEX, [IF-MATCH], [IF-NO-MATCH]) dnl dnl Greps FILE for REGEX. If it matches, runs IF-MATCH, otherwise IF-NO-MATCH. dnl If IF-MATCH is empty then it defines to OVS_DEFINE(HAVE_<REGEX>), with dnl <REGEX> translated to uppercase. AC_DEFUN([OVS_GREP_IFELSE], [ [..snip..]
On Mon, Jan 22, 2018 at 02:34:53PM -0800, Pravin Shelar wrote: > On Mon, Jan 22, 2018 at 11:10 AM, Eric Garver <e@erig.me> wrote: > > Upstream commit: > > commit b8226962b1c49c784aeddb9d2fafbf53dfdc2190 > > Author: Eric Garver <e@erig.me> > > Date: Tue Oct 10 16:54:44 2017 -0400 > > > > openvswitch: add ct_clear action > > > > 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 <e@erig.me> > > Acked-by: Pravin B Shelar <pshelar@ovn.org> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > > Notes: > > - hunk from include/uapi/linux/openvswitch.h is missing because it > > was added with userspace support in 1fe178d251c8 ("dpif: Add support > > for OVS_ACTION_ATTR_CT_CLEAR") > > - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set() > > calls do. Since we're setting ct to NULL this is okay. > > > > Signed-off-by: Eric Garver <e@erig.me> > > LGTM. > > Acked-by: Pravin B Shelar <pshelar@ovn.org> Thanks for reviewing this patch series. Do you intend to apply it directly to the OVS repo, or would you prefer for someone else to do it? Thanks, Ben.
On Mon, Jan 22, 2018 at 3:56 PM, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Jan 22, 2018 at 02:34:53PM -0800, Pravin Shelar wrote: >> On Mon, Jan 22, 2018 at 11:10 AM, Eric Garver <e@erig.me> wrote: >> > Upstream commit: >> > commit b8226962b1c49c784aeddb9d2fafbf53dfdc2190 >> > Author: Eric Garver <e@erig.me> >> > Date: Tue Oct 10 16:54:44 2017 -0400 >> > >> > openvswitch: add ct_clear action >> > >> > 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 <e@erig.me> >> > Acked-by: Pravin B Shelar <pshelar@ovn.org> >> > Signed-off-by: David S. Miller <davem@davemloft.net> >> > >> > Notes: >> > - hunk from include/uapi/linux/openvswitch.h is missing because it >> > was added with userspace support in 1fe178d251c8 ("dpif: Add support >> > for OVS_ACTION_ATTR_CT_CLEAR") >> > - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set() >> > calls do. Since we're setting ct to NULL this is okay. >> > >> > Signed-off-by: Eric Garver <e@erig.me> >> >> LGTM. >> >> Acked-by: Pravin B Shelar <pshelar@ovn.org> > > Thanks for reviewing this patch series. Do you intend to apply it > directly to the OVS repo, or would you prefer for someone else to do it? > I thought it was part of userspace patch series. But now I see the first patch. I will commit it soon.
On Mon, Jan 22, 2018 at 11:10 AM, Eric Garver <e@erig.me> wrote: > Upstream commit: > commit b8226962b1c49c784aeddb9d2fafbf53dfdc2190 > Author: Eric Garver <e@erig.me> > Date: Tue Oct 10 16:54:44 2017 -0400 > > openvswitch: add ct_clear action > > 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 <e@erig.me> > Acked-by: Pravin B Shelar <pshelar@ovn.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Notes: > - hunk from include/uapi/linux/openvswitch.h is missing because it > was added with userspace support in 1fe178d251c8 ("dpif: Add support > for OVS_ACTION_ATTR_CT_CLEAR") > - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set() > calls do. Since we're setting ct to NULL this is okay. > > Signed-off-by: Eric Garver <e@erig.me> I pushed series to master and branch 2.9. Thanks.
diff --git a/datapath/actions.c b/datapath/actions.c index ad18c2cc768a..1840fe556baf 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -1214,6 +1214,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; diff --git a/datapath/conntrack.c b/datapath/conntrack.c index d517a87b0474..3f7943370fb3 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -1170,6 +1170,21 @@ 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)); +#ifdef HAVE_IP_CT_UNTRACKED + nf_ct_set(skb, NULL, IP_CT_UNTRACKED); +#else + nf_ct_set(skb, NULL, 0); +#endif + ovs_ct_fill_key(skb, key); + } + + return 0; +} + static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name, const struct sw_flow_key *key, bool log) { diff --git a/datapath/conntrack.h b/datapath/conntrack.h index 69ceb80e5a73..0c3964f5bcaf 100644 --- a/datapath/conntrack.h +++ b/datapath/conntrack.h @@ -31,6 +31,7 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *, struct sk_buff *); int ovs_ct_execute(struct net *, struct sk_buff *, struct sw_flow_key *, const struct ovs_conntrack_info *); +int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key); void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key); int ovs_ct_put_key(const struct sw_flow_key *swkey, @@ -74,6 +75,12 @@ static inline int ovs_ct_execute(struct net *net, struct sk_buff *skb, return -ENOTSUPP; } +static inline int ovs_ct_clear(struct sk_buff *skb, + struct sw_flow_key *key) +{ + return -ENOTSUPP; +} + static inline void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key) { diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c index b3b209269dcc..303337c4e99f 100644 --- a/datapath/flow_netlink.c +++ b/datapath/flow_netlink.c @@ -77,6 +77,7 @@ static bool actions_may_change_flow(const struct nlattr *actions) break; case OVS_ACTION_ATTR_CT: + case OVS_ACTION_ATTR_CT_CLEAR: case OVS_ACTION_ATTR_HASH: case OVS_ACTION_ATTR_POP_ETH: case OVS_ACTION_ATTR_POP_MPLS: @@ -2487,6 +2488,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, [OVS_ACTION_ATTR_SAMPLE] = (u32)-1, [OVS_ACTION_ATTR_HASH] = sizeof(struct ovs_action_hash), [OVS_ACTION_ATTR_CT] = (u32)-1, + [OVS_ACTION_ATTR_CT_CLEAR] = 0, [OVS_ACTION_ATTR_TRUNC] = sizeof(struct ovs_action_trunc), [OVS_ACTION_ATTR_PUSH_ETH] = sizeof(struct ovs_action_push_eth), [OVS_ACTION_ATTR_POP_ETH] = 0, @@ -2628,6 +2630,9 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, skip_copy = true; break; + case OVS_ACTION_ATTR_CT_CLEAR: + break; + case OVS_ACTION_ATTR_PUSH_ETH: /* Disallow pushing an Ethernet header if one * is already present */
Upstream commit: commit b8226962b1c49c784aeddb9d2fafbf53dfdc2190 Author: Eric Garver <e@erig.me> Date: Tue Oct 10 16:54:44 2017 -0400 openvswitch: add ct_clear action 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 <e@erig.me> Acked-by: Pravin B Shelar <pshelar@ovn.org> Signed-off-by: David S. Miller <davem@davemloft.net> Notes: - hunk from include/uapi/linux/openvswitch.h is missing because it was added with userspace support in 1fe178d251c8 ("dpif: Add support for OVS_ACTION_ATTR_CT_CLEAR") - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set() calls do. Since we're setting ct to NULL this is okay. Signed-off-by: Eric Garver <e@erig.me> --- datapath/actions.c | 4 ++++ datapath/conntrack.c | 15 +++++++++++++++ datapath/conntrack.h | 7 +++++++ datapath/flow_netlink.c | 5 +++++ 4 files changed, 31 insertions(+)