[ovs-dev,11/11] datapath: Allow attaching helper in later commit
diff mbox series

Message ID 1571074671-31834-12-git-send-email-yihung.wei@gmail.com
State New
Headers show
Series
  • Backport upstream conntrack related patches
Related show

Commit Message

Yi-Hung Wei Oct. 14, 2019, 5:37 p.m. UTC
Upstream commit:
commit 248d45f1e1934f7849fbdc35ef1e57151cf063eb
Author: Yi-Hung Wei <yihung.wei@gmail.com>
Date:   Fri Oct 4 09:26:44 2019 -0700

    openvswitch: Allow attaching helper in later commit

    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>
    Signed-off-by: David S. Miller <davem@davemloft.net>

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

Comments

Yifeng Sun Oct. 14, 2019, 11:36 p.m. UTC | #1
LGTM, thanks.

Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>

On Mon, Oct 14, 2019 at 10:56 AM Yi-Hung Wei <yihung.wei@gmail.com> wrote:
>
> Upstream commit:
> commit 248d45f1e1934f7849fbdc35ef1e57151cf063eb
> Author: Yi-Hung Wei <yihung.wei@gmail.com>
> Date:   Fri Oct 4 09:26:44 2019 -0700
>
>     openvswitch: Allow attaching helper in later commit
>
>     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>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
>  datapath/conntrack.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index f6e9386f4707..838cf63c908f 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1045,6 +1045,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
> @@ -1062,16 +1064,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)) {
> @@ -1081,11 +1084,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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox series

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index f6e9386f4707..838cf63c908f 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1045,6 +1045,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
@@ -1062,16 +1064,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)) {
@@ -1081,11 +1084,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;
 		}