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

[Alexei Starovoitov]

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.