netdev - Re: [PATCH v10 net-next 15/15] p4tc: add P4 classifier

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: <a567ac93-2564-2235-b65f-d0940da076a5@iogearbox.net>
Date: Thu, 25 Jan 2024 16:47:20 +0100
From: Daniel Borkmann <daniel@...earbox.net>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: netdev@...r.kernel.org, deb.chatterjee@...el.com,
 anjali.singhai@...el.com, namrata.limaye@...el.com, tom@...anda.io,
 mleitner@...hat.com, Mahesh.Shirshyad@....com, tomasz.osinski@...el.com,
 jiri@...nulli.us, xiyou.wangcong@...il.com, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, vladbu@...dia.com,
 horms@...nel.org, khalidm@...dia.com, toke@...hat.com, mattyk@...dia.com,
 bpf@...r.kernel.org
Subject: Re: [PATCH v10 net-next 15/15] p4tc: add P4 classifier

On 1/24/24 3:40 PM, Jamal Hadi Salim wrote:
> On Wed, Jan 24, 2024 at 8:59 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>> On 1/22/24 8:48 PM, Jamal Hadi Salim wrote:
[...]
>>>
>>> It should also be noted that it is feasible to split some of the ingress
>>> datapath into XDP first and more into TC later (as was shown above for
>>> example where the parser runs at XDP level). YMMV.
>>> Regardless of choice of which scheme to use, none of these will affect
>>> UAPI. It will all depend on whether you generate code to load on XDP vs
>>> tc, etc.
>>>
>>> Co-developed-by: Victor Nogueira <victor@...atatu.com>
>>> Signed-off-by: Victor Nogueira <victor@...atatu.com>
>>> Co-developed-by: Pedro Tammela <pctammela@...atatu.com>
>>> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
>>> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
>>
>> My objections from last iterations still stand, and I also added a nak,
>> so please do not just drop it with new revisions.. from the v10 as you
>> wrote you added further code but despite the various community feedback
>> the design still stands as before, therefore:
>>
>> Nacked-by: Daniel Borkmann <daniel@...earbox.net>
> 
> We didnt make code changes - but did you read the cover letter and the
> extended commentary in this patch's commit log? We should have
> mentioned it in the changes log. It did respond to your comments.
> There's text that says "the filter manages the lifetime of the
> pipeline" - which in the future could include not only tc but XDP but
> also the hardware path (in the form of a file that gets loaded). I am
> not sure if that message is clear. Your angle being this is layer
> violation. In the last discussion i asked you for suggestions and we
> went the tcx route, which didnt make sense, and  then you didnt
> respond.
[...]

>> Also as mentioned earlier I don't think tc should hold references on
>> XDP programs in here. It doesn't make any sense aside from the fact
>> that the cls_p4 is also not doing anything with it. This is something
>> that a user space control plane should be doing i.e. managing a XDP
>> link on the target device.
> 
> This is the same argument about layer violation that you made earlier.
> The filter manages the p4 pipeline - i.e it's not just about the ebpf
> blob(s) but for example in the future (discussions are still ongoing
> with vendors who have P4 NICs) a filter could be loaded to also
> specify the location of the hardware blob.

Ah, so there is a plan to eventually add HW offload support for cls_p4?
Or is this only specifiying a location of a blob through some opaque
cookie value from user space?

> I would be happy with a suggestion that gets us moving forward with
> that context in mind.

My question on the above is mainly what does it bring you to hold a
reference on the XDP program? There is no guarantee that something else
will get loaded onto XDP, and then eventually the cls_p4 is the only
entity holding the reference but w/o 'purpose'. We do have BPF links
and the user space component orchestrating all this needs to create
and pin the BPF link in BPF fs, for example. An artificial reference
on XDP prog feels similar as if you'd hold a reference on an inode
out of tc.. Again, that should be delegated to the control plane you
have running interacting with the compiler which then manages and
loads its artifacts. What if you would also need to set up some
netfilter rules for the SW pipeline, would you then embed this too?

Thanks,
Daniel