Re: [PATCH v2 bpf-next 12/17] bpf: Fix race in btf_resolve_helper_id()

On 11/7/19 9:13 PM, Andrii Nakryiko wrote:

On Wed, Nov 6, 2019 at 9:48 PM Alexei Starovoitov [off-list ref] wrote:

quoted

btf_resolve_helper_id() caching logic is racy, since under root the verifier
can verify several programs in parallel. Fix it with extra spin_lock.

Fixes: a7658e1a4164 ("bpf: Check types of arguments passed into helpers")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
  include/linux/bpf.h   |  3 ++-
  kernel/bpf/btf.c      | 34 +++++++++++++++++++++++++++++++++-
  kernel/bpf/verifier.c |  6 +-----
  3 files changed, 36 insertions(+), 7 deletions(-)

[...]

quoted

+       /* ok to race the search. The result is the same */
+       ret = __btf_resolve_helper_id(log, fn->func, arg);
+       if (!ret) {
+               bpf_log(log, "BTF resolution bug\n");
+               return -EFAULT;
+       }
+       spin_lock(&btf_resolve_lock);
+       if (*btf_id) {
+               ret = *btf_id;
+               goto out;
+       }
+       *btf_id = ret;
+out:
+       spin_unlock(&btf_resolve_lock);

Is this race a problem? Does it cause any issues? Given that even if
you do parallel resolutions at the same time, they all will have to
result in the same btf_id, so just setting it unconditionally multiple
times without locking should be ok, no? Maybe WRITE_ONCE, but not sure
why all the way to spinlock.

Hmm. Indeed. Let me switch to READ_ONCE/WRITE_ONCE.