From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 96611C433EF for ; Mon, 13 Dec 2021 07:38:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=jYVEZak0sTSa+C1fjKD7CnG751UD8dMBwlNKCxa8HD8=; b=Tqc/6ICH7IGO9+ nYaIFIEbmOwyGRVVh23QRvYMQ8u2ih61bQgtWfpC1rIbl7JVsoA6bzS7aNLOm55Qw3UNDMfzYAHKj T8SzlDpFVN7d5OwoAKExviuWPG+YefPfQXTCh0IuzZ7ha8XPEClg2KWgrJFw9da37QvgoYnmvO2HE mvyJpv1eIQQn5JfutDAdAno1f6yjXDlkomjra7r32Fw4xbzbropQM/yrqV8IOQLiMHTLbSvbXxn0e JpqNRfpBGUBxB+hZvcSCOxJSRi6LTFMuN4cOA9AWU/o2+q7LQ2N0DSgNzFcVhfytmOgGccR9P8/ue zNZfOCd23y4eqaksiVng==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mwftk-0087CB-VS; Mon, 13 Dec 2021 07:37:25 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mwfta-008796-Fl for linux-arm-kernel@lists.infradead.org; Mon, 13 Dec 2021 07:37:16 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1639381028; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HG5GXt2X3txUdKV6Wl9T8pkiLGxRuVFP50xZy6gQCPI=; b=SSOXogj0Eg9NAgpyp6xHdY4S02jTqtYBZSdOrlwxeZWFtbgvgLVIC7IiOGWoVTpezfrGY3 bN/vDiWupOkXIP5dlzu372EYnH+Wg1K/8ZQisQmnHBd9Ts+66hcqHAkSVQReFP09nofdaR /wVUAOvHkPhvIAgUIdHdMdefzsq4qMw= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-427-xEvUE21YML65YcOef47Lng-1; Mon, 13 Dec 2021 02:37:07 -0500 X-MC-Unique: xEvUE21YML65YcOef47Lng-1 Received: by mail-wm1-f72.google.com with SMTP id n16-20020a05600c3b9000b003331973fdbbso6174396wms.0 for ; Sun, 12 Dec 2021 23:37:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:organization:in-reply-to :content-transfer-encoding; bh=HG5GXt2X3txUdKV6Wl9T8pkiLGxRuVFP50xZy6gQCPI=; b=li2yZogBB7VCxzsA2v/k8RyAeKNKE01bGNPPXDpNmza9DdG1hakNEMVVhzkbu/hc69 G1sA/SkV1W50D1QFwW63/KUhRR3SWhPPSZY+qwEAvHM7ckogJrInWE6M1JgLHjHKbnmQ eZUHaEXKut/BvqxSf/Bf7cC9UD7d3xrZs3/AT5ioJbaPj6GOKAt1AOfFrvvgF8V4ToMY CxnAFTnPKl5N19KuKFumx6fK5ubF/vrfTJ8yCx7p50oJyn5uarOJ26WFGJOvr+jzA1B5 dkj+EZ4+WIJi7ZvNPaMSrY+X5iIgwvJ+9z6+uW7YMJPpbQuVCet07UXVbelPow9QxJ3k Qp0w== X-Gm-Message-State: AOAM5331PyeuWJZnRMcrXgwbMHqnG+0c3V5WM3Qtg0B1MWE1okxqsvyH BA6XHtNo+cOgGCDKP7aPFSkowxRCYs+m7h/YWcLrbhFJq5lfZO7iNLRNj1pdoh09qT7rAYivEr2 bDLTyXXXhfi+iimJQULB18Lgn2iyEGzIfjY8= X-Received: by 2002:a1c:7f43:: with SMTP id a64mr35961253wmd.133.1639381026575; Sun, 12 Dec 2021 23:37:06 -0800 (PST) X-Google-Smtp-Source: ABdhPJzELkYNz/2adwZUHSUUG9XDtH76JyPqxjTL2ad3vfwMERgieLxcHKAA9dThvvLRhPxv+xoWbg== X-Received: by 2002:a1c:7f43:: with SMTP id a64mr35961235wmd.133.1639381026356; Sun, 12 Dec 2021 23:37:06 -0800 (PST) Received: from [192.168.3.132] (p5b0c6276.dip0.t-ipconnect.de. [91.12.98.118]) by smtp.gmail.com with ESMTPSA id t8sm7581987wmq.32.2021.12.12.23.37.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 12 Dec 2021 23:37:06 -0800 (PST) Message-ID: <3366ba9f-5993-1c52-de0c-53e618f20cd8@redhat.com> Date: Mon, 13 Dec 2021 08:37:04 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create pud mapping To: Jianyong Wu , Anshuman Khandual , Catalin Marinas , "will@kernel.org" , "akpm@linux-foundation.org" Cc: "ardb@kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "gshan@redhat.com" , Justin He , nd References: <20211210095432.51798-1-jianyong.wu@arm.com> From: David Hildenbrand Organization: Red Hat In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=david@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211212_233714_673888_EAA96307 X-CRM114-Status: GOOD ( 16.73 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 13.12.21 08:27, Jianyong Wu wrote: > > >> -----Original Message----- >> From: Anshuman Khandual >> Sent: Monday, December 13, 2021 2:56 PM >> To: Jianyong Wu ; Catalin Marinas >> ; will@kernel.org; akpm@linux-foundation.org >> Cc: ardb@kernel.org; linux-kernel@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; david@redhat.com; gshan@redhat.com; Justin >> He ; nd >> Subject: Re: [PATCH v2] arm64/mm: avoid fixmap race condition when create >> pud mapping >> >> >> >> On 12/10/21 3:24 PM, Jianyong Wu wrote: >>> fixmap is a global resource and is used recursively in create pud mapping. >>> It may lead to race condition when alloc_init_pud is called concurrently. >>> >>> Fox example: >>> alloc_init_pud is called when kernel_init. If memory hotplug thread, >>> which will also call alloc_init_pud, happens during kernel_init, the >>> race for fixmap occurs. >>> >>> The race condition flow can be: >>> >>> *************** begin ************** >>> >>> kerenl_init thread virtio-mem workqueue thread >>> ================== ======== ================== >>> alloc_init_pud(...) >>> pudp = pud_set_fixmap_offset(..) alloc_init_pud(...) >>> ... ... >>> READ_ONCE(*pudp) //OK! pudp = pud_set_fixmap_offset( >>> ... ... >>> pud_clear_fixmap() //fixmap break >>> READ_ONCE(*pudp) //CRASH! >>> >>> **************** end *************** >>> >>> Hence, a spin lock is introduced to protect the fixmap during create >>> pdg mapping. >>> >>> Signed-off-by: Jianyong Wu >>> --- >>> arch/arm64/mm/mmu.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index >>> acfae9b41cc8..98ac09ae9588 100644 >>> --- a/arch/arm64/mm/mmu.c >>> +++ b/arch/arm64/mm/mmu.c >>> @@ -63,6 +63,7 @@ static pmd_t bm_pmd[PTRS_PER_PMD] >> __page_aligned_bss >>> __maybe_unused; static pud_t bm_pud[PTRS_PER_PUD] >> __page_aligned_bss >>> __maybe_unused; >>> >>> static DEFINE_SPINLOCK(swapper_pgdir_lock); >>> +static DEFINE_SPINLOCK(fixmap_lock); >>> >>> void set_swapper_pgd(pgd_t *pgdp, pgd_t pgd) { @@ -329,6 +330,11 @@ >>> static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long >> end, >>> } >>> BUG_ON(p4d_bad(p4d)); >>> >>> + /* >>> + * fixmap is global resource, thus it needs to be protected by a lock >>> + * in case of race condition. >>> + */ >>> + spin_lock(&fixmap_lock); >>> pudp = pud_set_fixmap_offset(p4dp, addr); >>> do { >>> pud_t old_pud = READ_ONCE(*pudp); >>> @@ -359,6 +365,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned >> long addr, unsigned long end, >>> } while (pudp++, addr = next, addr != end); >>> >>> pud_clear_fixmap(); >>> + spin_unlock(&fixmap_lock); >>> } >>> >>> static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys, >>> >> >> As the race could only happen with memory hotplug being enabled, could >> not we wrap this around with CONFIG_MEMORY_HOTPLUG, just to narrow >> its scope possibly speed up other non-hotplug cases ? > > I think it's better. We better avoid using ifdef if not really necessary, it just uglifies the code. We could add if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG)) ... But should we really try to micto-optimize this code and make it harder to read for the purpose of an unproven performance gain? (if there is no contention, we'll most probably not even recognize that we're taking a lock) -- Thanks, David / dhildenb _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel