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] [day] [month] [year] [list]

Message-ID: <20170907175224.176b41e6@cakuba.netronome.com>
Date:   Thu, 7 Sep 2017 17:52:24 +0100
From:   Jakub Kicinski <kubakici@...pl>
To:     Jiri Pirko <jiri@...nulli.us>
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

On Thu, 7 Sep 2017 16:05:03 +0200, Jiri Pirko wrote:
> 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.

Right, the key/mask combination doesn't have to be unique anyway...

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

Yes, or you could take it to the extreme ;)
 DESTROY == offload(old, NULL)
 ADD     == offload(NULL, new)
 REPLACE == offload(obj, new)

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

Ack, if you're unifying things, I don't mind how things are muxed as
long as atomic replace is possible.

FWIW cls_bpf doesn't pass old prog in REPLACE right now, but I have
patch to add it anyway since it simplifies the driver when maps are
involved.  I should probably stop looking at the .command completely,
just rely on new/old programs being populated.