[PATCH net] openvswitch: fix conntrack netlink event delivery

[PATCH net] openvswitch: fix conntrack netlink event delivery

[Samuel Gauthier]

Only the first and last netlink message for a particular conntrack are
actually sent. The first message is sent through nf_conntrack_confirm when
the conntrack is committed. The last one is sent when the conntrack is
destroyed on timeout. The other conntrack state change messages are not
advertised.

When the conntrack subsystem is used from netfilter, nf_conntrack_confirm
is called for each packet, from the postrouting hook, which in turn calls
nf_ct_deliver_cached_events to send the state change netlink messages.

This commit fixes the problem by calling nf_conntrack_confirm all the time,
i.e not only in the commit case.

Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
CC: Joe Stringer <joestringer@nicira.com>
CC: Justin Pettit <jpettit@nicira.com>
CC: Andy Zhou <azhou@nicira.com>
CC: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---
This patch was tested against the net tree, checking the notifications with
conntrack -E.

David, this patch conflicts with the patch 7d904c7bcd51 ("openvswitch: Only
set mark and labels with a commit flag.") from net-next. I can help solving
the conflict if you need to.

 net/openvswitch/conntrack.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 3d5feede962d..4ea97f1c3861 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -834,9 +834,6 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
 	err = __ovs_ct_lookup(net, key, info, skb);
 	if (err)
 		return err;
-	/* This is a no-op if the connection has already been confirmed. */
-	if (nf_conntrack_confirm(skb) != NF_ACCEPT)
-		return -EINVAL;
 
 	return 0;
 }
@@ -888,6 +885,11 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 	if (labels_nonzero(&info->labels.mask))
 		err = ovs_ct_set_labels(skb, key, &info->labels.value,
 					&info->labels.mask);
+
+	/* This is a no-op if the connection has already been confirmed. */
+	if (nf_conntrack_confirm(skb) != NF_ACCEPT)
+		return -EINVAL;
+
 err:
 	skb_push(skb, nh_ofs);
 	if (err)
-- 
2.2.1.62.g3f15098

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net] openvswitch: fix conntrack netlink event delivery

[Joe Stringer]

On 28 June 2016 at 14:12, Samuel Gauthier <samuel.gauthier@6wind.com> wrote:
> Only the first and last netlink message for a particular conntrack are
> actually sent. The first message is sent through nf_conntrack_confirm when
> the conntrack is committed. The last one is sent when the conntrack is
> destroyed on timeout. The other conntrack state change messages are not
> advertised.
>
> When the conntrack subsystem is used from netfilter, nf_conntrack_confirm
> is called for each packet, from the postrouting hook, which in turn calls
> nf_ct_deliver_cached_events to send the state change netlink messages.
>
> This commit fixes the problem by calling nf_conntrack_confirm all the time,
> i.e not only in the commit case.
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> CC: Joe Stringer <joestringer@nicira.com>
> CC: Justin Pettit <jpettit@nicira.com>
> CC: Andy Zhou <azhou@nicira.com>
> CC: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>

This breaks the semantics of OVS_CT_ATTR_COMMIT. If you just want to
ensure that nf_ct_deliver_cached_events() is run, then we should call
to that for confirmed connections in the non-commit case.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[PATCH net v2] openvswitch: fix conntrack netlink event delivery

[Samuel Gauthier]

Only the first and last netlink message for a particular conntrack are
actually sent. The first message is sent through nf_conntrack_confirm when
the conntrack is committed. The last one is sent when the conntrack is
destroyed on timeout. The other conntrack state change messages are not
advertised.

When the conntrack subsystem is used from netfilter, nf_conntrack_confirm
is called for each packet, from the postrouting hook, which in turn calls
nf_ct_deliver_cached_events to send the state change netlink messages.

This commit fixes the problem by calling nf_ct_deliver_cached_events in the
non-commit case as well.

Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
CC: Joe Stringer <joestringer@nicira.com>
CC: Justin Pettit <jpettit@nicira.com>
CC: Andy Zhou <azhou@nicira.com>
CC: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---

v1 -> v2: don't break OVS_CT_ATTR_COMMIT attribute

This patch was tested against the net tree, checking the notifications with
conntrack -E.

 net/openvswitch/conntrack.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 3d5feede962d..d84312584ee4 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -818,8 +818,18 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 		 */
 		state = OVS_CS_F_TRACKED | OVS_CS_F_NEW | OVS_CS_F_RELATED;
 		__ovs_ct_update_key(key, state, &info->zone, exp->master);
-	} else
-		return __ovs_ct_lookup(net, key, info, skb);
+	} else {
+		struct nf_conn *ct;
+		int err;
+
+		err = __ovs_ct_lookup(net, key, info, skb);
+		if (err)
+			return err;
+
+		ct = (struct nf_conn *)skb->nfct;
+		if (ct)
+			nf_ct_deliver_cached_events(ct);
+	}
 
 	return 0;
 }
-- 
2.2.1.62.g3f15098

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH net v2] openvswitch: fix conntrack netlink event delivery

[Joe Stringer]

On 28 June 2016 at 17:22, Samuel Gauthier <samuel.gauthier@6wind.com> wrote:
> Only the first and last netlink message for a particular conntrack are
> actually sent. The first message is sent through nf_conntrack_confirm when
> the conntrack is committed. The last one is sent when the conntrack is
> destroyed on timeout. The other conntrack state change messages are not
> advertised.
>
> When the conntrack subsystem is used from netfilter, nf_conntrack_confirm
> is called for each packet, from the postrouting hook, which in turn calls
> nf_ct_deliver_cached_events to send the state change netlink messages.
>
> This commit fixes the problem by calling nf_ct_deliver_cached_events in the
> non-commit case as well.
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> CC: Joe Stringer <joestringer@nicira.com>
> CC: Justin Pettit <jpettit@nicira.com>
> CC: Andy Zhou <azhou@nicira.com>
> CC: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>

Acked-by: Joe Stringer <joe@ovn.org>

Re: [PATCH net v2] openvswitch: fix conntrack netlink event delivery

[David Miller]

From: Samuel Gauthier <samuel.gauthier@6wind.com>
Date: Tue, 28 Jun 2016 17:22:26 +0200

> Only the first and last netlink message for a particular conntrack are
> actually sent. The first message is sent through nf_conntrack_confirm when
> the conntrack is committed. The last one is sent when the conntrack is
> destroyed on timeout. The other conntrack state change messages are not
> advertised.
> 
> When the conntrack subsystem is used from netfilter, nf_conntrack_confirm
> is called for each packet, from the postrouting hook, which in turn calls
> nf_ct_deliver_cached_events to send the state change netlink messages.
> 
> This commit fixes the problem by calling nf_ct_deliver_cached_events in the
> non-commit case as well.
> 
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> CC: Joe Stringer <joestringer@nicira.com>
> CC: Justin Pettit <jpettit@nicira.com>
> CC: Andy Zhou <azhou@nicira.com>
> CC: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>

Applied.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev