Re: [PATCH bpf-next v7 21/26] bpf: Add 'release on unlock' logic for bpf_list_push_{front,back}

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Tue, Nov 15, 2022 at 10:22:56PM IST, Dave Marchevsky wrote:
> On 11/14/22 2:15 PM, Kumar Kartikeya Dwivedi wrote:
> > This commit implements the delayed release logic for bpf_list_push_front
> > and bpf_list_push_back.
> >
> > Once a node has been added to the list, it's pointer changes to
> > PTR_UNTRUSTED. However, it is only released once the lock protecting the
> > list is unlocked. For such PTR_TO_BTF_ID | MEM_ALLOC with PTR_UNTRUSTED
> > set but an active ref_obj_id, it is still permitted to read them as long
> > as the lock is held. Writing to them is not allowed.
> >
> > This allows having read access to push items we no longer own until we
> > release the lock guarding the list, allowing a little more flexibility
> > when working with these APIs.
> >
> > Note that enabling write support has fairly tricky interactions with
> > what happens inside the critical section. Just as an example, currently,
> > bpf_obj_drop is not permitted, but if it were, being able to write to
> > the PTR_UNTRUSTED pointer while the object gets released back to the
> > memory allocator would violate safety properties we wish to guarantee
> > (i.e. not crashing the kernel). The memory could be reused for a
> > different type in the BPF program or even in the kernel as it gets
> > eventually kfree'd.
> >
> > Not enabling bpf_obj_drop inside the critical section would appear to
> > prevent all of the above, but that is more of an artifical limitation
> > right now. Since the write support is tangled with how we handle
> > potential aliasing of nodes inside the critical section that may or may
> > not be part of the list anymore, it has been deferred to a future patch.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
>
> Can the two WARN_ON_ONCE in this patch be converted to
> verifier-log-and-EFAULT? Looks like they're both in
> functions with access to 'env' and are checking for
> scenarios that should be considered bugs in the verifier.
>

Will do.

> Aside from that style nit, logic and patch summary updates
> here LGTM.
>
> Acked-by: Dave Marchevsky <davemarchevsky@fb.com>