netdev - RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device

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]

Date:   Wed, 24 Nov 2021 13:47:47 +0000
From:   Baowen Zheng <baowen.zheng@...igine.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Cong Wang <xiyou.wangcong@...il.com>,
        Ido Schimmel <idosch@...dia.com>,
        Jiri Pirko <jiri@...nulli.us>, Oz Shlomo <ozsh@...dia.com>,
        Roi Dayan <roid@...dia.com>, Vlad Buslov <vladbu@...dia.com>,
        Louis Peens <louis.peens@...igine.com>,
        oss-drivers <oss-drivers@...igine.com>
Subject: RE: [PATCH v4 04/10] flow_offload: allow user to offload tc action to
 net device

On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote:
>On 2021-11-23 21:59, Baowen Zheng wrote:
>> Sorry for reply this message again.
>> On November 24, 2021 10:11 AM, Baowen Zheng wrote:
>>> On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:
>
>[..]
>
>>>>
>>>> BTW: shouldnt extack be used here instead of returning just -EINVAL?
>>>> I didnt stare long enough but it seems extack is not passed when
>>>> deleting from hardware? I saw a NULL being passed in one of the patches.
>> Maybe I misunderstand what you mean previously, when I look through
>> the implement in flow_action_init, I did not found we use the extack to
>make a log before return -EINVAL.
>> So could you please figure it out? Maybe I miss something or misunderstand
>again.
>
>I mean there are maybe 1-2 places where you called that function
>flow_action_init() with extack being NULL but the others with legitimate extack.
>I pointed to offload delete as an example. This may have existed before your
>changes (but it is hard to tell from just eyeballing patches); regardless it is a
>problem for debugging incase some delete offload fails, no?
Yes, you are right, for the most of the delete scenario, the extack is NULL since
The original implement to delete the action does not include an extack, so we will
Use extack when it is available.
>
>BTW:
>now that i am looking at the patches again - small details:
>struct flow_offload_action is sometimes initialized and sometimes not (and
>sometimes allocated and sometimes off the stack). Maybe to be consistent
>pick one style and stick with it.
For this implement, it is because for the action offload process, we need items of 
flow_action_entry in the flow_offload_action, then the size of flow_offload_action is
dependent on the action to be offloaded. But for delete case, we just need a pure
flow_offload_action, so we take it in stack.  You can refer to the implement in cls_flower,
it is similar with our case.
Do you think if it make sense to us?
>cheers,
>jamal