netdev - Re: [RFC PATCH bpf-next 10/15] bpf: Use bpf_map_pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZR41_JcQMvBfqB_7rcRZW97cJ_0WfWh7uh4Tt==A6zXw@mail.gmail.com>
Date:   Mon, 1 Aug 2022 16:16:48 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yafang Shao <laoar.shao@...il.com>
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        kafai@...com, songliubraving@...com, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, hannes@...xchg.org,
        mhocko@...nel.org, roman.gushchin@...ux.dev, shakeelb@...gle.com,
        songmuchun@...edance.com, akpm@...ux-foundation.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [RFC PATCH bpf-next 10/15] bpf: Use bpf_map_pages_alloc in ringbuf

On Fri, Jul 29, 2022 at 8:23 AM Yafang Shao <laoar.shao@...il.com> wrote:
>
> Introduce new helper bpf_map_pages_alloc() for this memory allocation.
>
> Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> ---
>  include/linux/bpf.h  |  4 ++++
>  kernel/bpf/ringbuf.c | 27 +++++++++------------------
>  kernel/bpf/syscall.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 18 deletions(-)
>

[...]

>         /* Each data page is mapped twice to allow "virtual"
>          * continuous read of samples wrapping around the end of ring
> @@ -95,16 +95,10 @@ static struct bpf_ringbuf *bpf_ringbuf_area_alloc(struct bpf_map *map,
>         if (!pages)
>                 return NULL;
>
> -       for (i = 0; i < nr_pages; i++) {
> -               page = alloc_pages_node(numa_node, flags, 0);
> -               if (!page) {
> -                       nr_pages = i;
> -                       goto err_free_pages;
> -               }
> -               pages[i] = page;
> -               if (i >= nr_meta_pages)
> -                       pages[nr_data_pages + i] = page;
> -       }
> +       ptr = bpf_map_pages_alloc(map, pages, nr_meta_pages, nr_data_pages,
> +                                 numa_node, flags, 0);
> +       if (!ptr)

bpf_map_pages_alloc() has some weird and confusing interface. It fills
out pages (second argument) and also returns pages as void *. Why not
just return int error (0 or -ENOMEM)? You are discarding this ptr
anyways.


But also thinking some more, bpf_map_pages_alloc() is very ringbuf
specific (which other map will have exactly the same meaning for
nr_meta_pages and nr_data_pages, where we also allocate 2 *
nr_data_pages, etc).

I don't think it makes sense to expose it as a generic internal API.
Why not keep all that inside kernel/bpf/ringbuf.c instead?

> +               goto err_free_pages;
>
>         rb = vmap(pages, nr_meta_pages + 2 * nr_data_pages,
>                   VM_MAP | VM_USERMAP, PAGE_KERNEL);

[...]