Messages in this thread | | | Date | Fri, 12 Oct 2018 13:54:28 -0700 | From | Joel Fernandes <> | Subject | Re: [PATCH v4] Wait for running BPF programs when updating map-in-map |
| |
On Fri, Oct 12, 2018 at 03:54:27AM -0700, Daniel Colascione wrote: > The map-in-map frequently serves as a mechanism for atomic > snapshotting of state that a BPF program might record. The current > implementation is dangerous to use in this way, however, since > userspace has no way of knowing when all programs that might have > retrieved the "old" value of the map may have completed. > > This change ensures that map update operations on map-in-map map types > always wait for all references to the old map to drop before returning > to userspace. > > Signed-off-by: Daniel Colascione <dancol@google.com> > --- > kernel/bpf/syscall.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 8339d81cba1d..d7c16ae1e85a 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -741,6 +741,18 @@ static int map_lookup_elem(union bpf_attr *attr) > return err; > } > > +static void maybe_wait_bpf_programs(struct bpf_map *map) > +{ > + /* Wait for any running BPF programs to complete so that > + * userspace, when we return to it, knows that all programs > + * that could be running use the new map value. > + */ > + if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS || > + map->map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS) { > + synchronize_rcu(); > + } > +} > + > #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags > > static int map_update_elem(union bpf_attr *attr) > @@ -831,6 +843,7 @@ static int map_update_elem(union bpf_attr *attr) > } > __this_cpu_dec(bpf_prog_active); > preempt_enable(); > + maybe_wait_bpf_programs(map); > out: > free_value: > kfree(value); > @@ -883,6 +896,7 @@ static int map_delete_elem(union bpf_attr *attr) > rcu_read_unlock(); > __this_cpu_dec(bpf_prog_active); > preempt_enable(); > + maybe_wait_bpf_programs(map);
Looks good to me,
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Also I believe that those rcu_read_lock() and rcu_read_unlock() calls in the existing code are useless. preempt_disable()d code is already an RCU read-side section, and synchronize_rcu and friends work on those type of read-side sections as well (as of recent kernel releases) however removing it may make lockdep unhappy, unless we also replace all rcu_dereference() usages with rcu_dereference_sched(), so lets leave that alone for now I guess.
thanks,
- Joel
| |