lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Apr 2022 12:39:41 -0700 From: sdf@...gle.com To: Andrii Nakryiko <andrii.nakryiko@...il.com> Cc: Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <kafai@...com> Subject: Re: [PATCH bpf-next] bpf: move rcu lock management out of BPF_PROG_RUN routines On 04/13, Andrii Nakryiko wrote: > On Wed, Apr 13, 2022 at 11:33 AM Stanislav Fomichev <sdf@...gle.com> > 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)? Yeah, that should work. But why do you think it's better to hide them? I find those automatic rcu locks deep in the call stack a bit obscure (when reasoning about sleepable vs non-sleepable contexts/bpf). I, as the caller, know that the effective array is rcu-managed (it has __rcu annotation) and it seems natural for me to grab rcu lock while work with it; I might grab it for some other things like cgroup anyway.
Powered by blists - more mailing lists