netdev - Re: nfp bpf offload add/replace

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]

Message-ID: <20170907140503.GE1967@nanopsycho>
Date:   Thu, 7 Sep 2017 16:05:03 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kubakici@...pl>
Cc:     netdev@...r.kernel.org, mlxsw@...lanox.com,
        Daniel Borkmann <daniel@...earbox.net>,
        Simon Horman <simon.horman@...ronome.com>
Subject: Re: nfp bpf offload add/replace

Thu, Sep 07, 2017 at 03:44:12PM CEST, kubakici@...pl wrote:
>On Thu, 7 Sep 2017 11:10:33 +0200, Jiri Pirko wrote:
>> Hi Kuba.
>> 
>> I'm looking into cls_bpf code and nfp_net_bpf_offload function in your
>> driver. Why do you need TC_CLSBPF_ADD? Seems like TC_CLSBPF_REPLACE
>> should be enough. It would make the cls_bpf code easier.
>>
>> Note that other cls just have replace/destroy (u32 too, as drivers
>> handle NEW/REPLACE in one switch-case - will patch this).
>
>Could we clarify what the REPLACE is actually supposed to do?  :)
>
>In the flower code and the REPLACE looks a lot like ADD on the
>surface...  If change is called it will invoke REPLACE with the new
>filter and then if there was an old filter, it will do DELETE.  Is my
>understanding correct?

Yes, correct.


>
>If so I found this model of operation somehow confusing.  Plus the
>management of flows may get slightly tricky if there is a possibility of
>"replacing" a flow with an identical one.  Flower may make calls like
>these:
>
>add flower vlan_id 100 action ...
># REPLACE vid 100 ...
>change ... flower vlan_id 100 action ...
># REPLACE vid 100 ...
># DELETE  vid 100 ...

Yes, that is the flow.


>
>Doesn't this force driver/HW to implement refcounting on the rules?

Why do you think so? There is a cookie that is passed from flower down
and driver uses it to remove the entry.


>
>On why I need the replace - BPF unlike other classifiers usually
>installs a single program, I think offloading multiple TC filters is
>questionable (people will use tailcalls instead most likely).  I want to
>be able to implement atomic replace of that single program (i.e. not ADD
>followed by DELETE) because that simplifies the driver quite a bit.

Understood. So, looks like the REPLACE/DESTROY would be sufficient for
bpf. ADD is not needed as it can be done by REPLACE-NULL, right?

On the other hand, the rest of the cls, namely flower, u32 and matchall
need ADD/DESTROY as they don't really do no replacing.

Makes sense?