* [PATCH net-next] openvswitch: add ct_clear action
@ 2017-10-06 16:44 Eric Garver
2017-10-10 3:52 ` David Miller
2017-10-10 4:41 ` Pravin Shelar
0 siblings, 2 replies; 9+ messages in thread
From: Eric Garver @ 2017-10-06 16:44 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA; +Cc: dev-yBygre7rU0TnMu66kgdUjQ
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(+)
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 */
--
2.12.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] openvswitch: add ct_clear action
2017-10-06 16:44 [PATCH net-next] openvswitch: add ct_clear action Eric Garver
@ 2017-10-10 3:52 ` David Miller
2017-10-10 4:41 ` Pravin Shelar
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2017-10-10 3:52 UTC (permalink / raw)
To: e; +Cc: netdev, dev, pshelar
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] openvswitch: add ct_clear action
2017-10-06 16:44 [PATCH net-next] openvswitch: add ct_clear action Eric Garver
2017-10-10 3:52 ` David Miller
@ 2017-10-10 4:41 ` Pravin Shelar
2017-10-10 12:33 ` Joe Stringer
[not found] ` <CAOrHB_BUVwWqYQcbWiQ69EhnQpY8epqu55pRBGA6g7xG21zSqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 2 replies; 9+ messages in thread
From: Pravin Shelar @ 2017-10-10 4:41 UTC (permalink / raw)
To: Eric Garver; +Cc: Linux Kernel Network Developers, ovs dev
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)
> {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] openvswitch: add ct_clear action
2017-10-10 4:41 ` Pravin Shelar
@ 2017-10-10 12:33 ` Joe Stringer
2017-10-10 15:09 ` Eric Garver
[not found] ` <CAOrHB_BUVwWqYQcbWiQ69EhnQpY8epqu55pRBGA6g7xG21zSqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 9+ messages in thread
From: Joe Stringer @ 2017-10-10 12:33 UTC (permalink / raw)
To: Pravin Shelar; +Cc: Eric Garver, Linux Kernel Network Developers, ovs dev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] openvswitch: add ct_clear action
[not found] ` <CAOrHB_BUVwWqYQcbWiQ69EhnQpY8epqu55pRBGA6g7xG21zSqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-10-10 12:48 ` Eric Garver
0 siblings, 0 replies; 9+ messages in thread
From: Eric Garver @ 2017-10-10 12:48 UTC (permalink / raw)
To: Pravin Shelar; +Cc: ovs dev, Linux Kernel Network Developers
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)
> > {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] openvswitch: add ct_clear action
2017-10-10 12:33 ` Joe Stringer
@ 2017-10-10 15:09 ` Eric Garver
2017-10-10 17:24 ` Joe Stringer
0 siblings, 1 reply; 9+ messages in thread
From: Eric Garver @ 2017-10-10 15:09 UTC (permalink / raw)
To: Joe Stringer; +Cc: Pravin Shelar, Linux Kernel Network Developers, ovs dev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] openvswitch: add ct_clear action
2017-10-10 15:09 ` Eric Garver
@ 2017-10-10 17:24 ` Joe Stringer
2017-10-10 19:13 ` [ovs-dev] " Eric Garver
0 siblings, 1 reply; 9+ messages in thread
From: Joe Stringer @ 2017-10-10 17:24 UTC (permalink / raw)
To: Eric Garver, Joe Stringer, Pravin Shelar,
Linux Kernel Network Developers, ovs dev
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-LZ6Gd1LRuIk@public.gmane.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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action
2017-10-10 17:24 ` Joe Stringer
@ 2017-10-10 19:13 ` Eric Garver
2017-10-10 20:24 ` Joe Stringer
0 siblings, 1 reply; 9+ messages in thread
From: Eric Garver @ 2017-10-10 19:13 UTC (permalink / raw)
To: Joe Stringer; +Cc: Pravin Shelar, Linux Kernel Network Developers, ovs dev
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action
2017-10-10 19:13 ` [ovs-dev] " Eric Garver
@ 2017-10-10 20:24 ` Joe Stringer
0 siblings, 0 replies; 9+ messages in thread
From: Joe Stringer @ 2017-10-10 20:24 UTC (permalink / raw)
To: Eric Garver, Joe Stringer, Pravin Shelar,
Linux Kernel Network Developers, ovs dev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-10 20:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-06 16:44 [PATCH net-next] openvswitch: add ct_clear action Eric Garver
2017-10-10 3:52 ` David Miller
2017-10-10 4:41 ` Pravin Shelar
2017-10-10 12:33 ` Joe Stringer
2017-10-10 15:09 ` Eric Garver
2017-10-10 17:24 ` Joe Stringer
2017-10-10 19:13 ` [ovs-dev] " Eric Garver
2017-10-10 20:24 ` Joe Stringer
[not found] ` <CAOrHB_BUVwWqYQcbWiQ69EhnQpY8epqu55pRBGA6g7xG21zSqQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-10-10 12:48 ` Eric Garver
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.