netdev - Re: [ RFC net-next v2 2/2] net: flow

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] [day] [month] [year] [list]

Message-ID: <c3868497-a299-3796-fe79-439dfb0653d2@gmail.com>
Date:   Thu, 6 Oct 2022 10:53:08 +0100
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Oz Shlomo <ozsh@...dia.com>, netdev@...r.kernel.org
Cc:     Jiri Pirko <jiri@...dia.com>, Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...igine.com>,
        Baowen Zheng <baowen.zheng@...igine.com>,
        Vlad Buslov <vladbu@...dia.com>,
        Ido Schimmel <idosch@...dia.com>, Roi Dayan <roid@...dia.com>
Subject: Re: [ RFC net-next v2 2/2] net: flow_offload: add action stats api

On 04/10/2022 09:22, Oz Shlomo wrote:
>> @@ -588,6 +596,8 @@ struct flow_cls_offload {
>>       unsigned long cookie;
>>       struct flow_rule *rule;
>>       struct flow_stats stats;
>> +    struct flow_act_stat act_stats[FLOW_OFFLOAD_MAX_ACT_STATS];
> 
> Apparently this array can exceed the stack frame size for several archs (reported by the kernel test bot).

Seems to me like yet another sign this array shouldn't exist in the
 first place and you should be calling a driver offload function per
 action and not trying to squeeze all this through the existing
 flow-rule-centric API.
Why can't the action stats be stored in struct flow_action_entry,
 inside struct flow_rule?  Then, if you really want a single call
 for performance reasons, action-stats-aware drivers could just walk
 through flow_action_for_each(..., &flow_rule->action) and update
 each action's stats based on action->act_cookie.  That should still
 allow proper shared action handling, and seems far more elegant
 than this array.  (Any time you have two arrays and there's a close
 connection between a[i] and b[i], that's a good sign one's elements
 should be a struct member of the other's, rather than them being
 tied together only by index.)

-ed