On Wed, Apr 13, 2022 at 11:33 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> Commit 7d08c2c91171 ("bpf: Refactor BPF_PROG_RUN_ARRAY family of macros
> into functions") switched a bunch of BPF_PROG_RUN macros to inline
> routines. This changed the semantic a bit. Due to arguments expansion
> of macros, it used to be:
>
> rcu_read_lock();
> array = rcu_dereference(cgrp->bpf.effective[atype]);
> ...
>
> Now, with with inline routines, we have:
> array_rcu = rcu_dereference(cgrp->bpf.effective[atype]);
> /* array_rcu can be kfree'd here */
> rcu_read_lock();
> array = rcu_dereference(array_rcu);
>
So subtle difference, wow...
But this open-coding of rcu_read_lock() seems very unfortunate as
well. Would making BPF_PROG_RUN_ARRAY back to a macro which only does
rcu lock/unlock and grabs effective array and then calls static inline
function be a viable solution?
#define BPF_PROG_RUN_ARRAY_CG_FLAGS(array_rcu, ctx, run_prog, ret_flags) \
({
int ret;
rcu_read_lock();
ret = __BPF_PROG_RUN_ARRAY_CG_FLAGS(rcu_dereference(array_rcu), ....);
rcu_read_unlock();
ret;
})
where __BPF_PROG_RUN_ARRAY_CG_FLAGS is what
BPF_PROG_RUN_ARRAY_CG_FLAGS is today but with __rcu annotation dropped
(and no internal rcu stuff)?
> I'm assuming in practice rcu subsystem isn't fast enough to trigger
> this but let's use rcu API properly: ask callers of BPF_PROG_RUN
> to manage rcu themselves.
>
> Also, rename to lower caps to not confuse with macros. Additionally,
> drop and expand BPF_PROG_CGROUP_INET_EGRESS_RUN_ARRAY.
>
> See [1] for more context.
>
> [1] https://lore.kernel.org/bpf/CAKH8qBs60fOinFdxiiQikK_q0EcVxGvNTQoWvHLEUGbgcj1UYg@xxxxxxxxxxxxxx/T/#u
>
> Cc: Martin KaFai Lau <kafai@xxxxxx>
> Fixes: 7d08c2c91171 ("bpf: Refactor BPF_PROG_RUN_ARRAY family of macros into functions")
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
> drivers/media/rc/bpf-lirc.c | 8 +++-
> include/linux/bpf.h | 70 ++++++-------------------------
> kernel/bpf/cgroup.c | 84 +++++++++++++++++++++++++++++--------
> kernel/trace/bpf_trace.c | 5 ++-
> 4 files changed, 90 insertions(+), 77 deletions(-)
>
[...]
