On Mon, Feb 7, 2022 at 1:27 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Feb 07, 2022 at 12:04:14PM -0800, Andrii Nakryiko wrote:
> > > */
> > > struct bpf_map_desc {
> > > - union {
> > > - /* input for the loader prog */
> > > - struct {
> > > - __aligned_u64 initial_value;
> > > - __u32 max_entries;
> > > - };
> > > + struct {
> >
> > Is this anonymous struct still needed?
>
> Right. Will remove.
>
> > > +static inline void *skel_alloc(size_t size)
> > > +{
> > > + return kcalloc(1, size, GFP_KERNEL);
> > > +}
> > > +static inline void skel_free(const void *p)
> > > +{
> > > + kfree(p);
> > > +}
> >
> > any reason to skim on empty lines between functions? The rest of this
> > file (and libbpf code in general) feels very different in terms of
> > spacing.
>
> Because it's more compact, but I don't mind extra lines.
>
> > > +static inline void skel_free_map_data(void *p, __u64 addr, size_t sz)
> > > +{
> > > + if (addr && addr != ~0ULL)
> > > + vm_munmap(addr, sz);
> > > + if (addr != ~0ULL)
> > > + kvfree(p);
> >
> > minor nit: a small comment explaining that we set addr to ~0ULL on
> > error (but we still call skel_free_map_data) would help a bit here
>
> ok.
>
> > > +}
> > > +/* skel->bss/rodata maps are populated in three steps.
> > > + *
> > > + * For kernel use:
> > > + * skel_prep_map_data() allocates kernel memory that kernel module can directly access.
> > > + * skel_prep_init_value() allocates a region in user space process and copies
> > > + * potentially modified initial map value into it.
> > > + * The loader program will perform copy_from_user() from maps.rodata.initial_value.
> >
> > I'm missing something here. If a light skeleton is used from a kernel
> > module, then this initialization data is also pointing to kernel
> > module memory, no? So why the copy_from_user() then?
> >
> > Also this vm_mmap() and vm_munmap(), is it necessary for in-kernel
> > skeleton itself, or it's required so that if some user-space process
> > would fetch that BPF map by ID and tried to mmap() its content it
> > would be possible? Otherwise it's not clear, as kernel module can
> > access BPF array's value pointer directly anyways, so why the mmaping?
>
> vm_mmap step only to preserve one version of light skeleton that
> works for both user space and kernel. Otherwise bpftool would need another flag
> and test coverage would need to increase. This way light skeleton for kernel
> doesn't need a bunch of new tests.
> Another option would be to add 'is_kernel' flag to bpf_loader_ctx and generate
> loader program like:
> if (ctx->is_kernel)
> bpf_probe_read_kernel
> else
> bpf_copy_from_user
>
> but 'ctx' will be modified after signature check, so the user space user
> of light skel might trick it to populate maps with garbage kernel data.
> The loader prog needs cap_perfmon anyway, so there are no security concerns,
> but it's not great to have such room for error.
> vm_mmap approach is not pretty, but looks to be the lesser evil.
> I'm all ears if there are other options.
Ah, ok, I didn't get that. Please leave comments explaining this. I
have no preference regarding ctx->is_kernel vs doing in-kernel
vm_mmap(), but this needs to be spelled out, as it's very non-obvious.
