netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] net/sched: act_ct: Offload connections with commit action
@ 2021-05-05 10:56 Paul Blakey
  2021-05-05 16:39 ` mleitner
  0 siblings, 1 reply; 3+ messages in thread
From: Paul Blakey @ 2021-05-05 10:56 UTC (permalink / raw)
  To: Paul Blakey, Jamal Hadi Salim, David S. Miller, Jakub Kicinski,
	Cong Wang, netdev
  Cc: Oz Shlomo, Roi Dayan, Jiri Pirko, Saeed Mahameed, Vlad Buslov

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)) {
 			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


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net-next] net/sched: act_ct: Offload connections with commit action
  2021-05-05 10:56 [PATCH net-next] net/sched: act_ct: Offload connections with commit action Paul Blakey
@ 2021-05-05 16:39 ` mleitner
  2021-05-26 11:46   ` Paul Blakey
  0 siblings, 1 reply; 3+ messages in thread
From: mleitner @ 2021-05-05 16:39 UTC (permalink / raw)
  To: Paul Blakey
  Cc: Jamal Hadi Salim, David S. Miller, Jakub Kicinski, Cong Wang,
	netdev, Oz Shlomo, Roi Dayan, Jiri Pirko, Saeed Mahameed,
	Vlad Buslov

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
>


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net-next] net/sched: act_ct: Offload connections with commit action
  2021-05-05 16:39 ` mleitner
@ 2021-05-26 11:46   ` Paul Blakey
  0 siblings, 0 replies; 3+ messages in thread
From: Paul Blakey @ 2021-05-26 11:46 UTC (permalink / raw)
  To: mleitner
  Cc: Jamal Hadi Salim, David S. Miller, Jakub Kicinski, Cong Wang,
	netdev, Oz Shlomo, Roi Dayan, Jiri Pirko, Saeed Mahameed,
	Vlad Buslov



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
> >
> 
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-26 11:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05 10:56 [PATCH net-next] net/sched: act_ct: Offload connections with commit action Paul Blakey
2021-05-05 16:39 ` mleitner
2021-05-26 11:46   ` Paul Blakey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).