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 06:39:41 -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-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?

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.

cheers,
jamal