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 11:40:10 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Pablo Neira Ayuso <pablo@...filter.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,  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.

>  	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_DISABLED =
> +		BIT(FLOW_ACTION_HW_STATS_DISABLED_BIT),
>  };
>  
>  typedef void (*action_destr)(void *priv);
> @@ -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..56cf1b9e1e24 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3523,6 +3523,16 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
>  #endif
>  }
>  
> +static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats)
> +{
> +	if (WARN_ON_ONCE(hw_stats > TCA_ACT_HW_STATS_ANY))
> +		return FLOW_ACTION_HW_STATS_DONT_CARE;
> +	else if (!hw_stats)
> +		return FLOW_ACTION_HW_STATS_DISABLED;
> +
> +	return hw_stats;
> +}
> +
>  int tc_setup_flow_action(struct flow_action *flow_action,
>  			 const struct tcf_exts *exts)
>  {
> @@ -3546,7 +3556,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>  		if (err)
>  			goto err_out_locked;
>  
> -		entry->hw_stats = act->hw_stats;
> +		entry->hw_stats = tc_act_hw_stats(act->hw_stats);
>  
>  		if (is_tcf_gact_ok(act)) {
>  			entry->id = FLOW_ACTION_ACCEPT;
> @@ -3614,7 +3624,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>  				entry->mangle.mask = tcf_pedit_mask(act, k);
>  				entry->mangle.val = tcf_pedit_val(act, k);
>  				entry->mangle.offset = tcf_pedit_offset(act, k);
> -				entry->hw_stats = act->hw_stats;
> +				entry->hw_stats = tc_act_hw_stats(act->hw_stats);
>  				entry = &flow_action->entries[++j];
>  			}
>  		} else if (is_tcf_csum(act)) {