lkml.org 
[lkml]   [2023]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH bpf-next 1/3] bpf: Make struct task_struct an RCU-safe type
On Fri, Mar 31, 2023 at 10:35 AM David Vernet <void@manifault.com> 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.

\
 
 \ /
  Last update: 2023-03-31 19:50    [W:0.044 / U:1.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site