netdev - Re: [PATCH net] 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 12:08:14 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, kuba@...nel.org, ecree@...arflare.com
Subject: Re: [PATCH net] net: flow_offload: skip hw stats check for
 FLOW_ACTION_HW_STATS_DONT_CARE

On Tue, May 05, 2020 at 11:49:00AM +0200, Jiri Pirko wrote:
> Tue, May 05, 2020 at 11:21:59AM CEST, pablo@...filter.org 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.
> >
> >Remove BUILD_BUG_ON since TCA_ACT_HW_STATS_* don't map 1:1 with
> >FLOW_ACTION_HW_STATS_* anymore. 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>
> >---
> >This is a follow up after "net: flow_offload: skip hw stats check for
> >FLOW_ACTION_HW_STATS_DISABLED". This patch restores the netfilter hardware
> >offloads.
> >
> > include/net/flow_offload.h |  9 ++++++++-
> > net/sched/cls_api.c        | 23 +++++++++++++++++------
> > 2 files changed, 25 insertions(+), 7 deletions(-)
> >
> >diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> >index 3619c6acf60f..0c75163699f0 100644
> >--- a/include/net/flow_offload.h
> >+++ b/include/net/flow_offload.h
> >@@ -164,12 +164,15 @@ enum flow_action_mangle_base {
> > };
> > 
> > enum flow_action_hw_stats_bit {
> >+	FLOW_ACTION_HW_STATS_DISABLED_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 = 0,
> >+	FLOW_ACTION_HW_STATS_DISABLED =
> >+		BIT(FLOW_ACTION_HW_STATS_DISABLED_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),
> >@@ -325,7 +328,11 @@ __flow_action_hw_stats_check(const struct flow_action *action,
> > 		return true;
> > 	if (!flow_action_mixed_hw_stats_check(action, extack))
> > 		return false;
> >+
> > 	action_entry = flow_action_first_entry_get(action);
> >+	if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DONT_CARE)
> >+		return true;
> >+
> > 	if (!check_allow_bit &&
> > 	    action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
> > 		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
> >diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> >index 55bd1429678f..8ddc16a1ca68 100644
> >--- a/net/sched/cls_api.c
> >+++ b/net/sched/cls_api.c
> >@@ -3523,16 +3523,27 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
> > #endif
> > }
> > 
> >+static enum flow_action_hw_stats tc_act_hw_stats_array[] = {
> >+	[0]				= FLOW_ACTION_HW_STATS_DISABLED,
> >+	[TCA_ACT_HW_STATS_IMMEDIATE]	= FLOW_ACTION_HW_STATS_IMMEDIATE,
> >+	[TCA_ACT_HW_STATS_DELAYED]	= FLOW_ACTION_HW_STATS_DELAYED,
> >+	[TCA_ACT_HW_STATS_ANY]		= FLOW_ACTION_HW_STATS_ANY,
> 
> TCA_ACT_HW_* are bits. There can be a combination of those according
> to the user request. For 2 bits, it is not problem, but I have a
> patchset in pipes adding another type.
> Then you need to have 1:1 mapping in this array for all bit
> combinations. That is not right.
> 
> How about putting DISABLED to the at the end of enum
> flow_action_hw_stats? They you can just map 0 here to FLOW_ACTION_HW_STATS_DISABLED
> as an exception, but the bits you can take 1:1.

Another possibility is to remove this mapping code is to update tc
UAPI to get it aligned with this update.

This UAPI is only available in the few 5.7.0-rc kernel releases,
I think only developers are using this at this stage?