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]

Date: Tue, 14 Nov 2023 10:18:42 -0500
From: Xin Long <lucien.xin@...il.com>
To: Jamal Hadi Salim <jhs@...atatu.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

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.

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