[PATCH] openvswitch: Allow attaching helper in later commit

[PATCH] openvswitch: Allow attaching helper in later commit

[Yi-Hung Wei]

This patch allows to attach conntrack helper to a confirmed conntrack
entry.  Currently, we can only attach alg helper to a conntrack entry
when it is in the unconfirmed state.  This patch enables an use case
that we can firstly commit a conntrack entry after it passed some
initial conditions.  After that the processing pipeline will further
check a couple of packets to determine if the connection belongs to
a particular application, and attach alg helper to the connection
in a later stage.

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 net/openvswitch/conntrack.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 05249eb45082..4eb3d2748b65 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -971,6 +971,8 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (ct) {
+		bool add_helper = false;
+
 		/* Packets starting a new connection must be NATted before the
 		 * helper, so that the helper knows about the NAT.  We enforce
 		 * this by delaying both NAT and helper calls for unconfirmed
@@ -988,16 +990,17 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 		}
 
 		/* Userspace may decide to perform a ct lookup without a helper
-		 * specified followed by a (recirculate and) commit with one.
-		 * Therefore, for unconfirmed connections which we will commit,
-		 * we need to attach the helper here.
+		 * specified followed by a (recirculate and) commit with one,
+		 * or attach a helper in a later commit.  Therefore, for
+		 * connections which we will commit, we may need to attach
+		 * the helper here.
 		 */
-		if (!nf_ct_is_confirmed(ct) && info->commit &&
-		    info->helper && !nfct_help(ct)) {
+		if (info->commit && info->helper && !nfct_help(ct)) {
 			int err = __nf_ct_try_assign_helper(ct, info->ct,
 							    GFP_ATOMIC);
 			if (err)
 				return err;
+			add_helper = true;
 
 			/* helper installed, add seqadj if NAT is required */
 			if (info->nat && !nfct_seqadj(ct)) {
@@ -1007,11 +1010,13 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 		}
 
 		/* Call the helper only if:
-		 * - nf_conntrack_in() was executed above ("!cached") for a
-		 *   confirmed connection, or
+		 * - nf_conntrack_in() was executed above ("!cached") or a
+		 *   helper was just attached ("add_helper") for a confirmed
+		 *   connection, or
 		 * - When committing an unconfirmed connection.
 		 */
-		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit) &&
+		if ((nf_ct_is_confirmed(ct) ? !cached | add_helper :
+					      info->commit) &&
 		    ovs_ct_helper(skb, info->family) != NF_ACCEPT) {
 			return -EINVAL;
 		}
-- 
2.7.4

Re: [PATCH] openvswitch: Allow attaching helper in later commit

[David Miller]

From: Yi-Hung Wei <yihung.wei@gmail.com>
Date: Mon, 30 Sep 2019 12:39:04 -0700

> -		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit) &&
> +		if ((nf_ct_is_confirmed(ct) ? !cached | add_helper :

I would suggest using "||" instea of "|" here since you are computing
a boolean.

Re: [PATCH] openvswitch: Allow attaching helper in later commit

[Yi-Hung Wei]

On Thu, Oct 3, 2019 at 8:31 AM David Miller <davem@davemloft.net> wrote:
>
> From: Yi-Hung Wei <yihung.wei@gmail.com>
> Date: Mon, 30 Sep 2019 12:39:04 -0700
>
> > -             if ((nf_ct_is_confirmed(ct) ? !cached : info->commit) &&
> > +             if ((nf_ct_is_confirmed(ct) ? !cached | add_helper :
>
> I would suggest using "||" instea of "|" here since you are computing
> a boolean.

Thanks for review.  It makes sense to use "||" rather than "|".  I
will wait a bit to gather more feedback before I send v2.

Thanks,

-Yi-Hung