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: <20221014212133.2520531-1-void@manifault.com> <20221014212133.2520531-3-void@manifault.com>
In-Reply-To: <20221014212133.2520531-3-void@manifault.com>
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date:   Tue, 18 Oct 2022 07:25:46 +0530
Message-ID: <CAP01T77WVXqeZVBYP=Ur=74oTwvGaLOeryYo=HqeBQ9Db9A93g@mail.gmail.com>
Subject: Re: [PATCH v5 2/3] bpf: Add kfuncs for storing struct task_struct *
 as a kptr
To:     David Vernet <void@manifault.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@fb.com, tj@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, 15 Oct 2022 at 02:51, David Vernet <void@manifault.com> wrote:
>
> Now that BPF supports adding new kernel functions with kfuncs, and
> storing kernel objects in maps with kptrs, we can add a set of kfuncs
> which allow struct task_struct objects to be stored in maps as
> referenced kptrs. The possible use cases for doing this are plentiful.
> During tracing, for example, it would be useful to be able to collect
> some tasks that performed a certain operation, and then periodically
> summarize who they are, which cgroup they're in, how much CPU time
> they've utilized, etc.
>
> In order to enable this, this patch adds three new kfuncs:
>
> struct task_struct *bpf_task_acquire(struct task_struct *p);
> struct task_struct *bpf_task_kptr_get(struct task_struct **pp);
> void bpf_task_release(struct task_struct *p);
>
> A follow-on patch will add selftests validating these kfuncs.
>
> Signed-off-by: David Vernet <void@manifault.com>
> ---
>  [...]
> +/**
> + * bpf_task_release - Release the reference acquired on a struct task_struct *.
> + * If this kfunc is invoked in an RCU read region, the task_struct is
> + * guaranteed to not be freed until the current grace period has ended, even if
> + * its refcount drops to 0.
> + * @p: The task on which a reference is being released.
> + */
> +__used noinline void bpf_task_release(struct task_struct *p)
> +{
> +       if (!p)
> +               return;
> +
> +       put_task_struct_rcu_user(p);
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(generic_kfunc_btf_ids)
>  #ifdef CONFIG_KEXEC_CORE
>  BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
>  #endif
> -BTF_SET8_END(tracing_btf_ids)
> +BTF_ID_FLAGS(func, bpf_task_acquire, KF_ACQUIRE | KF_RET_NULL | KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_task_kptr_get, KF_ACQUIRE | KF_KPTR_GET | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_task_release, KF_RELEASE | KF_TRUSTED_ARGS)

You can remove KF_TRUSTED_ARGS here for bpf_task_release, if this is
required, it would be a bug.