On Thu, Feb 8, 2024 at 10:45 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Feb 8, 2024 at 10:29 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Feb 7, 2024 at 5:38 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Wed, Feb 7, 2024 at 5:15 PM Andrii Nakryiko
> > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > On Tue, Feb 6, 2024 at 2:05 PM Alexei Starovoitov
> > > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > > >
> > > > > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> > > > >
> > > > > mmap() bpf_arena right after creation, since the kernel needs to
> > > > > remember the address returned from mmap. This is user_vm_start.
> > > > > LLVM will generate bpf_arena_cast_user() instructions where
> > > > > necessary and JIT will add upper 32-bit of user_vm_start
> > > > > to such pointers.
> > > > >
> > > > > Use traditional map->value_size * map->max_entries to calculate mmap sz,
> > > > > though it's not the best fit.
> > > >
> > > > We should probably make bpf_map_mmap_sz() aware of specific map type
> > > > and do different calculations based on that. It makes sense to have
> > > > round_up(PAGE_SIZE) for BPF map arena, and use just just value_size or
> > > > max_entries to specify the size (fixing the other to be zero).
> > >
> > > I went with value_size == key_size == 8 in order to be able to extend
> > > it in the future and allow map_lookup/update/delete to do something
> > > useful. Ex: lookup/delete can behave just like arena_alloc/free_pages.
> > >
> > > Are you proposing to force key/value_size to zero ?
> >
> > Yeah, I was thinking either (value_size=<size-in-bytes> and
> > max_entries=0) or (value_size=0 and max_entries=<size-in-bytes>). The
> > latter is what we do for BPF ringbuf, for example.
>
> Ouch. since map_update_elem() does:
> value_size = bpf_map_value_size(map);
> value = kvmemdup_bpfptr(uvalue, value_size);
> ...
> static inline void *kvmemdup_bpfptr(bpfptr_t src, size_t len)
> {
> void *p = kvmalloc(len, GFP_USER | __GFP_NOWARN);
>
> if (!p)
> return ERR_PTR(-ENOMEM);
> if (copy_from_bpfptr(p, src, len)) {
> ...
> if (unlikely(!size))
> return ZERO_SIZE_PTR;
>
> and it's probably crashing the kernel.
You mean when doing this from SYSCALL program?
>
> Looks like we have fixes to do anyway :(
Yeah, it's kind of weird to first read key/value "memory", and then
getting -ENOTSUP for maps that don't support lookup/update. We should
error out sooner.
