Re: [PATCH v2 bpf-next 03/13] bpf, x86: add new map type: instructions array

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/09/22 10:57AM, Alexei Starovoitov wrote:
> On Mon, Sep 22, 2025 at 10:31 AM Anton Protopopov
> <a.s.protopopov@gmail.com> wrote:
> >
> > On 25/09/22 09:16AM, Alexei Starovoitov wrote:
> > > On Mon, Sep 22, 2025 at 3:32 AM Anton Protopopov
> > > <a.s.protopopov@gmail.com> wrote:
> > > > > > +int bpf_insn_array_init(struct bpf_map *map, const struct bpf_prog *prog)
> > > > > > +{
> > > > > > +       struct bpf_insn_array *insn_array = cast_insn_array(map);
> > > > > > +       int i;
> > > > > > +
> > > > > > +       if (!valid_offsets(insn_array, prog))
> > > > > > +               return -EINVAL;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * There can be only one program using the map
> > > > > > +        */
> > > > > > +       mutex_lock(&insn_array->state_mutex);
> > > > > > +       if (insn_array->state != INSN_ARRAY_STATE_FREE) {
> > > > > > +               mutex_unlock(&insn_array->state_mutex);
> > > > > > +               return -EBUSY;
> > > > > > +       }
> > > > > > +       insn_array->state = INSN_ARRAY_STATE_INIT;
> > > > > > +       mutex_unlock(&insn_array->state_mutex);
> > > > >
> > > > > only verifier calls this helpers, no?
> > > > > Why all the mutexes here and below ?
> > > > > All the mutexes is a big red flag to me.
> > > > > Will stop any further comments here.
> > > >
> > > > Mutex came here from the future patch for static keys.
> > > > I will see how to rewrite this with just an atomic state.
> > >
> > > I don't follow. Who will be calling them other than the verifier?
> > > Some kfunc? I couldn't find that in the patch set.
> > > If so, add synchronization logic in the patch set that
> > > actually needs it. This one doesn't not. So don't add
> > > any mutex or atomics here.
> >
> > The usage of this map is as follows:
> >
> >   1. A user creates it and fills in the values using the map_update_element (syscall)
> >   2. Then the program is loaded
> >
> > The map <-> program is 1:1 relation, so I want to prevent users from
> >
> >   1. Updating the map after the program started loading
> >   2. Allowing two programs to use the same map (while, say, loading simultaneously)
> 
> Then the user space should freeze the map after updating and
> before loading.
> As far as 1-1 relation, we just landed exclusive map support
> that ties a map to one specific program.

AFAICS, this api is not applicable here, as it says "this map can
only be used with the program with sha256 hash X", but nothing
prevents users from loading, say, 2 same programs with the same map.

Are you ok with just this for 1:1 correspondance:

    if (atomic64_fetch_add_unless(&insn_array->used, 1, 1))
        return -EBUSY;

> This mechanism can be used or 1-1 can be established by the kernel
> internally.
> 
> > At the same time I want map to be reusable for the same program for the case
> > when the program failed to load and is reloaded with the log buffer.
> > So there should be some synchronisation mechanism.
> >
> > (In future patchset, the bpf(STATIC_KEY_UPDATE) syscall needs to execute. It
> > needs to be sure that the map was successfully loaded with the program. But
> > you're right that this doesn't make sense to leak part of this patch into this
> > patchset.)
> 
> Even when that bit will be available it won't be modifying the map.
> At best it will flip flag or bit whether the branch is nop or jmp.
> I still don't see a need for mutexes.