netdev - Re: [PATCH net] net: sched: do not offload flows with a helper in act

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMmMMMyxsktxCezjw-oePU-Lqsw2MMwMA5_hOLXiv5i4WA@mail.gmail.com>
Date: Wed, 15 Nov 2023 11:37:51 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Xin Long <lucien.xin@...il.com>
Cc: 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>, Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net] net: sched: do not offload flows with a helper in act_ct

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?

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