Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221002171012.3529521-1-void@manifault.com>
In-Reply-To: <20221002171012.3529521-1-void@manifault.com>
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date:   Mon, 3 Oct 2022 01:35:49 +0200
Message-ID: <CAP01T76SFT7TM02RaR9CMtBww34swXZotS2FkGKVBE6+o5XqBw@mail.gmail.com>
Subject: Re: [PATCH v2] selftests/bpf: Update map_kptr examples to reflect
 real use-cases
To:     David Vernet <void@manifault.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        martin.lau@linux.dev, kernel-team@fb.com, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, yhs@fb.com, song@kernel.org,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, tj@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, 2 Oct 2022 at 19:10, David Vernet <void@manifault.com> wrote:
>
> In the map_kptr selftest, the bpf_kfunc_call_test_kptr_get() kfunc is used
> to verify and illustrate a typical use case of kptrs wherein an additional
> reference is taken on a referenced kptr that is already stored in a map.
> This would be useful for programs that, for example, want to pass the
> referenced kptr to a kfunc without removing it from the map.
>
> Unfortunately, the implementation of bpf_kfunc_call_test_kptr_get() isn't
> representative of a real kfunc that needs to guard against possible
> refcounting races by BPF program callers. bpf_kfunc_call_test_kptr_get()
> does a READ_ONCE() on the struct prog_test_ref_kfunc **pp passed by the
> user and then calls refcount_inc() if the pointer is nonzero, but this
> can race with another callback in the same program that removes the kptr
> from the map  and frees it:
>
> 1. A BPF program with a referenced kptr in a map passes the kptr to
>    bpf_kfunc_call_test_kptr_get() as:
>
>    p = bpf_kfunc_call_test_kptr_get(&v->ref_ptr, 0, 0);
>
>    from CPU 0.
>
> 2. bpf_kfunc_call_test_kptr_get() does READ_ONCE(), and sees that the
>    struct prog_test_ref_kfunc **pp contains a non-NULL pointer.
>
> 3. Another BPF handler on CPU 1 then invokes bpf_kptr_xchg() to remove
>    the kptr from the map, and frees it with a call to
>    bpf_kfunc_call_test_release(). This drops the final refcount on the
>    kptr.
>
> 4. CPU 0 then issues refcount_inc() on the kptr with refcount 0, causing
>    a use-after-free.
>
> In the map_kptr selftest, this doesn't cause a use-after-free because
> the structure being refcounted is statically allocated, and the
> refcounts aren't actually used to control the object lifecycle. In a
> kfunc supporting a real use case, the refcount going to 0 would likely
> cause the object to be freed, as it does for e.g. struct task_struct.
>
> A more realistic use-case would use something like RCU in the kfunc
> handler to ensure that the kptr object can be safely accessed, and then
> issuing a refcount_inc_not_zero() to acquire a refcount on the object.
> This patch updates the map_kptr selftest to do this.
>
> Signed-off-by: David Vernet <void@manifault.com>
> ---

In my defense, I did note all this in the commit adding support for
kptr_get, so it seemed overkill to do it for a static struct.
But it's probably not a bad idea to have a real example, given it's a
necessity that such a helper requires reclamation of the object
through RCU, and people probably won't go and read the commit message.

However, some questions below...

>  net/bpf/test_run.c                            | 31 ++++++++++++++++---
>  .../selftests/bpf/prog_tests/map_kptr.c       |  4 +--
>  .../testing/selftests/bpf/verifier/map_kptr.c |  4 +--
>  3 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 13d578ce2a09..3fe9495abcbe 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -565,6 +565,8 @@ struct prog_test_ref_kfunc {
>         int b;
>         struct prog_test_member memb;
>         struct prog_test_ref_kfunc *next;
> +       struct rcu_head rcu;
> +       atomic_t destroyed;
>         refcount_t cnt;
>  };
>
> @@ -572,12 +574,14 @@ static struct prog_test_ref_kfunc prog_test_struct = {
>         .a = 42,
>         .b = 108,
>         .next = &prog_test_struct,
> +       .destroyed = ATOMIC_INIT(0),
>         .cnt = REFCOUNT_INIT(1),
>  };
>
>  noinline struct prog_test_ref_kfunc *
>  bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
>  {
> +       WARN_ON_ONCE(atomic_read(&prog_test_struct.destroyed));
>         refcount_inc(&prog_test_struct.cnt);
>         return &prog_test_struct;
>  }
> @@ -589,12 +593,22 @@ bpf_kfunc_call_memb_acquire(void)
>         return NULL;
>  }
>
> +static void delayed_destroy_test_ref_struct(struct rcu_head *rhp)
> +{
> +       struct prog_test_ref_kfunc *p = container_of(rhp, struct prog_test_ref_kfunc, rcu);
> +
> +       WARN_ON_ONCE(refcount_read(&p->cnt) > 0);
> +       atomic_set(&p->destroyed, true);
> +}
> +
>  noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
>  {
>         if (!p)
>                 return;
>
> -       refcount_dec(&p->cnt);
> +       WARN_ON_ONCE(atomic_read(&p->destroyed));
> +       if (refcount_dec_and_test(&p->cnt))
> +               call_rcu(&p->rcu, delayed_destroy_test_ref_struct);

I wonder whether this is ever called, I haven't really given this
patch a shot, but I don't see how the refcount can ever drop back to
zero. It's initialized as 1, and then only acquired after that, so
pairing all releases should still preserve refcount as 1.

Also, even if you made it work, wouldn't you have the warning once you
run more selftests using prog_test_run, if you just set the  destroyed
bit on each test run?