On Fri, Mar 31, 2023 at 10:05:04AM -0700, Alexei Starovoitov wrote: > On Thu, Mar 30, 2023 at 07:57:31PM -0500, David Vernet wrote: > > kernel/bpf/helpers.c | 11 ++- > > kernel/bpf/verifier.c | 1 + > > .../selftests/bpf/prog_tests/task_kfunc.c | 2 + > > .../selftests/bpf/progs/task_kfunc_common.h | 5 + > > .../selftests/bpf/progs/task_kfunc_failure.c | 98 +++++++++++++++++-- > > .../selftests/bpf/progs/task_kfunc_success.c | 52 +++++++++- > > 6 files changed, 153 insertions(+), 16 deletions(-) > > See CI failures on gcc compiled kernel: > https://github.com/kernel-patches/bpf/actions/runs/4570493668/jobs/8068004031 Thanks for the heads up, I'll take a look and resubmit v2 with fixes for gcc. In general it seems like a good idea to test both gcc and clang selftest builds; I'll do that from now on. > > __bpf_kfunc struct task_struct *bpf_task_acquire(struct task_struct *p) > > { > > - return get_task_struct(p); > > + if (refcount_inc_not_zero(&p->rcu_users)) > > + return p; > > + return NULL; > > } > > I wonder whether we should add a bit of safety net here. > Like do not allow acquire of tasks with PF_KTHREAD | PF_EXITING That's certainly an option, though I don't think it buys us much. It doesn't prevent the task from being pinned if it's acquired a bit earlier, and others in the kernel can acquire a task with get_task_struct() regardless of whether it's PF_EXITING (or an idle task, etc). IMO it's a better UX to provide a complementary API to get_task_struct(), but with RCU protection. On the other hand, it's already KF_RET_NULL, and I doubt needing to acquire a task that's PF_EXITING would be a common occurrence. We could always go the more restrictive route, and then loosen it if there's a valid use case? My only concern is that this safety net arguably doesn't really protect us from anything (given that you can just acquire the task before it's exiting), but maybe I'm wrong about that. > or at least is_idle_task ? Hmm, this one I'm really not sure about. On the one hand I can't think of a reason why anyone would need to acquire a reference to an idle task. On the other hand, it seems pretty benign to pin an idle task. I guess my sentiment is the same as above. I'm fine with adding a restriction and then loosening it later if there's a valid reason (and I can add a comment explaining this).
