diff mbox series

[net-next] net/sched: act_ct: Offload connections with commit action

Message ID 1620212166-29031-1-git-send-email-paulb@nvidia.com (mailing list archive)
State Deferred
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net/sched: act_ct: Offload connections with commit action | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: jiri@resnulli.us
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 21 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Paul Blakey May 5, 2021, 10:56 a.m. UTC 
Currently established connections are not offloaded if the filter has a
"ct commit" action. This behavior will not offload connections of the
following scenario:

$ tc_filter add dev $DEV ingress protocol ip prio 1 flower \
  ct_state -trk \
  action ct commit action goto chain 1

$ tc_filter add dev $DEV ingress protocol ip chain 1 prio 1 flower \
  action mirred egress redirect dev $DEV2

$ tc_filter add dev $DEV2 ingress protocol ip prio 1 flower \
  action ct commit action goto chain 1

$ tc_filter add dev $DEV2 ingress protocol ip prio 1 chain 1 flower \
  ct_state +trk+est \
  action mirred egress redirect dev $DEV

Offload established connections, regardless of the commit flag.

Reviewed-by: Oz Shlomo <ozsh@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Paul Blakey <paulb@nvidia.com>
---
 net/sched/act_ct.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Marcelo Ricardo Leitner May 5, 2021, 4:39 p.m. UTC | #1 
On Wed, May 05, 2021 at 01:56:06PM +0300, Paul Blakey wrote:
> Currently established connections are not offloaded if the filter has a
> "ct commit" action. This behavior will not offload connections of the
> following scenario:
>
> $ tc_filter add dev $DEV ingress protocol ip prio 1 flower \
>   ct_state -trk \
>   action ct commit action goto chain 1
>
> $ tc_filter add dev $DEV ingress protocol ip chain 1 prio 1 flower \
>   action mirred egress redirect dev $DEV2
>
> $ tc_filter add dev $DEV2 ingress protocol ip prio 1 flower \
>   action ct commit action goto chain 1
>
> $ tc_filter add dev $DEV2 ingress protocol ip prio 1 chain 1 flower \
>   ct_state +trk+est \
>   action mirred egress redirect dev $DEV
>
> Offload established connections, regardless of the commit flag.
>
> Reviewed-by: Oz Shlomo <ozsh@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Paul Blakey <paulb@nvidia.com>
> ---
>  net/sched/act_ct.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index ec7a1c438df9..b1473a1aecdd 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -984,7 +984,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	 */
>  	cached = tcf_ct_skb_nfct_cached(net, skb, p->zone, force);
>  	if (!cached) {
> -		if (!commit && tcf_ct_flow_table_lookup(p, skb, family)) {
> +		if (tcf_ct_flow_table_lookup(p, skb, family)) {

Took me a while to realize that a zone check is not needed here
because when committing to a different zone it will check the new
flowtable here already. Otherwise, for commits, the zone update was
enforced in the few lines below.

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

>  			skip_add = true;
>  			goto do_nat;
>  		}
> @@ -1022,10 +1022,11 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  		 * even if the connection is already confirmed.
>  		 */
>  		nf_conntrack_confirm(skb);
> -	} else if (!skip_add) {
> -		tcf_ct_flow_table_process_conn(p->ct_ft, ct, ctinfo);
>  	}
>
> +	if (!skip_add)
> +		tcf_ct_flow_table_process_conn(p->ct_ft, ct, ctinfo);
> +
>  out_push:
>  	skb_push_rcsum(skb, nh_ofs);
>
> --
> 2.30.1
>
Paul Blakey May 26, 2021, 11:46 a.m. UTC | #2 
On Wed, 5 May 2021, mleitner@redhat.com wrote:

> On Wed, May 05, 2021 at 01:56:06PM +0300, Paul Blakey wrote:
> > Currently established connections are not offloaded if the filter has a
> > "ct commit" action. This behavior will not offload connections of the
> > following scenario:
> >
> > $ tc_filter add dev $DEV ingress protocol ip prio 1 flower \
> >   ct_state -trk \
> >   action ct commit action goto chain 1
> >
> > $ tc_filter add dev $DEV ingress protocol ip chain 1 prio 1 flower \
> >   action mirred egress redirect dev $DEV2
> >
> > $ tc_filter add dev $DEV2 ingress protocol ip prio 1 flower \
> >   action ct commit action goto chain 1
> >
> > $ tc_filter add dev $DEV2 ingress protocol ip prio 1 chain 1 flower \
> >   ct_state +trk+est \
> >   action mirred egress redirect dev $DEV
> >
> > Offload established connections, regardless of the commit flag.
> >
> > Reviewed-by: Oz Shlomo <ozsh@nvidia.com>
> > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > Signed-off-by: Paul Blakey <paulb@nvidia.com>
> > ---
> >  net/sched/act_ct.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > index ec7a1c438df9..b1473a1aecdd 100644
> > --- a/net/sched/act_ct.c
> > +++ b/net/sched/act_ct.c
> > @@ -984,7 +984,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
> >  	 */
> >  	cached = tcf_ct_skb_nfct_cached(net, skb, p->zone, force);
> >  	if (!cached) {
> > -		if (!commit && tcf_ct_flow_table_lookup(p, skb, family)) {
> > +		if (tcf_ct_flow_table_lookup(p, skb, family)) {
> 
> Took me a while to realize that a zone check is not needed here
> because when committing to a different zone it will check the new
> flowtable here already. Otherwise, for commits, the zone update was
> enforced in the few lines below.
> 
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 

Thanks,
Sent to net as a fix.

Paul.

> >  			skip_add = true;
> >  			goto do_nat;
> >  		}
> > @@ -1022,10 +1022,11 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
> >  		 * even if the connection is already confirmed.
> >  		 */
> >  		nf_conntrack_confirm(skb);
> > -	} else if (!skip_add) {
> > -		tcf_ct_flow_table_process_conn(p->ct_ft, ct, ctinfo);
> >  	}
> >
> > +	if (!skip_add)
> > +		tcf_ct_flow_table_process_conn(p->ct_ft, ct, ctinfo);
> > +
> >  out_push:
> >  	skb_push_rcsum(skb, nh_ofs);
> >
> > --
> > 2.30.1
> >
> 
>
diff mbox series

Patch

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index ec7a1c438df9..b1473a1aecdd 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -984,7 +984,7 @@  static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 	 */
 	cached = tcf_ct_skb_nfct_cached(net, skb, p->zone, force);
 	if (!cached) {
-		if (!commit && tcf_ct_flow_table_lookup(p, skb, family)) {
+		if (tcf_ct_flow_table_lookup(p, skb, family)) {
 			skip_add = true;
 			goto do_nat;
 		}
@@ -1022,10 +1022,11 @@  static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 		 * even if the connection is already confirmed.
 		 */
 		nf_conntrack_confirm(skb);
-	} else if (!skip_add) {
-		tcf_ct_flow_table_process_conn(p->ct_ft, ct, ctinfo);
 	}
 
+	if (!skip_add)
+		tcf_ct_flow_table_process_conn(p->ct_ft, ct, ctinfo);
+
 out_push:
 	skb_push_rcsum(skb, nh_ofs);