From: Michael Ellerman <mpe@ellerman.id.au>
To: Timothy Pearson <tpearson@raptorengineering.com>
Cc: Jens Axboe <axboe@kernel.dk>,
regressions <regressions@lists.linux.dev>,
npiggin <npiggin@gmail.com>,
christophe leroy <christophe.leroy@csgroup.eu>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save
Date: Tue, 21 Nov 2023 10:39:52 +1100 [thread overview]
Message-ID: <874jhg6lkn.fsf@mail.lhotse> (raw)
In-Reply-To: <439072392.48800901.1700498743840.JavaMail.zimbra@raptorengineeringinc.com>
Timothy Pearson <tpearson@raptorengineering.com> writes:
> ----- Original Message -----
>> From: "Michael Ellerman" <mpe@ellerman.id.au>
...
>>
>> But we now have a new path, because io-uring can call copy_process() via
>> create_io_thread() from the signal handling path. That's OK if the signal is
>> handled as we return from a syscall, but it's not OK if the signal is handled
>> due to some other interrupt.
>>
>> Which is:
>>
>> interrupt_return_srr_user()
>> interrupt_exit_user_prepare()
>> interrupt_exit_user_prepare_main()
>> do_notify_resume()
>> get_signal()
>> task_work_run()
>> create_worker_cb()
>> create_io_worker()
>> copy_process()
>> dup_task_struct()
>> arch_dup_task_struct()
>> flush_all_to_thread()
>> save_all()
>> if (tsk->thread.regs->msr & MSR_FP)
>> save_fpu()
>> # fr0 is clobbered and potentially live in userspace
>>
>>
>> So tldr I think the corruption is only an issue since io-uring started doing
>> the clone via signal, which I think matches the observed timeline of this bug
>> appearing.
>
> I agree the corruption really only started showing up in earnest on
> io_uring clone-via-signal, as this was confirmed several times in the
> course of debugging.
Thanks.
> Note as well that I may very well have a wrong call order in the
> commit message, since I was relying on a couple of WARN_ON() macros I
> inserted to check for a similar (but not identical) condition and
> didn't spend much time getting new traces after identifying the root
> cause.
Yep no worries. I'll reword it to incorporate the full path from my mail.
> I went back and grabbed some real world system-wide stack traces, since I now know what to trigger on. A typical example is:
>
> interrupt_return_srr_user()
> interrupt_exit_user_prepare()
> interrupt_exit_user_prepare_main()
> schedule()
> __schedule()
> __switch_to()
> giveup_all()
> # tsk->thread.regs->msr MSR_FP is still set here
> __giveup_fpu()
> save_fpu()
> # fr0 is clobbered and potentially live in userspace
fr0 is not live there.
__giveup_fpu() does roughly:
msr = tsk->thread.regs->msr;
msr &= ~(MSR_FP|MSR_FE0|MSR_FE1);
msr &= ~MSR_VSX;
tsk->thread.regs = msr;
ie. it clears the FP etc. bits from the task's MSR. That means the FP
state will be reloaded from the thread struct before the task is run again.
Also on that path we're switching to another task, so we'll be reloading
the other task's FP state before returning to userspace.
So I don't see any bug there.
There's only two places that call save_fpu() and skip the giveup logic,
which is save_all() and kvmppc_save_user_regs().
save_all() is only called via clone() so I think that's unable to
actually cause visible register corruption as I described in my previous
mail.
I thought the KVM case was similar, as it's called via an ioctl, but
I'll have to talk to Nick as his mail indicates otherwise.
cheers
WARNING: multiple messages have this Message-ID (diff)
From: Michael Ellerman <mpe@ellerman.id.au>
To: Timothy Pearson <tpearson@raptorengineering.com>
Cc: Jens Axboe <axboe@kernel.dk>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
regressions <regressions@lists.linux.dev>,
npiggin <npiggin@gmail.com>
Subject: Re: [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save
Date: Tue, 21 Nov 2023 10:39:52 +1100 [thread overview]
Message-ID: <874jhg6lkn.fsf@mail.lhotse> (raw)
In-Reply-To: <439072392.48800901.1700498743840.JavaMail.zimbra@raptorengineeringinc.com>
Timothy Pearson <tpearson@raptorengineering.com> writes:
> ----- Original Message -----
>> From: "Michael Ellerman" <mpe@ellerman.id.au>
...
>>
>> But we now have a new path, because io-uring can call copy_process() via
>> create_io_thread() from the signal handling path. That's OK if the signal is
>> handled as we return from a syscall, but it's not OK if the signal is handled
>> due to some other interrupt.
>>
>> Which is:
>>
>> interrupt_return_srr_user()
>> interrupt_exit_user_prepare()
>> interrupt_exit_user_prepare_main()
>> do_notify_resume()
>> get_signal()
>> task_work_run()
>> create_worker_cb()
>> create_io_worker()
>> copy_process()
>> dup_task_struct()
>> arch_dup_task_struct()
>> flush_all_to_thread()
>> save_all()
>> if (tsk->thread.regs->msr & MSR_FP)
>> save_fpu()
>> # fr0 is clobbered and potentially live in userspace
>>
>>
>> So tldr I think the corruption is only an issue since io-uring started doing
>> the clone via signal, which I think matches the observed timeline of this bug
>> appearing.
>
> I agree the corruption really only started showing up in earnest on
> io_uring clone-via-signal, as this was confirmed several times in the
> course of debugging.
Thanks.
> Note as well that I may very well have a wrong call order in the
> commit message, since I was relying on a couple of WARN_ON() macros I
> inserted to check for a similar (but not identical) condition and
> didn't spend much time getting new traces after identifying the root
> cause.
Yep no worries. I'll reword it to incorporate the full path from my mail.
> I went back and grabbed some real world system-wide stack traces, since I now know what to trigger on. A typical example is:
>
> interrupt_return_srr_user()
> interrupt_exit_user_prepare()
> interrupt_exit_user_prepare_main()
> schedule()
> __schedule()
> __switch_to()
> giveup_all()
> # tsk->thread.regs->msr MSR_FP is still set here
> __giveup_fpu()
> save_fpu()
> # fr0 is clobbered and potentially live in userspace
fr0 is not live there.
__giveup_fpu() does roughly:
msr = tsk->thread.regs->msr;
msr &= ~(MSR_FP|MSR_FE0|MSR_FE1);
msr &= ~MSR_VSX;
tsk->thread.regs = msr;
ie. it clears the FP etc. bits from the task's MSR. That means the FP
state will be reloaded from the thread struct before the task is run again.
Also on that path we're switching to another task, so we'll be reloading
the other task's FP state before returning to userspace.
So I don't see any bug there.
There's only two places that call save_fpu() and skip the giveup logic,
which is save_all() and kvmppc_save_user_regs().
save_all() is only called via clone() so I think that's unable to
actually cause visible register corruption as I described in my previous
mail.
I thought the KVM case was similar, as it's called via an ioctl, but
I'll have to talk to Nick as his mail indicates otherwise.
cheers
next prev parent reply other threads:[~2023-11-20 23:40 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-19 15:18 [PATCH v2] powerpc: Don't clobber fr0/vs0 during fp|altivec register save Timothy Pearson
2023-11-20 7:10 ` Michael Ellerman
2023-11-20 14:32 ` Nicholas Piggin
2023-11-20 16:45 ` Timothy Pearson
2023-11-20 16:45 ` Timothy Pearson
2023-11-20 23:39 ` Michael Ellerman [this message]
2023-11-20 23:39 ` Michael Ellerman
2023-11-21 0:27 ` Nicholas Piggin
2023-11-21 0:27 ` Nicholas Piggin
2023-11-21 1:23 ` Timothy Pearson
2023-11-21 1:23 ` Timothy Pearson
2023-11-21 7:56 ` Nicholas Piggin
2023-11-21 7:56 ` Nicholas Piggin
2023-11-21 4:10 ` Timothy Pearson
2023-11-21 4:10 ` Timothy Pearson
2023-11-21 4:26 ` Timothy Pearson
2023-11-21 4:26 ` Timothy Pearson
2023-11-21 7:54 ` Nicholas Piggin
2023-11-21 7:54 ` Nicholas Piggin
2023-11-22 5:01 ` Michael Ellerman
2023-11-22 5:01 ` Michael Ellerman
2023-11-24 0:01 ` Timothy Pearson
2023-11-24 0:01 ` Timothy Pearson
2023-11-27 18:39 ` Timothy Pearson
2023-11-27 18:39 ` Timothy Pearson
2023-11-27 19:58 ` Christophe Leroy
2023-11-27 19:58 ` Christophe Leroy
2023-11-28 0:59 ` Michael Ellerman
2023-11-28 0:59 ` Michael Ellerman
2023-11-28 1:40 ` Nicholas Piggin
2023-11-28 1:40 ` Nicholas Piggin
2023-11-27 22:53 ` Michael Ellerman
2023-11-27 22:53 ` Michael Ellerman
2023-11-28 12:57 ` Michael Ellerman
2023-11-28 12:57 ` Michael Ellerman
2023-11-30 16:29 ` Timothy Pearson
2023-11-30 16:29 ` Timothy Pearson
2023-11-21 0:18 ` Nicholas Piggin
2023-11-21 0:18 ` Nicholas Piggin
2023-12-02 23:00 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874jhg6lkn.fsf@mail.lhotse \
--to=mpe@ellerman.id.au \
--cc=axboe@kernel.dk \
--cc=christophe.leroy@csgroup.eu \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=npiggin@gmail.com \
--cc=regressions@lists.linux.dev \
--cc=tpearson@raptorengineering.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.