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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 26 May 2021 14:46:24 +0300
From:   Paul Blakey <paulb@...dia.com>
To:     <mleitner@...hat.com>
CC:     Jamal Hadi Salim <jhs@...atatu.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Cong Wang <xiyou.wangcong@...il.com>, <netdev@...r.kernel.org>,
        Oz Shlomo <ozsh@...dia.com>, Roi Dayan <roid@...dia.com>,
        Jiri Pirko <jiri@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Vlad Buslov <vladbu@...dia.com>
Subject: Re: [PATCH net-next] net/sched: act_ct: Offload connections with
 commit action



On Wed, 5 May 2021, mleitner@...hat.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@...dia.com>
> > Reviewed-by: Jiri Pirko <jiri@...dia.com>
> > Signed-off-by: Paul Blakey <paulb@...dia.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@...il.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
> >
> 
>