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
| ||
|
Message-ID: <20190429152128.4mbqyipjv25jiiko@salvia> Date: Mon, 29 Apr 2019 17:21:28 +0200 From: Pablo Neira Ayuso <pablo@...filter.org> To: Edward Cree <ecree@...arflare.com> Cc: Jamal Hadi Salim <jhs@...atatu.com>, netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...nulli.us>, Cong Wang <xiyou.wangcong@...il.com> Subject: Re: TC stats / hw offload question On Mon, Apr 29, 2019 at 03:11:06PM +0100, Edward Cree wrote: > On 26/04/2019 19:49, Pablo Neira Ayuso wrote: > > On Fri, Apr 26, 2019 at 01:13:41PM +0100, Edward Cree wrote: > >> Thus if (and only if) two TC actions have the same tcfa_index, they will > >> share a single counter in the HW. > >> I gathered from a previous conversation with Jamal[1] that that was the > >> correct behaviour: > >>> Note, your counters should also be shareable; example, count all > >>> the drops in one counter across multiple flows as in the following > >>> case where counter index 1 is used. > >>> > >>> tc flower match foo action drop index 1 > >>> tc flower match bar action drop index 1 > > The flow_action_entry structure needs a new 'counter_index' field to > > store this. The tc_setup_flow_action() function needs to be updated > > for this for the FLOW_ACTION_{ACCEPT,DROP,REDIRECT,MIRRED} cases to > > set this entry->counter_index field to tcfa_index, so the driver has > > access to this. > Hmm, I'm still not sure this solves everything. > Before, we could write > tc flower match foo \ > action mirred egress mirror eth1 index 1 \ > action mirred egress redirect eth2 index 2 > and have two distinct HW counters (one of which might e.g. be shared > with another rule). But when reading those counters, under > fl_hw_update_stats(), the driver only gets to return one set of flow > stats for both actions. > Previously, the driver's TC_CLSFLOWER_STATS handler was updating the > action stats directly, so was able to do something different for each > action, but that's not possible in 5.1. At stats gathering time, the > driver doesn't even have access to anything that's per-action and > thus could have a flow_stats member shoved in it. > AFAICT, the only reason this isn't a regression is that existing > drivers didn't implement the old semantics correctly. > > This is a bit of a mess; the best idea I've got is for the > TC_CLSFLOWER_STATS call to include a tcfa_index. Then the driver > returns counter stats for that index, and tcf_exts_stats_update() > only updates those actions whose index matches. But then > fl_hw_update_stats() would have to iterate over all the indices in > f->exts. What do you think? You could extend struct flow_stats to pass an array of stats to the driver, including one stats per action and the counter index. Then, tcf_exts_stats_update() uses this array of stats to update per-action stats. struct flow_action_stats { u32 counter_index; u64 pkts; u64 bytes; u64 lastused; }; struct flow_stats { struct flow_action_stats *stats[]; u32 num_actions; }; As you mentioned, no driver supports for tcfa_index so far, probably it would be a good idea to return -EOPNOTSUPP in such case by now.
Powered by blists - more mailing lists