[v4,bpf-next,1/2] bpf: Allow ringbuf memory to be used as map key

Commit Message

Dave Marchevsky Sept. 23, 2022, 6:06 a.m. UTC

This patch adds support for the following pattern:

  struct some_data *data = bpf_ringbuf_reserve(&ringbuf, sizeof(struct some_data, 0));
  if (!data)
    return;
  bpf_map_lookup_elem(&another_map, &data->some_field);
  bpf_ringbuf_submit(data);

Currently the verifier does not consider bpf_ringbuf_reserve's
PTR_TO_MEM | MEM_ALLOC ret type a valid key input to bpf_map_lookup_elem.
Since PTR_TO_MEM is by definition a valid region of memory, it is safe
to use it as a key for lookups.

Signed-off-by: Dave Marchevsky <[e-mail]>
Acked-by: Yonghong Song <[e-mail]>
---
v2->v3: lore.kernel.org/bpf/[e-mail]

  * Add Yonghong ack, rebase

v1->v2: lore.kernel.org/bpf/[e-mail]

  * Move test changes into separate patch - patch 2 in this series.
    (Kumar, Yonghong). That patch's changelog enumerates specific
    changes from v1
  * Remove PTR_TO_MEM addition from this patch - patch 1 (Yonghong)
    * I don't have a usecase for PTR_TO_MEM w/o MEM_ALLOC
  * Add "if (!data)" error check to example pattern in this patch
    (Yonghong)
  * Remove patch 2 from v1's series, which removed map_key_value_types
    as it was more-or-less duplicate of mem_types
    * Now that PTR_TO_MEM isn't added here, more differences between
      map_key_value_types and mem_types, and no usecase for PTR_TO_BUF,
      so drop for now.

 kernel/bpf/verifier.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Andrii Nakryiko Sept. 23, 2022, 10:14 p.m. UTC | #1

On Thu, Sep 22, 2022 at 11:06 PM Dave Marchevsky <[e-mail]> wrote:
>
> This patch adds support for the following pattern:
>
>   struct some_data *data = bpf_ringbuf_reserve(&ringbuf, sizeof(struct some_data, 0));
>   if (!data)
>     return;
>   bpf_map_lookup_elem(&another_map, &data->some_field);
>   bpf_ringbuf_submit(data);
>
> Currently the verifier does not consider bpf_ringbuf_reserve's
> PTR_TO_MEM | MEM_ALLOC ret type a valid key input to bpf_map_lookup_elem.
> Since PTR_TO_MEM is by definition a valid region of memory, it is safe
> to use it as a key for lookups.
>
> Signed-off-by: Dave Marchevsky <[e-mail]>
> Acked-by: Yonghong Song <[e-mail]>
> ---
> v2->v3: lore.kernel.org/bpf/[e-mail]
>
>   * Add Yonghong ack, rebase
>
> v1->v2: lore.kernel.org/bpf/[e-mail]
>
>   * Move test changes into separate patch - patch 2 in this series.
>     (Kumar, Yonghong). That patch's changelog enumerates specific
>     changes from v1
>   * Remove PTR_TO_MEM addition from this patch - patch 1 (Yonghong)
>     * I don't have a usecase for PTR_TO_MEM w/o MEM_ALLOC
>   * Add "if (!data)" error check to example pattern in this patch
>     (Yonghong)
>   * Remove patch 2 from v1's series, which removed map_key_value_types
>     as it was more-or-less duplicate of mem_types
>     * Now that PTR_TO_MEM isn't added here, more differences between
>       map_key_value_types and mem_types, and no usecase for PTR_TO_BUF,
>       so drop for now.
>
>  kernel/bpf/verifier.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6f6d2d511c06..97351ae3e7a7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5641,6 +5641,7 @@ static const struct bpf_reg_types map_key_value_types = {
>                 PTR_TO_PACKET_META,
>                 PTR_TO_MAP_KEY,
>                 PTR_TO_MAP_VALUE,
> +               PTR_TO_MEM | MEM_ALLOC,

are there any differences between mem_types and map_key_value_types?
If not, should we just drop map_key_value_types? mem_types also alloc
any PTR_TO_MEM (not just ringbuf's MEM_ALLOC) and PTR_TO_BUF
(tracepoint context structs, I think?)

>         },
>  };
>
> --
> 2.30.2
>

Kumar Kartikeya Dwivedi Sept. 23, 2022, 10:38 p.m. UTC | #2

On Sat, 24 Sept 2022 at 00:14, Andrii Nakryiko
<[e-mail]> wrote:
>
> On Thu, Sep 22, 2022 at 11:06 PM Dave Marchevsky <[e-mail]> wrote:
> >
> > This patch adds support for the following pattern:
> >
> >   struct some_data *data = bpf_ringbuf_reserve(&ringbuf, sizeof(struct some_data, 0));
> >   if (!data)
> >     return;
> >   bpf_map_lookup_elem(&another_map, &data->some_field);
> >   bpf_ringbuf_submit(data);
> >
> > Currently the verifier does not consider bpf_ringbuf_reserve's
> > PTR_TO_MEM | MEM_ALLOC ret type a valid key input to bpf_map_lookup_elem.
> > Since PTR_TO_MEM is by definition a valid region of memory, it is safe
> > to use it as a key for lookups.
> >
> > Signed-off-by: Dave Marchevsky <[e-mail]>
> > Acked-by: Yonghong Song <[e-mail]>
> > ---
> > v2->v3: lore.kernel.org/bpf/[e-mail]
> >
> >   * Add Yonghong ack, rebase
> >
> > v1->v2: lore.kernel.org/bpf/[e-mail]
> >
> >   * Move test changes into separate patch - patch 2 in this series.
> >     (Kumar, Yonghong). That patch's changelog enumerates specific
> >     changes from v1
> >   * Remove PTR_TO_MEM addition from this patch - patch 1 (Yonghong)
> >     * I don't have a usecase for PTR_TO_MEM w/o MEM_ALLOC
> >   * Add "if (!data)" error check to example pattern in this patch
> >     (Yonghong)
> >   * Remove patch 2 from v1's series, which removed map_key_value_types
> >     as it was more-or-less duplicate of mem_types
> >     * Now that PTR_TO_MEM isn't added here, more differences between
> >       map_key_value_types and mem_types, and no usecase for PTR_TO_BUF,
> >       so drop for now.
> >
> >  kernel/bpf/verifier.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 6f6d2d511c06..97351ae3e7a7 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5641,6 +5641,7 @@ static const struct bpf_reg_types map_key_value_types = {
> >                 PTR_TO_PACKET_META,
> >                 PTR_TO_MAP_KEY,
> >                 PTR_TO_MAP_VALUE,
> > +               PTR_TO_MEM | MEM_ALLOC,
>
> are there any differences between mem_types and map_key_value_types?
> If not, should we just drop map_key_value_types? mem_types also alloc
> any PTR_TO_MEM (not just ringbuf's MEM_ALLOC) and PTR_TO_BUF
> (tracepoint context structs, I think?)
>

This was discussed here:
https://lore.kernel.org/bpf/[e-mail]

I guess we can do it, since it may already be triggered using PTR_TO_MAP_VALUE.

Based on my reading that day, it looked as if passing with offset != 0
would fail in all other cases, but I might be missing some other
corner case. I later realised that memcpy does fallback to memmove
when it detects overlap, so it wouldn't lead to any corruption, just a
warning at runtime.

> >         },
> >  };
> >
> > --
> > 2.30.2
> >

Andrii Nakryiko Sept. 23, 2022, 11:13 p.m. UTC | #3

On Fri, Sep 23, 2022 at 3:39 PM Kumar Kartikeya Dwivedi
<[e-mail]> wrote:
>
> On Sat, 24 Sept 2022 at 00:14, Andrii Nakryiko
> <[e-mail]> wrote:
> >
> > On Thu, Sep 22, 2022 at 11:06 PM Dave Marchevsky <[e-mail]> wrote:
> > >
> > > This patch adds support for the following pattern:
> > >
> > >   struct some_data *data = bpf_ringbuf_reserve(&ringbuf, sizeof(struct some_data, 0));
> > >   if (!data)
> > >     return;
> > >   bpf_map_lookup_elem(&another_map, &data->some_field);
> > >   bpf_ringbuf_submit(data);
> > >
> > > Currently the verifier does not consider bpf_ringbuf_reserve's
> > > PTR_TO_MEM | MEM_ALLOC ret type a valid key input to bpf_map_lookup_elem.
> > > Since PTR_TO_MEM is by definition a valid region of memory, it is safe
> > > to use it as a key for lookups.
> > >
> > > Signed-off-by: Dave Marchevsky <[e-mail]>
> > > Acked-by: Yonghong Song <[e-mail]>
> > > ---
> > > v2->v3: lore.kernel.org/bpf/[e-mail]
> > >
> > >   * Add Yonghong ack, rebase
> > >
> > > v1->v2: lore.kernel.org/bpf/[e-mail]
> > >
> > >   * Move test changes into separate patch - patch 2 in this series.
> > >     (Kumar, Yonghong). That patch's changelog enumerates specific
> > >     changes from v1
> > >   * Remove PTR_TO_MEM addition from this patch - patch 1 (Yonghong)
> > >     * I don't have a usecase for PTR_TO_MEM w/o MEM_ALLOC
> > >   * Add "if (!data)" error check to example pattern in this patch
> > >     (Yonghong)
> > >   * Remove patch 2 from v1's series, which removed map_key_value_types
> > >     as it was more-or-less duplicate of mem_types
> > >     * Now that PTR_TO_MEM isn't added here, more differences between
> > >       map_key_value_types and mem_types, and no usecase for PTR_TO_BUF,
> > >       so drop for now.
> > >
> > >  kernel/bpf/verifier.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 6f6d2d511c06..97351ae3e7a7 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -5641,6 +5641,7 @@ static const struct bpf_reg_types map_key_value_types = {
> > >                 PTR_TO_PACKET_META,
> > >                 PTR_TO_MAP_KEY,
> > >                 PTR_TO_MAP_VALUE,
> > > +               PTR_TO_MEM | MEM_ALLOC,
> >
> > are there any differences between mem_types and map_key_value_types?
> > If not, should we just drop map_key_value_types? mem_types also alloc
> > any PTR_TO_MEM (not just ringbuf's MEM_ALLOC) and PTR_TO_BUF
> > (tracepoint context structs, I think?)
> >
>
> This was discussed here:
> https://lore.kernel.org/bpf/[e-mail]

My bad, I skipped previous revisions and didn't see this suggestion.

>
> I guess we can do it, since it may already be triggered using PTR_TO_MAP_VALUE.
>
> Based on my reading that day, it looked as if passing with offset != 0
> would fail in all other cases, but I might be missing some other
> corner case. I later realised that memcpy does fallback to memmove
> when it detects overlap, so it wouldn't lead to any corruption, just a
> warning at runtime.
>
> > >         },
> > >  };
> > >
> > > --
> > > 2.30.2
> > >

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6f6d2d511c06..97351ae3e7a7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5641,6 +5641,7 @@  static const struct bpf_reg_types map_key_value_types = {
 		PTR_TO_PACKET_META,
 		PTR_TO_MAP_KEY,
 		PTR_TO_MAP_VALUE,
+		PTR_TO_MEM | MEM_ALLOC,
 	},
 };