On Fri, 16 Feb 2024 at 13:02, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Thu, 2024-02-01 at 04:21 +0000, Kumar Kartikeya Dwivedi wrote:
>
> [...]
>
> > +int bpf_cleanup_resource_reg(struct bpf_frame_desc_reg_entry *fd, void *ptr)
> > +{
>
> Question:
> Maybe I missed something in frame descriptor construction process,
> but it appears like there is nothing guarding against double cleanup.
> E.g. consider a program like below:
>
> r6 = ... PTR_TO_SOCKET ...
> r7 = r6
> *(u64 *)(r10 - 16) = r6
> call bpf_throw()
>
> Would bpf_cleanup_resource_reg() be called for all r6, r7 and fp[-16],
> thus executing destructor for the same object multiple times?
Good observation. My idea was to rely on release_reference so that
duplicate resources get erased from verifier state in such a way that
we don't go over the same ref_obj_id twice. IIUC, we start from the
current frame, and since bpf_for_each_reg_in_vstate iterates over all
frames, every register/stack slot sharing the ref_obj_id is destroyed,
so we wouldn't encounter the same resource again, hence the frame
descriptor should at most have one entry per resource. We iterate over
the stack frame first since the location of registers holding
resources is relatively stable and increases chances of merging across
different paths.
>
> > + u64 reg_value = ptr ? *(u64 *)ptr : 0;
> > + struct btf_struct_meta *meta;
> > + const struct btf_type *t;
> > + u32 dtor_id;
> > +
> > + switch (fd->type) {
> > + case PTR_TO_SOCKET:
> > + case PTR_TO_TCP_SOCK:
> > + case PTR_TO_SOCK_COMMON:
> > + if (reg_value)
> > + bpf_sk_release_dtor((void *)reg_value);
> > + return 0;
>
> [...]
