Date: Thu, 16 Nov 2023 08:36:06 -0300
From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Xin Long <lucien.xin@...il.com>, network dev <netdev@...r.kernel.org>,
	davem@...emloft.net, kuba@...nel.org,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Vladyslav Tarasiuk <vladyslavt@...dia.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net] net: sched: do not offload flows with a helper in
 act_ct

On Wed, Nov 15, 2023 at 11:37:51AM -0500, Jamal Hadi Salim wrote:
> Hi Xin,
> 
> On Tue, Nov 14, 2023 at 10:18 AM Xin Long <lucien.xin@...il.com> wrote:
> >
> > On Mon, Nov 13, 2023 at 4:37 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > >
> > > On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@...il.com> wrote:
> > > >
> > > > There is no hardware supporting ct helper offload. However, prior to this
> > > > patch, a flower filter with a helper in the ct action can be successfully
> > > > set into the HW, for example (eth1 is a bnxt NIC):
> > > >
> > > >   # tc qdisc add dev eth1 ingress_block 22 ingress
> > > >   # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \
> > > >     dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp
> > > >   # tc filter show dev eth1 ingress
> > > >
> > > >     filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1
> > > >       eth_type ipv4
> > > >       ip_proto tcp
> > > >       dst_port 21
> > > >       ct_state -trk
> > > >       skip_sw
> > > >       in_hw in_hw_count 1   <----
> > > >         action order 1: ct zone 0 helper ipv4-tcp-ftp pipe
> > > >          index 2 ref 1 bind 1
> > > >         used_hw_stats delayed
> > > >
> > > > This might cause the flower filter not to work as expected in the HW.
> > > >
> > > > This patch avoids this problem by simply returning -EOPNOTSUPP in
> > > > tcf_ct_offload_act_setup() to not allow to offload flows with a helper
> > > > in act_ct.
> > > >
> > > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> > > > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > >
> > > I didnt quite follow:
> > > The driver accepted the config, but the driver "kind of '' supports
> > > it. (enough to not complain and then display it when queried).
> > > Should the driver have rejected something it doesnt fully support?
> > Hi, Jamal,
> >
> > The sad thing is now it does not pass the 'helper' param to the HW in
> > tcf_ct_offload_act_setup() via struct flow_action_entry, in fact
> > flow_action_entry does not even have a member to keep 'helper'.
> >
> > Since no HW currently supports 'helper', we can stop it setting to HW
> > from here for now. In future, if HWs and struct flow_action_entry
> > support it, we can set it to the entry and reply on HWs to reject
> > it when not supported, as you mentioned above.
> 
> That makes sense - so i am wondering why that was ever added there to
> begin with. Would there be any hardware that would have any helper
> support? If no, Shouldnt that code be deleted altogether?

Not sure if I follow you, Jamal. There's no code at all to pass the
helper information down to the drivers. So drivers ended up accepting
this flow because they had no idea that a helper was attached to it.

Then yes, ideally, it should be driver the one to reject the flow that
it doesn't support. But as currently zero drivers support it, and I
doubt one will in the future [*], this patch simplifies it by instead
of adding all the helper stuff to flow_action_entry, to just abort the
offload earlier.

[*] it requires parsing TCP payload, including over packet boundaries.
This is very expensive in hw. And leads to another problem: the HW
having to tell the SW stack about new conntrack expectations.

Chers,
Marcelo

> 
> In any case, to the code correctness:
> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> 
> cheers,
> jamal
> > Thanks.
> > >
> > >
> > > cheers,
> > > jamal
> > >
> > > > ---
> > > >  include/net/tc_act/tc_ct.h | 9 +++++++++
> > > >  net/sched/act_ct.c         | 3 +++
> > > >  2 files changed, 12 insertions(+)
> > > >
> > > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
> > > > index 8a6dbfb23336..77f87c622a2e 100644
> > > > --- a/include/net/tc_act/tc_ct.h
> > > > +++ b/include/net/tc_act/tc_ct.h
> > > > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a)
> > > >         return to_ct_params(a)->nf_ft;
> > > >  }
> > > >
> > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a)
> > > > +{
> > > > +       return to_ct_params(a)->helper;
> > > > +}
> > > > +
> > > >  #else
> > > >  static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; }
> > > >  static inline int tcf_ct_action(const struct tc_action *a) { return 0; }
> > > > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a)
> > > >  {
> > > >         return NULL;
> > > >  }
> > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a)
> > > > +{
> > > > +       return NULL;
> > > > +}
> > > >  #endif /* CONFIG_NF_CONNTRACK */
> > > >
> > > >  #if IS_ENABLED(CONFIG_NET_ACT_CT)
> > > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > > > index 0db0ecf1d110..b3f4a503ee2b 100644
> > > > --- a/net/sched/act_ct.c
> > > > +++ b/net/sched/act_ct.c
> > > > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data,
> > > >         if (bind) {
> > > >                 struct flow_action_entry *entry = entry_data;
> > > >
> > > > +               if (tcf_ct_helper(act))
> > > > +                       return -EOPNOTSUPP;
> > > > +
> > > >                 entry->id = FLOW_ACTION_CT;
> > > >                 entry->ct.action = tcf_ct_action(act);
> > > >                 entry->ct.zone = tcf_ct_zone(act);
> > > > --
> > > > 2.39.1
> > > >

Powered by blists - more mailing lists