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 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: <f27a6a44-5016-1d17-580c-08682d29a767@solarflare.com>
Date:   Mon, 20 May 2019 16:26:20 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Jamal Hadi Salim <jhs@...atatu.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 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:
>>> I'm now leaning towards the
>>>   approach of adding "unsigned long cookie" to struct flow_action_entry
>>>   and populating it with (unsigned long)act in tc_setup_flow_action().
>>
>> For concreteness, here's what that looks like: patch 1 is replaced with
>>   the following, the other two are unchanged.
>> Drivers now have an easier job, as they can just use the cookie directly
>>   as a hashtable key, rather than worrying about which action types share
>>   indices.
>
> Per my other email, this will break tc semantics. It doesnt look
> possible to specify an index from user space. Did i miss
> something?
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.
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).

(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.

-Ed