On Wed, Dec 13, 2023 at 11:31 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 12/14/2023 2:22 PM, John Fastabend wrote:
> > Hou Tao wrote:
> >> From: Hou Tao <houtao1@xxxxxxxxxx>
> >>
> >> There is no rcu-read-lock requirement for ops->map_fd_get_ptr() or
> >> ops->map_fd_put_ptr(), so doesn't use rcu-read-lock for these two
> >> callbacks.
> >>
> >> For bpf_fd_array_map_update_elem(), accessing array->ptrs doesn't need
> >> rcu-read-lock because array->ptrs must still be allocated. For
> >> bpf_fd_htab_map_update_elem(), htab_map_update_elem() only requires
> >> rcu-read-lock to be held to avoid the WARN_ON_ONCE(), so only use
> >> rcu_read_lock() during the invocation of htab_map_update_elem().
> >>
> >> Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> >> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> >> ---
> >> kernel/bpf/hashtab.c | 6 ++++++
> >> kernel/bpf/syscall.c | 4 ----
> >> 2 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> >> index 5b9146fa825f..ec3bdcc6a3cf 100644
> >> --- a/kernel/bpf/hashtab.c
> >> +++ b/kernel/bpf/hashtab.c
> >> @@ -2523,7 +2523,13 @@ int bpf_fd_htab_map_update_elem(struct bpf_map *map, struct file *map_file,
> >> if (IS_ERR(ptr))
> >> return PTR_ERR(ptr);
> >>
> >> + /* The htab bucket lock is always held during update operations in fd
> >> + * htab map, and the following rcu_read_lock() is only used to avoid
> >> + * the WARN_ON_ONCE in htab_map_update_elem().
> >> + */
> >> + rcu_read_lock();
> >> ret = htab_map_update_elem(map, key, &ptr, map_flags);
> >> + rcu_read_unlock();
> > Did we consider dropping the WARN_ON_ONCE in htab_map_update_elem()? It
> > looks like there are two ways to get to htab_map_update_elem() either
> > through a syscall and the path here (bpf_fd_htab_map_update_elem) or
> > through a BPF program calling, bpf_update_elem()? In the BPF_CALL
> > case bpf_map_update_elem() already has,
> >
> > WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_bh_held())
> >
> > The htab_map_update_elem() has an additional check for
> > rcu_read_lock_trace_held(), but not sure where this is coming from
> > at the moment. Can that be added to the BPF caller side if needed?
> >
> > Did I miss some caller path?
>
> No. But I think the main reason for the extra WARN in
> bpf_map_update_elem() is that bpf_map_update_elem() may be inlined by
> verifier in do_misc_fixups(), so the WARN_ON_ONCE in
> bpf_map_update_elem() will not be invoked ever. For
> rcu_read_lock_trace_held(), I have added the assertion in
> bpf_map_delete_elem() recently in commit 169410eba271 ("bpf: Check
> rcu_read_lock_trace_held() before calling bpf map helpers").
Yep.
We should probably remove WARN_ONs from
bpf_map_update_elem() and others in kernel/bpf/helpers.c
since they are inlined by the verifier with 99% probability
and the WARNs are never called even in DEBUG kernels.
And confusing developers. As this thread shows.
We can replace them with a comment that explains this inlining logic
and where the real WARNs are.
