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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DM5PR1301MB217290C4B7759215BB5B184FE7629@DM5PR1301MB2172.namprd13.prod.outlook.com>
Date:   Thu, 25 Nov 2021 00:49:07 +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 10:59 PM, Jamal Hadi Salim wrote:
>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.
For the initialization, Vald mentioned this in another message, we will add the
Initialization in action delete, thanks.
>cheers,
>jamal