netdev - Re: [RFC PATCH v2 net-next 0/3] flow_offload: Re-add per-action statistics

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 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, 20 May 2019 11:38:14 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Edward Cree <ecree@...arflare.com>, Jiri Pirko <jiri@...nulli.us>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        David Miller <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Vishal Kulkarni <vishal@...lsio.com>
Subject: Re: [RFC PATCH v2 net-next 0/3] flow_offload: Re-add per-action
 statistics

On 2019-05-20 11:26 a.m., Edward Cree wrote:
> On 18/05/2019 21:39, Jamal Hadi Salim wrote:
>> On 2019-05-17 1:14 p.m., Edward Cree wrote:
>>> On 17/05/2019 16:27, Edward Cree wrote:

> Unless *I* missed something, I'm not changing the TC<=>user-space API at
>   all.  If user space specifies an index, then TC will either create a new
>   action with that index, or find an existing one.  Then flow_offload turns
>   that into a cookie; in the 'existing action' case it'll be the same
>   cookie as any previous offloads of that action, in the 'new action' case
>   it'll be a cookie distinct from any existing action.

That is fine then if i could do:

tc actions add action drop index 104
then
followed by for example the two filters you show below..

Is your hardware not using explicit indices into a stats table?

> Drivers aren't interested in the specific index value, only in "which
>   other actions (counters) I've offloaded are shared with this one?", which
>   the cookie gives them.
> 
> With my (unreleased) driver code, I've successfully tested this with e.g.
>   the following rules:
> tc filter add dev $vfrep parent ffff: protocol arp flower skip_sw \
>      action vlan push id 100 protocol 802.1q \
>      action mirred egress mirror dev $pf index 101 \
>      action vlan pop \
>      action drop index 104
> tc filter add dev $vfrep parent ffff: protocol ipv4 flower skip_sw \
>      action vlan push id 100 protocol 802.1q \
>      action mirred egress mirror dev $pf index 102 \
>      action vlan pop \
>      action drop index 104
> 
> Then when viewing with `tc -stats filter show`, the mirreds count their
>   traffic separately (and with an extra 4 bytes per packet for the VLAN),
>   whereas the drops (index 104, shared) show the total count (and without
>   the 4 bytes).
>

Beauty.  Assuming the stats are being synced to the kernel?
Test 1:
What does "tc -s actions ls action drop index 104" show?
Test 2:
Delete one of the filters above then dump actions again as above.

> (From your other email)
>> tcfa_index + action identifier seem to be sufficiently global, no?
> The reason I don't like using the action identifier is because flow_offload
>   slightly alters those: mirred gets split into two (FLOW_ACTION_REDIRECT
>   and FLOW_ACTION_MIRRED (mirror)).  Technically it'll still work (a redirect
>   and a mirror are different actions, so can't have the same index, so it
>   doesn't matter if they're treated as the same action-type or not) but it
>   feels like a kludge.
> 

As long as uapi semantics are not broken (which you are demonstrating it
is not) then we are good.

cheers,
jamal

> -Ed
>