Received: by 2002:a25:824b:0:0:0:0:0 with SMTP id d11csp7023772ybn; Mon, 30 Sep 2019 07:32:11 -0700 (PDT) X-Google-Smtp-Source: APXvYqz+jFtz0edBANjgmdz4DbTSmChLc++ZPAX4SJsugI9177OmnnNqmwFa/A5gNjODybc/OwMo X-Received: by 2002:a1c:3cc3:: with SMTP id j186mr16608449wma.119.1569853931067; Mon, 30 Sep 2019 07:32:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1569853931; cv=none; d=google.com; s=arc-20160816; b=fRfCiYnh2xSyVP7EWAeJcVtqsnLJRr0t+pzXL4mE4Vhfa0Cu10th84N8I05ldSrZbl zDVG+xjm1yflXAnudcXNZrIbJpZKAVmDZMOGiDXBEgTneR84l3Ufr1C7BZKuUB1jt2Si TTqTYLU5kR6fjLsGd2bVwYcGcwhldUgGFywggdGUnye2HUsO4P1awGIirfFTAR7B0GBs fDi+huqVlCnclUaJ6/XsYxXl12OLlEUfNa8DMCkIU04weg9WoTSkF46e0l/hhrLquwL5 wlEVfQ2Oz98O5qHOJksND+3SWBoJ9YBjwo14wWeuDMfLXvLUi2ZJc8nib9nuzbGGmh5M 96Jg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=sjn2J1X49Pi46tSFkNPgKivYUAqzgU4dvUWvnLT/A+k=; b=1HClzdLgkkyq9+4ai+RKKnxFqnIk/jwbKaDgcoNhjFYecKpNyu26DQ+wF0iBGaqpeR nQVibvmgoyyZdfWQWkUAFmTELduTMCiFAluujDEZLINwOIWV59MNWdgGkdDQAAcMt/tL 61LXNikilgUB3CwsV6b93dKh3xTA0C9DuhQI8c1axJa/Vl1jUbGI9ZOdhQBLcN1UWFij teE1kjnYdCYzymdhTsqbOClCjdm47ZRdxBv2reCDsoJhcJpurWXydwRphcFp0SzHIQDI LdsOckdFBd8Wlum7tmsdhRcHxG05OK/8jUMHX+jb4F9gQ9cfYjwamxUbMLut4LWSkqvd 78+Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=l1I5R6RI; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 96si7065211edq.24.2019.09.30.07.31.45; Mon, 30 Sep 2019 07:32:11 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=l1I5R6RI; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731678AbfI3OaC (ORCPT + 99 others); Mon, 30 Sep 2019 10:30:02 -0400 Received: from mail-qt1-f194.google.com ([209.85.160.194]:45900 "EHLO mail-qt1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731417AbfI3OaB (ORCPT ); Mon, 30 Sep 2019 10:30:01 -0400 Received: by mail-qt1-f194.google.com with SMTP id c21so17236983qtj.12 for ; Mon, 30 Sep 2019 07:29:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=sjn2J1X49Pi46tSFkNPgKivYUAqzgU4dvUWvnLT/A+k=; b=l1I5R6RItUb8JBSLMy2KjEKywL+N5KRy2KecVolnRM5Ue9kSrk+uZPBKH3waMz9HZC FaAcBM16VuGym3aAiw4UIxd6i/mG2L4vbxAekrkhGm4MeFvH/hxGLuCOsFKSE0Uh2Dtn XP+TMDF8+ggFz3idHPkP3Alr5WbqAQCWHQdDhC8m4Mz4bf4wgFu5iGFhbwu+qeLUMCjG i+hi3oZFSLmCZyZYXIxRoLn9eLh0RuZGfeNxxP91Zw69fFfxb2lEc/o7mZ7hBZY/1om8 XH7AfkwibVGvu/e943yOQ4XYxgwAmYKGL2aralp9u1cFWhrxoqRTusZUfL2taH6zev1r vxKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=sjn2J1X49Pi46tSFkNPgKivYUAqzgU4dvUWvnLT/A+k=; b=TRppucHeqnMw19LNnM7IUYn12tS1Ixe9+nV9/WcDJGVt8DU+bQ0xesX/XFG/h+DjD4 2aw+u30c5jxdMoM4VtCqpoB9s+t8gxW186G7HvkT6vtYYid9EP1Srw+BHIkOC2qdyHTo wTFSPHk0Asc52Nb98TNQ1BnixSttvUefpr10uWRWC75N+5SLIdS0BaLy0HLLmH15BUvS iuKX4tapkPCuHmJkEMYVSbjWLW2T5tIrA6Bc2mCnjmYU2VviDCnwbJGAQs7NUX7s6jzp iJTYn4xOfomCf+wS8jHV24yLRanp8H+i7tTHAlVCxEbre5crSaswLPnBEednbwSplAmu ao2w== X-Gm-Message-State: APjAAAX1/l4TFq0DUn5WJInYhaQQSb6nswVhNfYY2DD7Uw92hCBPTGAy KQiNT8HTx3pVul4LyLywmWriAds= X-Received: by 2002:a0c:b068:: with SMTP id l37mr20801495qvc.36.1569853799300; Mon, 30 Sep 2019 07:29:59 -0700 (PDT) Received: from gabell (nat-pool-bos-t.redhat.com. [66.187.233.206]) by smtp.gmail.com with ESMTPSA id g192sm5504351qke.52.2019.09.30.07.29.58 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 30 Sep 2019 07:29:58 -0700 (PDT) Date: Mon, 30 Sep 2019 10:29:53 -0400 From: Masayoshi Mizuma To: Dave Martin , Julien Grall Cc: Catalin Marinas , Will Deacon , linux-arm-kernel@lists.infradead.org, Masayoshi Mizuma , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] arm64/sve: Fix wrong free for task->thread.sve_state Message-ID: <20190930142952.7zwbucjvh2wxbzis@gabell> References: <20190927153949.29870-1-msys.mizuma@gmail.com> <20190930130244.GT27757@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190930130244.GT27757@arm.com> User-Agent: NeoMutt/20180716 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Julien and Dave, On Mon, Sep 30, 2019 at 02:02:46PM +0100, Dave Martin wrote: > On Mon, Sep 30, 2019 at 01:23:18PM +0100, Julien Grall wrote: > > Hi, > > > > On 27/09/2019 16:39, Masayoshi Mizuma wrote: > > >From: Masayoshi Mizuma > > > > > >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") > > > > Looking at the log, it looks like THREAD_INFO_IN_TASK was selected before > > the bc0ee4760364. So it should be fine to backport for all the Linux tree > > contain this commit. I think this patch is needed for the kernel has SVE support. I'll add the Cc tag as Dave said: Cc: stable@vger.kernel.org # 4.15+ So, I suppose this patch will be backported to stables 5.3.X, 5.2.X and longterm 4.19.X. Does this make sense? > > > > >Signed-off-by: Masayoshi Mizuma > > >Reported-by: Hidetoshi Seto > > >Suggested-by: Dave Martin > > > > I have tested the patch and can confirm that double-free disappeared after > > the patch is applied: > > > > Tested-by: Julien Grall Thank you so much! > > Good to have that confirmed -- thanks for verifying. > > [...] > > > >--- > > > 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. > > >- */ > > > > It would be good to explain in the commit message why tweaking "dst" in > > arch_dup_task_struct() is fine. > > > > From my understanding, Arm64 used to have thread_info on the stack. So it > > would not be possible to clear TIF_SVE until the stack is initialized. > > > > Now that the thread_info is part of the task, it should be valid to modify > > the flag from arch_dup_task_struct(). > > > > Note that technically, TIF_SVE does not need to be cleared from > > arch_dup_task_struct(). It could also be done from copy_thread(). But it is > > easier to keep the both changes together. Thanks, let me add some comments to the commit log. > > > > > int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > > > { > > > if (current->mm) > > > fpsimd_preserve_current_state(); > > > *dst = *src; > > Ack, some more explanation would be a good idea here. > > Maybe the following comments are sufficient? > > /* We rely on the above assingment to initialise dst's thread_flags: */ Thanks, I'll add this comment. > > > >+ BUILD_BUG_ON(!IS_ENABLED(CONFIG_THREAD_INFO_IN_TASK)); > > > > and > > /* > * Detach src's sve_state (if any) from dst so that it does not > * get erroneously used or freed prematurely. dst's sve_state > * will be allocated on demand later on if dst uses SVE. > * For consistency, also clear TIF_SVE here: this could be done > * later in copy_process(), but to avoid tripping up future > * maintainers it is best not to leave TIF_SVE and sve_state in > * an inconsistent state, even temporarily. > */ I'll add this comments. > > > >+ dst->thread.sve_state = NULL; > > >+ clear_tsk_thread_flag(dst, TIF_SVE); > > (TIF_SVE should not usually be set in the first place of course, since > we are in a fork() or clone() syscall in src. This may not be true if > a task is created using kernel_thread() while running in the context of > some user task that entered the kernel due to a trap or syscall -- > though probably nobody should be doing that.) Thanks! Masa