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 09:58:40 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Baowen Zheng <baowen.zheng@...igine.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 2021-11-24 08:47, Baowen Zheng wrote:
> 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.

You may have to go deeper in the code for this to work. I think if it is
tricky to do consider doing it as a followup patch.

>>
>> 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?

I think it does. Let me see if i can explain it in my words:
For delete you dont have any attributes to pass but for create you need
to pass attributes which may be variable sized depending on the action.
And for that reason for create you need to allocate (but for delete
you can use a variable on the stack).
If yes, then at least make sure you are consistent on the stack values;
I think i saw some cases you initialize and in some you didnt.

cheers,
jamal