| Message ID | 20171006164426.9752-1-e@erig.me |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] openvswitch: add ct_clear action | expand |
From: Eric Garver <e@erig.me> Date: Fri, 6 Oct 2017 12:44:26 -0400 > 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> Pravin et al., this needs your review. Thanks.
On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> 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 <e@erig.me> Patch mostly looks good. I have following comments. > --- > 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. > > 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) > {
On 9 October 2017 at 21:41, Pravin Shelar <pshelar@ovn.org> wrote: > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> 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 <e@erig.me> > Patch mostly looks good. I have following comments. > >> --- >> 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. > >> >> 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. Really this is trying to just zero the CT key fields, but reuses existing functions, right? This means that subsequent upcalls, for instance, won't have the outdated view of the CT state from the previous lookup (that was prior to the ct_clear). I'd expect these key fields to be cleared.
On Mon, Oct 09, 2017 at 09:41:53PM -0700, Pravin Shelar wrote: > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> 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 <e@erig.me> > Patch mostly looks good. I have following comments. Thanks for the review Pravin! > > --- > > 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. > Right. Not sure how that got there. :/ > > > > 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? > I think that will be fine. I'll run my tests again to verify. > > + } > > + > > + ovs_ct_fill_key(skb, key); > > + > I do not see need to refill the key if there is no skb-nf-ct. > Good point. > > + return 0; > > +} > > + > > static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name, > > const struct sw_flow_key *key, bool log) > > {
On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote: > On 9 October 2017 at 21:41, Pravin Shelar <pshelar@ovn.org> wrote: > > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> 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 <e@erig.me> > > Patch mostly looks good. I have following comments. > > > >> --- > >> 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. > > > >> > >> 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. > > Really this is trying to just zero the CT key fields, but reuses > existing functions, right? This means that subsequent upcalls, for Right. > instance, won't have the outdated view of the CT state from the > previous lookup (that was prior to the ct_clear). I'd expect these key > fields to be cleared. I assumed Pravin was saying that we don't need to clear them if there is no conntrack state. They should already be zero.
On 10 October 2017 at 08:09, Eric Garver <e@erig.me> wrote: > On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote: >> On 9 October 2017 at 21:41, Pravin Shelar <pshelar@ovn.org> wrote: >> > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> 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 <e@erig.me> >> > Patch mostly looks good. I have following comments. >> > >> >> --- >> >> 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. >> > >> >> >> >> 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. >> >> Really this is trying to just zero the CT key fields, but reuses >> existing functions, right? This means that subsequent upcalls, for > > Right. > >> instance, won't have the outdated view of the CT state from the >> previous lookup (that was prior to the ct_clear). I'd expect these key >> fields to be cleared. > > I assumed Pravin was saying that we don't need to clear them if there is > no conntrack state. They should already be zero. The conntrack calls aren't going to clear it, so I don't see what else would clear it? If you execute ct(),ct_clear(), then the first ct will set the values.. what will zero them?
On Tue, Oct 10, 2017 at 10:24:20AM -0700, Joe Stringer wrote: > On 10 October 2017 at 08:09, Eric Garver <e@erig.me> wrote: > > On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote: > >> On 9 October 2017 at 21:41, Pravin Shelar <pshelar@ovn.org> wrote: > >> > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> 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 <e@erig.me> > >> > Patch mostly looks good. I have following comments. > >> > > >> >> --- > >> >> 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. > >> > > >> >> > >> >> 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. > >> > >> Really this is trying to just zero the CT key fields, but reuses > >> existing functions, right? This means that subsequent upcalls, for > > > > Right. > > > >> instance, won't have the outdated view of the CT state from the > >> previous lookup (that was prior to the ct_clear). I'd expect these key > >> fields to be cleared. > > > > I assumed Pravin was saying that we don't need to clear them if there is > > no conntrack state. They should already be zero. > > The conntrack calls aren't going to clear it, so I don't see what else > would clear it? > > If you execute ct(),ct_clear(), then the first ct will set the > values.. what will zero them? I meant move ovs_ct_fill_key() to inside the if statement. i.e. if (skb_nfct(skb)) { nf_conntrack_put(skb_nfct(skb)); nf_ct_set(skb, NULL, IP_CT_UNTRACKED); ovs_ct_fill_key(skb, key); } Should be nothing to fill/zero if we have not yet done conntrack. Is there a case where we may lose skb->_nfct, but the key still has conntrack data?
On 10 October 2017 at 12:13, Eric Garver <e@erig.me> wrote: > On Tue, Oct 10, 2017 at 10:24:20AM -0700, Joe Stringer wrote: >> On 10 October 2017 at 08:09, Eric Garver <e@erig.me> wrote: >> > On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote: >> >> On 9 October 2017 at 21:41, Pravin Shelar <pshelar@ovn.org> wrote: >> >> > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> 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 <e@erig.me> >> >> > Patch mostly looks good. I have following comments. >> >> > >> >> >> --- >> >> >> 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. >> >> > >> >> >> >> >> >> 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. >> >> >> >> Really this is trying to just zero the CT key fields, but reuses >> >> existing functions, right? This means that subsequent upcalls, for >> > >> > Right. >> > >> >> instance, won't have the outdated view of the CT state from the >> >> previous lookup (that was prior to the ct_clear). I'd expect these key >> >> fields to be cleared. >> > >> > I assumed Pravin was saying that we don't need to clear them if there is >> > no conntrack state. They should already be zero. >> >> The conntrack calls aren't going to clear it, so I don't see what else >> would clear it? >> >> If you execute ct(),ct_clear(), then the first ct will set the >> values.. what will zero them? > > I meant move ovs_ct_fill_key() to inside the if statement. > i.e. > > if (skb_nfct(skb)) { > nf_conntrack_put(skb_nfct(skb)); > nf_ct_set(skb, NULL, IP_CT_UNTRACKED); > ovs_ct_fill_key(skb, key); > } > > Should be nothing to fill/zero if we have not yet done conntrack. > Is there a case where we may lose skb->_nfct, but the key still has > conntrack data? Ah, misreading on my part. Right, if there is no nfct then it should already have the right value. I don't think there's a way to lose it and leave the key with conntrack data.
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; + } 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); + } + + 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/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h index bc7efd1867ab..399dfdd2c4f9 100644 --- a/net/openvswitch/conntrack.h +++ b/net/openvswitch/conntrack.h @@ -30,6 +30,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, @@ -73,6 +74,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/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index e8eb427ce6d1..198bb828d592 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -75,6 +75,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: @@ -2479,6 +2480,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, @@ -2620,6 +2622,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 */
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> --- 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(+)