netdev - Re: [patch net-next 00/10] net: allow user specify TC filter 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]

Message-ID: <20200229080120.GP26061@nanopsycho>
Date:   Sat, 29 Feb 2020 09:01:20 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Edward Cree <ecree@...arflare.com>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, 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, xiyou.wangcong@...il.com,
        mlxsw@...lanox.com, Marian Pritsak <marianp@...lanox.com>
Subject: Re: [patch net-next 00/10] net: allow user specify TC filter HW
 stats type

Fri, Feb 28, 2020 at 08:59:09PM CET, pablo@...filter.org wrote:
>Hi Pirko,
>
>On Tue, Feb 25, 2020 at 05:22:03PM +0100, Jiri Pirko wrote:
>[...]
>> Eh, that is not that simple. The existing users are used to the fact
>> that the actions are providing counters by themselves. Having and
>> explicit counter action like this would break that expectation.
>> Also, I think it should be up to the driver implementation. Some HW
>> might only support stats per rule, not the actions. Driver should fit
>> into the existing abstraction, I think it is fine.
>
>Something like the sketch patch that I'm attaching?

But why? Actions are separate entities, with separate counters. The
driver is either able to offload that or not. Up to the driver to
abstract this out.


>
>The idea behind it is to provide a counter action through the
>flow_action API. Then, tc_setup_flow_action() checks if this action
>comes with counters in that case the counter action is added.
>
>My patch assumes tcf_vlan_counter() provides tells us what counter
>type the user wants, I just introduced this to provide an example.
>
>Thank you.

>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>index c6f7bd22db60..1a5006091edc 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -138,9 +138,16 @@ enum flow_action_id {
> 	FLOW_ACTION_MPLS_PUSH,
> 	FLOW_ACTION_MPLS_POP,
> 	FLOW_ACTION_MPLS_MANGLE,
>+	FLOW_ACTION_COUNTER,
> 	NUM_FLOW_ACTIONS,
> };
> 
>+enum flow_action_counter_type {
>+	FLOW_COUNTER_DISABLED		= 0,
>+	FLOW_COUNTER_DELAYED,
>+	FLOW_COUNTER_IMMEDIATE,
>+};
>+
> /* This is mirroring enum pedit_header_type definition for easy mapping between
>  * tc pedit action. Legacy TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK is mapped to
>  * FLOW_ACT_MANGLE_UNSPEC, which is supported by no driver.
>@@ -213,6 +220,9 @@ struct flow_action_entry {
> 			u8		bos;
> 			u8		ttl;
> 		} mpls_mangle;
>+		struct {				/* FLOW_ACTION_COUNTER */
>+			enum flow_action_counter_type	type;
>+		} counter;
> 	};
> };
> 
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index 13c33eaf1ca1..984f2129c760 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -3435,6 +3435,7 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
> int tc_setup_flow_action(struct flow_action *flow_action,
> 			 const struct tcf_exts *exts)
> {
>+	enum flow_action_counter_type counter = FLOW_COUNTER_DISABLED;
> 	struct tc_action *act;
> 	int i, j, k, err = 0;
> 
>@@ -3489,6 +3490,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> 				err = -EOPNOTSUPP;
> 				goto err_out_locked;
> 			}
>+			counter = tcf_vlan_counter(act);
> 		} else if (is_tcf_tunnel_set(act)) {
> 			entry->id = FLOW_ACTION_TUNNEL_ENCAP;
> 			err = tcf_tunnel_encap_get_tunnel(entry, act);
>@@ -3567,10 +3569,19 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> 			err = -EOPNOTSUPP;
> 			goto err_out_locked;
> 		}
>-		spin_unlock_bh(&act->tcfa_lock);
> 
> 		if (!is_tcf_pedit(act))
> 			j++;
>+
>+		if (counter) {
>+			struct flow_action_entry *entry;
>+
>+			entry = &flow_action->entries[j++];
>+			entry->id = FLOW_ACTION_COUNTER;
>+			entry->counter.type = counter;
>+			counter = FLOW_COUNTER_DISABLED;
>+		}
>+		spin_unlock_bh(&act->tcfa_lock);
> 	}
> 
> err_out: