netdev - Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

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:   Mon, 2 Mar 2020 14:20:16 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        saeedm@...lanox.com, leon@...nel.org, michael.chan@...adcom.com,
        vishal@...lsio.com, jeffrey.t.kirsher@...el.com,
        idosch@...lanox.com, aelior@...vell.com, peppe.cavallaro@...com,
        alexandre.torgue@...com, jhs@...atatu.com,
        xiyou.wangcong@...il.com, ecree@...arflare.com, mlxsw@...lanox.com,
        netfilter-devel@...r.kernel.org
Subject: Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW
 stats type

On Sun, Mar 01, 2020 at 09:44:43AM +0100, Jiri Pirko wrote:
> Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@...filter.org wrote:
> >On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote:
[...]
> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> >> index 4e864c34a1b0..eee1cbc5db3c 100644
> >> --- a/include/net/flow_offload.h
> >> +++ b/include/net/flow_offload.h
> >> @@ -154,6 +154,10 @@ enum flow_action_mangle_base {
> >>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
> >>  };
> >>  
> >> +enum flow_action_hw_stats_type {
> >> +	FLOW_ACTION_HW_STATS_TYPE_ANY,
> >> +};
> >> +
> >>  typedef void (*action_destr)(void *priv);
> >>  
> >>  struct flow_action_cookie {
> >> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
> >>  
> >>  struct flow_action_entry {
> >>  	enum flow_action_id		id;
> >> +	enum flow_action_hw_stats_type	hw_stats_type;
> >>  	action_destr			destructor;
> >>  	void				*destructor_priv;
> >>  	union {
> >> @@ -228,6 +233,7 @@ struct flow_action_entry {
> >>  };
> >>  
> >>  struct flow_action {
> >> +	bool				mixed_hw_stats_types;
> >
> >Why do you want to place this built-in into the struct flow_action as
> >a boolean?
> 
> Because it is convenient for the driver to know if multiple hw_stats_type
> values are used for multiple actions.
> 
> >You can express the same thing through a new FLOW_ACTION_COUNTER.
[...]
> >Please, explain me why it would be a problem from the driver side to
> >provide a separated counter action.
> 
> I don't see any point in doing that. The action itself implies that has
> stats, you don't need a separate action for that for the flow_offload
> abstraction layer. What you would end up with is:
> counter_action1, actual_action1, counter_action2, actual_action2,...
> 
> What is the point of that?

Yes, it's a bit more work for tc to generate counter action + actual
action.

However, netfilter has two ways to use counters:

1) per-rule counter, in this case the counter is updated after rule
   matching, right before calling the action. This is the legacy mode.

2) explicit counter action, in this case the user specifies explicitly
   that it needs a counter in a given position of the rule. This
   counter might come before or after the actual action.

ethtool does not have counters yet. Now there is a netlink interface
for it, there might be counters there at some point.

I'm suggesting a model that would work for the existing front-ends
using the flow_action API.