Re: [PATCH bpf-next 1/3] bpf: Make struct task_struct an RCU-safe type

[David Vernet]

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).