Re: [PATCH net] net/sched: act_ct: Fix ct template allocation for zone 0

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: Ariel Levkovich <lariel@nvidia.com>
Cc: netdev@vger.kernel.org, paulb@nvidia.com, jiri@resnulli.us
Subject: Re: [PATCH net] net/sched: act_ct: Fix ct template allocation for zone 0
Date: Thu, 27 May 2021 12:50:38 -0300	[thread overview]
Message-ID: <YK+/zn0R+M4lYfC+@horizon.localdomain> (raw)
In-Reply-To: <021dab3f-9ca3-ffeb-b18a-24c9207a7000@nvidia.com>

On Thu, May 27, 2021 at 11:36:18AM -0400, Ariel Levkovich wrote:
> 
> On 5/26/21 9:49 PM, Marcelo Ricardo Leitner wrote:
> > On Wed, May 26, 2021 at 08:01:10PM +0300, Ariel Levkovich wrote:
> > > Fix current behavior of skipping template allocation in case the
> > > ct action is in zone 0.
> > > 
> > > Skipping the allocation may cause the datapath ct code to ignore the
> > > entire ct action with all its attributes (commit, nat) in case the ct
> > > action in zone 0 was preceded by a ct clear action.
> > > 
> > > The ct clear action sets the ct_state to untracked and resets the
> > > skb->_nfct pointer. Under these conditions and without an allocated
> > > ct template, the skb->_nfct pointer will remain NULL which will
> > > cause the tc ct action handler to exit without handling commit and nat
> > > actions, if such exist.
> > > 
> > > For example, the following rule in OVS dp:
> > > recirc_id(0x2),ct_state(+new-est-rel-rpl+trk),ct_label(0/0x1), \
> > > in_port(eth0),actions:ct_clear,ct(commit,nat(src=10.11.0.12)), \
> > > recirc(0x37a)
> > > 
> > > Will result in act_ct skipping the commit and nat actions in zone 0.
> > > 
> > > The change removes the skipping of template allocation for zone 0 and
> > > treats it the same as any other zone.
> > > 
> > > Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
> > > Signed-off-by: Ariel Levkovich <lariel@nvidia.com>
> > > ---
> > >   net/sched/act_ct.c | 3 ---
> > Hah! I guess I had looked only at netfilter code regarding
> > NF_CT_DEFAULT_ZONE_ID.
> > 
> > >   1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > > index ec7a1c438df9..dfdfb677e6a9 100644
> > > --- a/net/sched/act_ct.c
> > > +++ b/net/sched/act_ct.c
> > > @@ -1202,9 +1202,6 @@ static int tcf_ct_fill_params(struct net *net,
> > >   				   sizeof(p->zone));
> > >   	}
> > > -	if (p->zone == NF_CT_DEFAULT_ZONE_ID)
> > > -		return 0;
> > > -
> > This patch makes act_ct behave like ovs kernel datapath, but I'm not
> > sure ovs kernel is doing the right thing. :-) (jump to last paragraph
> > for my suggestion, might ease the reading)
> > 
> > As you described:
> > "The ct clear action sets the ct_state to untracked and resets the
> > skb->_nfct pointer." I think the problem lies on the first part, on
> > setting it to untracked.
> > 
> > That was introduced in ovs kernel on commit b8226962b1c4
> > ("openvswitch: add ct_clear action") and AFAICT the idea there was to
> > "reset it to original state" [A].
> > 
> > Then ovs userspace has commit 0cdfddddb664 ("datapath: add ct_clear
> > action") as well, a mirror of the one above. There, it is noted:
> > "   - if IP_CT_UNTRACKED is not available use 0 as other nf_ct_set()
> >       calls do. Since we're setting ct to NULL this is okay."
> > 
> > Thing is, IP_CT_ESTABLISHED is the first member of enum
> > ip_conntrack_info and evalutes 0, while IP_CT_UNTRACKED is actually:
> > include/uapi/linux/netfilter/nf_conntrack_common.h:
> >          /* only for userspace compatibility */
> > #ifndef __KERNEL__
> >          IP_CT_NEW_REPLY = IP_CT_NUMBER,
> > #else
> >          IP_CT_UNTRACKED = 7,
> > #endif
> > 
> > In the commits above, none of them mention that the packet should be
> > set to Untracked. That's a different thing than "undoing CT"..
> > That setting untrack here is the equivalent of:
> >    # iptables -A ... -j CT --notrack
> > 
> > Then, when it finally reaches nf_conntrack_in:
> >            tmpl = nf_ct_get(skb, &ctinfo);
> > 	      vvvv--- NULL if from act_ct and zone 0, !NULL if from ovs
> >            if (tmpl || ctinfo == IP_CT_UNTRACKED) {
> > 	                     ^^-- always true after ct_clear
> >                    /* Previously seen (loopback or untracked)?  Ignore. */
> >                    if ((tmpl && !nf_ct_is_template(tmpl)) ||
> >                         ctinfo == IP_CT_UNTRACKED)
> > 		              ^^--- always true..
> >                            return NF_ACCEPT;
> > 			  ^^ returns her
> >                    skb->_nfct = 0;
> >            }
> > 
> > If ct_clear (act_ct and ovs) instead set it 0 (which, well, it's odd
> > but maps to IP_CT_ESTABLISHED), it wouldn't match on the conditions
> > above, and would do CT normally.
> > 
> > With all that, what about keeping the check here, as it avoids an
> > allocation that AFAICT is superfluous when not setting a zone != 0 and
> > atomics for _get/_put, and changing ct_clear instead (act_ct and ovs),
> > to NOT set the packet as IP_CT_UNTRACKED?
> > 
> > Thanks,
> > Marcelo
> 
> I understand your point. But if we go in this path, that means going into
> zone 0,
> 
> skb will not be associated with zone 0 unless there was a ct_clear action
> prior to that.
> 
> skb->_nfct will carry the pointer from previous zone. I see several
> scenarios where this will
> 
> be problematic.

I don't follow why. When the skb is created, skb->_nfct is "entirely NULL",
right? Done by the memset() on __alloc_skb().

Then,

@@ -950,7 +950,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
                ct = nf_ct_get(skb, &ctinfo);
                if (ct) {
                        nf_conntrack_put(&ct->ct_general);
-                       nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
+                       nf_ct_set(skb, NULL, 0);
                }

                goto out_clear;

would restore it to that state and not have a pointer to the previous
zone in skb->_nfct.

Thanks,
  Marcelo

> 
> 
> Thanks,
> 
> Ariel
> 
> > 
> > >   	nf_ct_zone_init(&zone, p->zone, NF_CT_DEFAULT_ZONE_DIR, 0);
> > >   	tmpl = nf_ct_tmpl_alloc(net, &zone, GFP_KERNEL);
> > >   	if (!tmpl) {
> > > -- 
> > > 2.25.2
> > > 
