[RFC] [net]openvswitch: Clear the ct flow key for the recirculated packet

[RFC] [net]openvswitch: Clear the ct flow key for the recirculated packet

[Numan Siddique]

It is possible that the ct flow key information would have
gone stale for the packets received from the userspace due to
clone or ct_clear actions.

In the case of OVN, it adds ping responder flows, which modifies
the original icmp4 request packet to a reply packet. It uses the
OVS actions - clone and ct_clear. When the reply packet hits the
"ovs_ct_execute" function, and since the ct flow key info is not
cleared, the connection tracker doesn't set the state to
ESTABLISHED state.

Note: This patch is marked as RFC, as I am not sure if this is the correct
place to address this issue or it should be addressed in ovs-vswitchd
to set the OVS_KEY_ATTR_CT_STATE and other related attributes
properly for ct_clear action.

Signed-off-by: Numan Siddique <nusiddiq-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 net/openvswitch/flow.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 9d4bb8e..72b73db 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -836,6 +836,11 @@ int ovs_flow_key_extract_userspace(struct net *net, const struct nlattr *attr,
 	if (err)
 		return err;
 
+	/* Clear the ct flow key after key_extract to avoid using
+	 * stale ct key information.
+	*/
+	ovs_ct_fill_key(skb, key);
+
 	/* Check that we have conntrack original direction tuple metadata only
 	 * for packets for which it makes sense.  Otherwise the key may be
 	 * corrupted due to overlapping key fields.
-- 
2.9.3

Re: [RFC] [net]openvswitch: Clear the ct flow key for the recirculated packet

[Lance Richardson]

> From: "Numan Siddique" <nusiddiq-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> To: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "ovs dev" <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>
> Cc: "Joe Stringer" <joe-LZ6Gd1LRuIk@public.gmane.org>, "Andy Zhou" <azhou-LZ6Gd1LRuIk@public.gmane.org>, jarno-LZ6Gd1LRuIk@public.gmane.org
> Sent: Thursday, March 16, 2017 8:25:06 AM
> Subject: [RFC] [net]openvswitch: Clear the ct flow key for the recirculated packet
> 
> It is possible that the ct flow key information would have
> gone stale for the packets received from the userspace due to
> clone or ct_clear actions.
> 
> In the case of OVN, it adds ping responder flows, which modifies
> the original icmp4 request packet to a reply packet. It uses the
> OVS actions - clone and ct_clear. When the reply packet hits the
> "ovs_ct_execute" function, and since the ct flow key info is not
> cleared, the connection tracker doesn't set the state to
> ESTABLISHED state.
> 
> Note: This patch is marked as RFC, as I am not sure if this is the correct
> place to address this issue or it should be addressed in ovs-vswitchd
> to set the OVS_KEY_ATTR_CT_STATE and other related attributes
> properly for ct_clear action.
> 
> Signed-off-by: Numan Siddique <nusiddiq-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---

Hi Numan,

With this patch applied I'm consistently seeing failures for two of the
kernel datapath unit tests (via "make check-kernel"):

 16: conntrack - force commit                        FAILED (system-traffic.at:692)
 54: conntrack - SNAT with ct_mark change on reply   FAILED (system-traffic.at:2446)

Re: [RFC] [net]openvswitch: Clear the ct flow key for the recirculated packet

[Joe Stringer]

On 16 March 2017 at 05:25, Numan Siddique <nusiddiq-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> It is possible that the ct flow key information would have
> gone stale for the packets received from the userspace due to
> clone or ct_clear actions.
>
> In the case of OVN, it adds ping responder flows, which modifies
> the original icmp4 request packet to a reply packet. It uses the
> OVS actions - clone and ct_clear. When the reply packet hits the
> "ovs_ct_execute" function, and since the ct flow key info is not
> cleared, the connection tracker doesn't set the state to
> ESTABLISHED state.
>
> Note: This patch is marked as RFC, as I am not sure if this is the correct
> place to address this issue or it should be addressed in ovs-vswitchd
> to set the OVS_KEY_ATTR_CT_STATE and other related attributes
> properly for ct_clear action.

Hmm. I see a couple of options.

At the moment the ct_clear at the higher layer is not backed by any
action in the datapath layer. Basically we just clear the fields in
the flow metadata then from the OpenFlow point of view there is no
such state, so we can match on it as though it hasn't been through the
connection tracker, and for instance, try to send it through the
connection tracker again. If we were to introduce an openvswitch
kernel API to actually clear the 'skb->nfct' and perhaps the related
ct flow key fields in the kernel, then subsequent ct lookups would not
assume that the skb is already cached, and it should run through the
connection tracker and establish the state.

Alternatively, without changing the OVS API, we could make the actions
a bit smarter. If you change fields related to CT lookup (ie, the
5tuple) via set actions, then the conntrack entry associated with the
skb is no longer valid for the current packet. Clear the skb->nfct
when manipulating those fields, then when the CT action gets executed,
it can treat this packet as completely new and run it through the
connection tracker to establish the state.

I tend to like the latter because it doesn't involve extending the
API, but I can't help but wonder if the fact we've now got a ct_clear
action at the layer above means that eventually we're going to need
the equivalent functionality in the datapath API. Ie, we could solve
this problem but maybe something more detailed and obscure comes up
later where we're not really satisfying the expectations of the
OpenFlow 'ct_clear' action.

Re: [RFC] [net]openvswitch: Clear the ct flow key for the recirculated packet

[Numan Siddique]

On Fri, Mar 17, 2017 at 5:42 AM, Joe Stringer <joe@ovn.org> wrote:

> On 16 March 2017 at 05:25, Numan Siddique <nusiddiq@redhat.com> wrote:
> > It is possible that the ct flow key information would have
> > gone stale for the packets received from the userspace due to
> > clone or ct_clear actions.
> >
> > In the case of OVN, it adds ping responder flows, which modifies
> > the original icmp4 request packet to a reply packet. It uses the
> > OVS actions - clone and ct_clear. When the reply packet hits the
> > "ovs_ct_execute" function, and since the ct flow key info is not
> > cleared, the connection tracker doesn't set the state to
> > ESTABLISHED state.
> >
> > Note: This patch is marked as RFC, as I am not sure if this is the
> correct
> > place to address this issue or it should be addressed in ovs-vswitchd
> > to set the OVS_KEY_ATTR_CT_STATE and other related attributes
> > properly for ct_clear action.
>
> Hmm. I see a couple of options.
>
> At the moment the ct_clear at the higher layer is not backed by any
> action in the datapath layer. Basically we just clear the fields in
> the flow metadata then from the OpenFlow point of view there is no
> such state, so we can match on it as though it hasn't been through the
> connection tracker, and for instance, try to send it through the
> connection tracker again. If we were to introduce an openvswitch
> kernel API to actually clear the 'skb->nfct' and perhaps the related
> ct flow key fields in the kernel, then subsequent ct lookups would not
> assume that the skb is already cached, and it should run through the
> connection tracker and establish the state.
>
> Alternatively, without changing the OVS API, we could make the actions
> a bit smarter. If you change fields related to CT lookup (ie, the
> 5tuple) via set actions, then the conntrack entry associated with the
> skb is no longer valid for the current packet. Clear the skb->nfct
> when manipulating those fields, then when the CT action gets executed,
> it can treat this packet as completely new and run it through the
> connection tracker to establish the state.
>


​Thanks ​

​for the comments. I will give this options a try.

​

>
> I tend to like the latter because it doesn't involve extending the
> API, but I can't help but wonder if the fact we've now got a ct_clear
> action at the layer above means that eventually we're going to need
> the equivalent functionality in the datapath API. Ie, we could solve
> this problem but maybe something more detailed and obscure comes up
> later where we're not really satisfying the expectations of the
> OpenFlow 'ct_clear' action.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [RFC] [net]openvswitch: Clear the ct flow key for the recirculated packet

[Numan Siddique]

On Fri, Mar 17, 2017 at 2:41 AM, Lance Richardson <lrichard@redhat.com>
wrote:

> > From: "Numan Siddique" <nusiddiq@redhat.com>
> > To: netdev@vger.kernel.org, "ovs dev" <dev@openvswitch.org>
> > Cc: "Joe Stringer" <joe@ovn.org>, "Andy Zhou" <azhou@ovn.org>,
> jarno@ovn.org
> > Sent: Thursday, March 16, 2017 8:25:06 AM
> > Subject: [RFC] [net]openvswitch: Clear the ct flow key for the
> recirculated packet
> >
> > It is possible that the ct flow key information would have
> > gone stale for the packets received from the userspace due to
> > clone or ct_clear actions.
> >
> > In the case of OVN, it adds ping responder flows, which modifies
> > the original icmp4 request packet to a reply packet. It uses the
> > OVS actions - clone and ct_clear. When the reply packet hits the
> > "ovs_ct_execute" function, and since the ct flow key info is not
> > cleared, the connection tracker doesn't set the state to
> > ESTABLISHED state.
> >
> > Note: This patch is marked as RFC, as I am not sure if this is the
> correct
> > place to address this issue or it should be addressed in ovs-vswitchd
> > to set the OVS_KEY_ATTR_CT_STATE and other related attributes
> > properly for ct_clear action.
> >
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > ---
>
> Hi Numan,
>
> With this patch applied I'm consistently seeing failures for two of the
> kernel datapath unit tests (via "make check-kernel"):
>
>  16: conntrack - force commit                        FAILED (
> system-traffic.at:692)
>  54: conntrack - SNAT with ct_mark change on reply   FAILED (
> system-traffic.at:2446)
>
>
Thanks Lance for comments.
I will take care of running the sanity checks next time.

​
_______________________________________________
dev mailing list
dev@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev