netdev - Re: [PATCH net] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS

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: <20200422183701.GN6581@nanopsycho.orion>
Date:   Wed, 22 Apr 2020 20:37:01 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     Florian Westphal <fw@...len.de>,
        Edward Cree <ecree@...arflare.com>,
        netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, kuba@...nel.org
Subject: Re: [PATCH net] net: flow_offload: skip hw stats check for
 FLOW_ACTION_HW_STATS_DISABLED

Mon, Apr 20, 2020 at 09:18:32PM CEST, pablo@...filter.org wrote:
>On Mon, Apr 20, 2020 at 04:14:22PM +0200, Jiri Pirko wrote:
>> Mon, Apr 20, 2020 at 03:57:54PM CEST, fw@...len.de wrote:
>[...]
>> >I mean, the user is forced to use SW datapath just because HW can't turn
>> >off stats?!  Same for a config change, why do i need to change my rules
>> 
>> By default, they are on. That is what user should do in most of the
>> cases.
>
>Fair enough, I can workaround this problem by using
>FLOW_ACTION_HW_STATS_ANY. However, I still don't need counters and
>there is no way to say "I don't care" to the drivers.
>
>Note that the flow_offload infrastructure is used by ethtool,
>netfilter, flowtable and tc these days.
>
>* ethtool's default behaviour is no counters.
>* netfilter's default behaviour is no counters.
>* flowtable's default behaviour is no counters.
>
>
>I understand FLOW_ACTION_HW_STATS_DISABLED means disabled, strictly.
>But would you allow me to introduce FLOW_ACTION_HW_STATS_DONT_CARE to
>fix ethtool, netfilter and flowtable? :-)
>
>FLOW_ACTION_HW_STATS_DONT_CARE means "this front-end doesn't need
>counters, let driver decide what it is best".
>
>Thank you.

>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>index 3619c6acf60f..ae09d1911912 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -164,17 +164,21 @@ enum flow_action_mangle_base {
> };
> 
> enum flow_action_hw_stats_bit {
>+	FLOW_ACTION_HW_STATS_DONT_CARE_BIT,
> 	FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
> 	FLOW_ACTION_HW_STATS_DELAYED_BIT,
> };
> 
> enum flow_action_hw_stats {
> 	FLOW_ACTION_HW_STATS_DISABLED = 0,
>+	FLOW_ACTION_HW_STATS_DONT_CARE =
>+		BIT(FLOW_ACTION_HW_STATS_DONT_CARE_BIT),
> 	FLOW_ACTION_HW_STATS_IMMEDIATE =
> 		BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT),
> 	FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT),
> 	FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_IMMEDIATE |
>-				   FLOW_ACTION_HW_STATS_DELAYED,
>+				   FLOW_ACTION_HW_STATS_DELAYED |
>+				   FLOW_ACTION_HW_STATS_DONT_CARE,

"Any" can't be "don't care". TC User expects stats. That's default.


Let's have "don't care" bit only and set it for
ethtool/netfilter/flowtable. Don't change any. Teach the drivers to deal
with "don't care", most probably using the default checker.


> };
> 
> typedef void (*action_destr)(void *priv);