On Fri, Mar 31, 2023 at 10:35 AM David Vernet <void@xxxxxxxxxxxxx> wrote: > > 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). Good point about pinning earlier. Let's keep it as-is then. bpf prog can do such checks on its own if it needs to.
