From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] arm64: fix bug for reloading FPSIMD state after execve on cpu 0.
Date: Wed, 26 Aug 2015 12:39:22 +0100 [thread overview]
Message-ID: <20150826113922.GE30466@arm.com> (raw)
In-Reply-To: <CAKv+Gu_VtFGsKPmFfLHm=kNM=vK=p1TR+fd52P68MiA6J0zUFQ@mail.gmail.com>
On Wed, Aug 26, 2015 at 12:32:03PM +0100, Ard Biesheuvel wrote:
> On 26 August 2015 at 13:12, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, Aug 26, 2015 at 03:40:41AM +0100, Chunyan Zhang wrote:
> >> From: Janet Liu <janet.liu@spreadtrum.com>
> >>
> >> If process A is running on CPU 0 and do execve syscall and after sched_exec,
> >> dest_cpu is 0, fpsimd_state.cpu is 0. If at the time Process A get scheduled
> >> out and after some kernel threads running on CPU 0, process A is back in CPU 0,
> >> A's fpsimd_state.cpu is current cpu id "0", and per_cpu(fpsimd_last_state)
> >> points A's fpsimd_state, TIF_FOREIGN_FPSTATE will be clear, kernel will not
> >> reload the context during it return to userspace. so set the cpu's
> >> fpsimd_last_state to NULL to avoid this.
> >
> > AFAICT, this is only a problem if one of the kernel threads uses the fpsimd
> > registers, right? However, kernel_neon_begin_partial clobbers
> > fpsimd_last_state, so I'm struggling to see the problem.
> >
>
> I think the problem is real, but it would be better to set the
> fpsimd_state::cpu field to an invalid value like we do in
> fpsimd_flush_task_state()
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 44d6f7545505..c56956a16d3f 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -158,6 +158,7 @@ void fpsimd_thread_switch(struct task_struct *next)
> void fpsimd_flush_thread(void)
> {
> memset(¤t->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
> + fpsimd_flush_task_state(current);
> set_thread_flag(TIF_FOREIGN_FPSTATE);
> }
>
> (note the memset erroneously initializes that field to CPU 0)
Aha, I see. So the problem is actually that we get a view on our fpsimd
state before the exec, rather than a view on some kernel state.
> This more accurately reflects the state of the process after forking,
> i.e., that its FPSIMD state has never been loaded into any CPU.
Yup, that's much clearer.
Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Chunyan Zhang <chunyan.zhang@spreadtrum.com>,
Catalin Marinas <Catalin.Marinas@arm.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"jianhua.ljh@gmail.com" <jianhua.ljh@gmail.com>,
"orson.zhai@spreadtrum.com" <orson.zhai@spreadtrum.com>,
"xiongshan.an@spreadtrum.com" <xiongshan.an@spreadtrum.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] arm64: fix bug for reloading FPSIMD state after execve on cpu 0.
Date: Wed, 26 Aug 2015 12:39:22 +0100 [thread overview]
Message-ID: <20150826113922.GE30466@arm.com> (raw)
In-Reply-To: <CAKv+Gu_VtFGsKPmFfLHm=kNM=vK=p1TR+fd52P68MiA6J0zUFQ@mail.gmail.com>
On Wed, Aug 26, 2015 at 12:32:03PM +0100, Ard Biesheuvel wrote:
> On 26 August 2015 at 13:12, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, Aug 26, 2015 at 03:40:41AM +0100, Chunyan Zhang wrote:
> >> From: Janet Liu <janet.liu@spreadtrum.com>
> >>
> >> If process A is running on CPU 0 and do execve syscall and after sched_exec,
> >> dest_cpu is 0, fpsimd_state.cpu is 0. If at the time Process A get scheduled
> >> out and after some kernel threads running on CPU 0, process A is back in CPU 0,
> >> A's fpsimd_state.cpu is current cpu id "0", and per_cpu(fpsimd_last_state)
> >> points A's fpsimd_state, TIF_FOREIGN_FPSTATE will be clear, kernel will not
> >> reload the context during it return to userspace. so set the cpu's
> >> fpsimd_last_state to NULL to avoid this.
> >
> > AFAICT, this is only a problem if one of the kernel threads uses the fpsimd
> > registers, right? However, kernel_neon_begin_partial clobbers
> > fpsimd_last_state, so I'm struggling to see the problem.
> >
>
> I think the problem is real, but it would be better to set the
> fpsimd_state::cpu field to an invalid value like we do in
> fpsimd_flush_task_state()
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 44d6f7545505..c56956a16d3f 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -158,6 +158,7 @@ void fpsimd_thread_switch(struct task_struct *next)
> void fpsimd_flush_thread(void)
> {
> memset(¤t->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
> + fpsimd_flush_task_state(current);
> set_thread_flag(TIF_FOREIGN_FPSTATE);
> }
>
> (note the memset erroneously initializes that field to CPU 0)
Aha, I see. So the problem is actually that we get a view on our fpsimd
state before the exec, rather than a view on some kernel state.
> This more accurately reflects the state of the process after forking,
> i.e., that its FPSIMD state has never been loaded into any CPU.
Yup, that's much clearer.
Will
next prev parent reply other threads:[~2015-08-26 11:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-26 2:40 [PATCH] arm64: fix bug for reloading FPSIMD state after execve on cpu 0 Chunyan Zhang
2015-08-26 2:40 ` Chunyan Zhang
2015-08-26 11:12 ` Will Deacon
2015-08-26 11:12 ` Will Deacon
2015-08-26 11:32 ` Ard Biesheuvel
2015-08-26 11:32 ` Ard Biesheuvel
2015-08-26 11:39 ` Will Deacon [this message]
2015-08-26 11:39 ` Will Deacon
2015-08-27 2:43 ` Jianhua Liu
2015-08-27 2:43 ` Jianhua Liu
2015-08-26 12:02 ` Jianhua Liu
2015-08-26 12:02 ` Jianhua Liu
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=20150826113922.GE30466@arm.com \
--to=will.deacon@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
/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.