From: Masayoshi Mizuma <msys.mizuma@gmail.com> To: Dave Martin <Dave.Martin@arm.com>, Julien Grall <julien.grall@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, linux-arm-kernel@lists.infradead.org Cc: Masayoshi Mizuma <msys.mizuma@gmail.com>, Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>, linux-kernel@vger.kernel.org Subject: [PATCH v2] arm64/sve: Fix wrong free for task->thread.sve_state Date: Fri, 27 Sep 2019 11:39:49 -0400 [thread overview] Message-ID: <20190927153949.29870-1-msys.mizuma@gmail.com> (raw) From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> The system which has SVE feature crashed because of the memory pointed by task->thread.sve_state was destroyed by someone. That is because sve_state is freed while the forking the child process. The child process has the pointer of sve_state which is same as the parent's because the child's task_struct is copied from the parent's one. If the copy_process() fails as an error on somewhere, for example, copy_creds(), then the sve_state is freed even if the parent is alive. The flow is as follows. copy_process p = dup_task_struct => arch_dup_task_struct *dst = *src; // copy the entire region. : retval = copy_creds if (retval < 0) goto bad_fork_free; : bad_fork_free: ... delayed_free_task(p); => free_task => arch_release_task_struct => fpsimd_release_task => __sve_free => kfree(task->thread.sve_state); // free the parent's sve_state Move child's sve_state = NULL and clearing TIF_SVE flag to arch_dup_task_struct() so that the child doesn't free the parent's one. Cc: stable@vger.kernel.org Fixes: bc0ee4760364 ("arm64/sve: Core task context handling") Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Reported-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> Suggested-by: Dave Martin <Dave.Martin@arm.com> --- arch/arm64/kernel/process.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index f674f28df..6937f5935 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -323,22 +323,16 @@ void arch_release_task_struct(struct task_struct *tsk) fpsimd_release_task(tsk); } -/* - * src and dst may temporarily have aliased sve_state after task_struct - * is copied. We cannot fix this properly here, because src may have - * live SVE state and dst's thread_info may not exist yet, so tweaking - * either src's or dst's TIF_SVE is not safe. - * - * The unaliasing is done in copy_thread() instead. This works because - * dst is not schedulable or traceable until both of these functions - * have been called. - */ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) { if (current->mm) fpsimd_preserve_current_state(); *dst = *src; + BUILD_BUG_ON(!IS_ENABLED(CONFIG_THREAD_INFO_IN_TASK)); + dst->thread.sve_state = NULL; + clear_tsk_thread_flag(dst, TIF_SVE); + return 0; } @@ -351,13 +345,6 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start, memset(&p->thread.cpu_context, 0, sizeof(struct cpu_context)); - /* - * Unalias p->thread.sve_state (if any) from the parent task - * and disable discard SVE state for p: - */ - clear_tsk_thread_flag(p, TIF_SVE); - p->thread.sve_state = NULL; - /* * In case p was allocated the same task_struct pointer as some * other recently-exited task, make sure p is disassociated from -- 2.18.1
WARNING: multiple messages have this Message-ID (diff)
From: Masayoshi Mizuma <msys.mizuma@gmail.com> To: Dave Martin <Dave.Martin@arm.com>, Julien Grall <julien.grall@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, linux-arm-kernel@lists.infradead.org Cc: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>, Masayoshi Mizuma <msys.mizuma@gmail.com>, linux-kernel@vger.kernel.org Subject: [PATCH v2] arm64/sve: Fix wrong free for task->thread.sve_state Date: Fri, 27 Sep 2019 11:39:49 -0400 [thread overview] Message-ID: <20190927153949.29870-1-msys.mizuma@gmail.com> (raw) From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> The system which has SVE feature crashed because of the memory pointed by task->thread.sve_state was destroyed by someone. That is because sve_state is freed while the forking the child process. The child process has the pointer of sve_state which is same as the parent's because the child's task_struct is copied from the parent's one. If the copy_process() fails as an error on somewhere, for example, copy_creds(), then the sve_state is freed even if the parent is alive. The flow is as follows. copy_process p = dup_task_struct => arch_dup_task_struct *dst = *src; // copy the entire region. : retval = copy_creds if (retval < 0) goto bad_fork_free; : bad_fork_free: ... delayed_free_task(p); => free_task => arch_release_task_struct => fpsimd_release_task => __sve_free => kfree(task->thread.sve_state); // free the parent's sve_state Move child's sve_state = NULL and clearing TIF_SVE flag to arch_dup_task_struct() so that the child doesn't free the parent's one. Cc: stable@vger.kernel.org Fixes: bc0ee4760364 ("arm64/sve: Core task context handling") Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Reported-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> Suggested-by: Dave Martin <Dave.Martin@arm.com> --- arch/arm64/kernel/process.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index f674f28df..6937f5935 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -323,22 +323,16 @@ void arch_release_task_struct(struct task_struct *tsk) fpsimd_release_task(tsk); } -/* - * src and dst may temporarily have aliased sve_state after task_struct - * is copied. We cannot fix this properly here, because src may have - * live SVE state and dst's thread_info may not exist yet, so tweaking - * either src's or dst's TIF_SVE is not safe. - * - * The unaliasing is done in copy_thread() instead. This works because - * dst is not schedulable or traceable until both of these functions - * have been called. - */ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) { if (current->mm) fpsimd_preserve_current_state(); *dst = *src; + BUILD_BUG_ON(!IS_ENABLED(CONFIG_THREAD_INFO_IN_TASK)); + dst->thread.sve_state = NULL; + clear_tsk_thread_flag(dst, TIF_SVE); + return 0; } @@ -351,13 +345,6 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start, memset(&p->thread.cpu_context, 0, sizeof(struct cpu_context)); - /* - * Unalias p->thread.sve_state (if any) from the parent task - * and disable discard SVE state for p: - */ - clear_tsk_thread_flag(p, TIF_SVE); - p->thread.sve_state = NULL; - /* * In case p was allocated the same task_struct pointer as some * other recently-exited task, make sure p is disassociated from -- 2.18.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next reply other threads:[~2019-09-27 15:39 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-27 15:39 Masayoshi Mizuma [this message] 2019-09-27 15:39 ` [PATCH v2] arm64/sve: Fix wrong free for task->thread.sve_state Masayoshi Mizuma 2019-09-27 16:15 ` Dave Martin 2019-09-27 16:15 ` Dave Martin 2019-09-27 19:56 ` Julien Grall 2019-09-27 19:56 ` Julien Grall 2019-09-30 12:23 ` Julien Grall 2019-09-30 12:23 ` Julien Grall 2019-09-30 13:02 ` Dave Martin 2019-09-30 13:02 ` Dave Martin 2019-09-30 14:29 ` Masayoshi Mizuma 2019-09-30 14:29 ` Masayoshi Mizuma 2019-09-30 15:34 ` Dave Martin 2019-09-30 15:34 ` Dave Martin
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=20190927153949.29870-1-msys.mizuma@gmail.com \ --to=msys.mizuma@gmail.com \ --cc=Dave.Martin@arm.com \ --cc=catalin.marinas@arm.com \ --cc=julien.grall@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=m.mizuma@jp.fujitsu.com \ --cc=will@kernel.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: linkBe 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.