Re: [PATCH v4 bpf-next 1/5] bpf: Remove redundant synchronize_rcu.

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Jun 29, 2020 at 5:58 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jun 29, 2020 at 5:35 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > bpf_free_used_maps() or close(map_fd) will trigger map_free callback.
> > bpf_free_used_maps() is called after bpf prog is no longer executing:
> > bpf_prog_put->call_rcu->bpf_prog_free->bpf_free_used_maps.
> > Hence there is no need to call synchronize_rcu() to protect map elements.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
>
> Seems correct. And nice that maps don't have to care about this anymore.
>

Actually, what about the map-in-map case?

What if you had an array-of-maps with an inner map element. It is the
last reference to that map. Now you have two BPF prog executions in
parallel. One looked up that inner map and is updating it at the
moment. Another execution at the same time deletes that map. That
deletion will call bpf_map_put(), which without synchronize_rcu() will
free memory. All the while the former BPF program execution is still
working with that map.

Or am I missing something that would prevent that?

> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> >  kernel/bpf/arraymap.c         | 9 ---------
> >  kernel/bpf/hashtab.c          | 8 +++-----
> >  kernel/bpf/lpm_trie.c         | 5 -----
> >  kernel/bpf/queue_stack_maps.c | 7 -------
> >  kernel/bpf/reuseport_array.c  | 2 --
> >  kernel/bpf/ringbuf.c          | 7 -------
> >  kernel/bpf/stackmap.c         | 3 ---
> >  7 files changed, 3 insertions(+), 38 deletions(-)
> >