netdev - Re: [RFC PATCH bpf-next RESEND 00/16] bpf: Checkpoint/Restore In eBPF (CRIB)

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: <etzm4h5qm2jhgi6d4pevooy2sebrvgb3lsa67ym4x7zbh5bgnj@feoli4hj22so>
Date: Mon, 22 Jul 2024 16:47:09 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Juntong Deng <juntong.deng@...look.com>
Cc: ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com, 
	martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev, 
	kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, 
	andrii@...nel.org, avagin@...il.com, snorcht@...il.com, bpf@...r.kernel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH bpf-next RESEND 00/16] bpf: Checkpoint/Restore In
 eBPF (CRIB)

On Thu, Jul 11, 2024 at 12:10:17PM +0100, Juntong Deng wrote:
> 
> In restore_udp_socket I had to add a struct bpf_crib_skb_info for
> restoring packets, this is because there is currently no BPF_CORE_WRITE.
> 
> I am not sure what the current attitude of the kernel community
> towards BPF_CORE_WRITE is, personally I think it is well worth adding,
> as we need a portable way to change the value in the kernel.
> 
> This not only allows more complexity in the CRIB restoring part to
> be transferred from CRIB kfuncs to CRIB ebpf programs, but also allows
> ebpf to unlock more possible application scenarios. 

There are lots of interesting ideas in this patch set, but it seems they are
doing the 'C-checkpoint' part of CRIx and something like BPF_CORE_WRITE
is necessary for 'R-restore'.
I'm afraid BPF_CORE_WRITE cannot be introduced without breaking all safety nets.
It will make bpf just as unsafe as any kernel module if bpf progs can start
writing into arbitrary kernel data structures. So it's a show stopper.
If you think there is a value in adding all these iterators for 'checkpoint'
part alone we can discuss and generalize individual patches.

High level feedback:

- no need for BPF_PROG_TYPE_CRIB program type. Existing syscall type should fit.

- proposed file/socket iterators are somewhat unnecessary in this open coded form.
  there is already file/socket iterator. From the selftests it looks like it
  can be used to do 'checkpoint' part already.

- KF_ITER_GETTER is a good addition, but we should be able to do it without these flags.
  kfunc-s should be able to accept iterator as an argument. Some __suffix annotation
  may be necessary to help verifier if BTF type alone of the argument won't be enough.

- KF_OBTAIN looks like a broken hammer to bypass safety. Like:

  > Currently we cannot pass the pointer returned by the iterator next
  > method as argument to the KF_TRUSTED_ARGS kfuncs, because the pointer
  > returned by the iterator next method is not "valid".

  It's true, but should be fixable directly. Make return pointer of iter_next() to be trusted.

- iterators for skb data don't feel right. bpf_dynptr_from_skb() should do the trick already.

- start with a small patch set.
  30 files changed, 3080 insertions(+), 12 deletions(-)
  isn't really reviewable.