On Thu, Sep 12, 2024 at 01:48:35PM +0100, Joey Gouly wrote:
> On Thu, Sep 12, 2024 at 11:50:18AM +0100, Will Deacon wrote:
> > On Wed, Sep 11, 2024 at 08:33:54AM -0700, Dave Hansen wrote:
> > > On 9/11/24 08:01, Kevin Brodsky wrote:
> > > > On 22/08/2024 17:10, Joey Gouly wrote:
> > > >> @@ -371,6 +382,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> > > >> if (system_supports_tpidr2())
> > > >> p->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
> > > >>
> > > >> + if (system_supports_poe())
> > > >> + p->thread.por_el0 = read_sysreg_s(SYS_POR_EL0);
> > > > Here we are only reloading POR_EL0's value if the target is a user
> > > > thread. However, as this series stands, POR_EL0 is also relevant to
> > > > kthreads, because any uaccess or GUP done from a kthread will also be
> > > > checked against POR_EL0. This is especially important in cases like the
> > > > io_uring kthread, which accesses the memory of the user process that
> > > > spawned it. To prevent such a kthread from inheriting a stale value of
> > > > POR_EL0, it seems that we should reload POR_EL0's value in all cases
> > > > (user and kernel thread).
> > >
> > > The problem with this is trying to figure out which POR_EL0 to use. The
> > > kthread could have been spawned ages ago and might not have a POR_EL0
> > > which is very different from the current value of any of the threads in
> > > the process right now.
> > >
> > > There's also no great way for a kthread to reach out and grab an updated
> > > value. It's all completely inherently racy.
> > >
> > > > Other approaches could also be considered (e.g. resetting POR_EL0 to
> > > > unrestricted when creating a kthread), see my reply on v4 [1].
> > >
> > > I kinda think this is the only way to go. It's the only sensible,
> > > predictable way. I _think_ it's what x86 will end up doing with PKRU,
> > > but there's been enough churn there that I'd need to go double check
> > > what happens in practice.
> >
> > I agree.
> >
> > > Either way, it would be nice to get an io_uring test in here that
> > > actually spawns kthreads:
> > >
> > > tools/testing/selftests/mm/protection_keys.c
> >
> > It would be good to update Documentation/core-api/protection-keys.rst
> > as well, since the example with read() raises more questions than it
> > answers!
> >
> > Kevin, Joey -- I've got this series queued in arm64 as-is, so perhaps
> > you could send some patches on top so we can iron this out in time for
> > 6.12? I'll also be at LPC next week if you're about.
>
> I found the code in arch/x86 that does this, I must have missed this previously.
>
> arch/x86/kernel/process.c: int copy_thread()
>
> /* Kernel thread ? */
> if (unlikely(p->flags & PF_KTHREAD)) {
> p->thread.pkru = pkru_get_init_value();
> memset(childregs, 0, sizeof(struct pt_regs));
> kthread_frame_init(frame, args->fn, args->fn_arg);
> return 0;
> }
>
> I can send a similar patch for arm64. I have no idea how to write io_uring
> code, so looking for examples I can work with to get a test written. Might just
> send the arm64 fix first, if that's fine?
I think fix + documentation is what we need before 6.12, but you've still
got plenty of time after the merge window.
Cheers,
Will
