netdev - Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT

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, 5 May 2020 21:31:45 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, jiri@...nulli.us, ecree@...arflare.com
Subject: Re: [PATCH net,v2] net: flow_offload: skip hw stats check for
 FLOW_ACTION_HW_STATS_DONT_CARE

On Tue, May 05, 2020 at 11:40:10AM -0700, Jakub Kicinski wrote:
> On Tue,  5 May 2020 19:47:36 +0200 Pablo Neira Ayuso wrote:
> > This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver
> > that the frontend does not need counters, this hw stats type request
> > never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests
> > the driver to disable the stats, however, if the driver cannot disable
> > counters, it bails out.
> > 
> > TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_*
> > except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED
> > (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between
> > TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*.
> > 
> > Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
> > Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> > ---
> > v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration
> >     as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_*
> >     and FLOW_ACTION_HW_STATS_* except by the disabled case.
> > 
> >  include/net/flow_offload.h |  9 ++++++++-
> >  net/sched/cls_api.c        | 14 ++++++++++++--
> >  2 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index 3619c6acf60f..efc8350b42fb 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -166,15 +166,18 @@ enum flow_action_mangle_base {
> >  enum flow_action_hw_stats_bit {
> >  	FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
> >  	FLOW_ACTION_HW_STATS_DELAYED_BIT,
> > +	FLOW_ACTION_HW_STATS_DISABLED_BIT,
> >  };
> >  
> >  enum flow_action_hw_stats {
> > -	FLOW_ACTION_HW_STATS_DISABLED = 0,
> > +	FLOW_ACTION_HW_STATS_DONT_CARE = 0,
> 
> Why not ~0? Or ANY | DISABLED? 
> Otherwise you may confuse drivers which check bit by bit.

I'm confused, you agreed with this behaviour:

https://lore.kernel.org/netfilter-devel/20200427111220.7b07aae1@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/T/#m6091486b2b0ddac512fe6c17f5508f280f630b60