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 X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A4F44C63697 for ; Sat, 28 Nov 2020 04:21:50 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2129F2078D for ; Sat, 28 Nov 2020 04:21:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2129F2078D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4FBC86B0068; Fri, 27 Nov 2020 23:21:49 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4AC876B006C; Fri, 27 Nov 2020 23:21:49 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3C3626B006E; Fri, 27 Nov 2020 23:21:49 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0074.hostedemail.com [216.40.44.74]) by kanga.kvack.org (Postfix) with ESMTP id 277866B0068 for ; Fri, 27 Nov 2020 23:21:49 -0500 (EST) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id E17CB3643 for ; Sat, 28 Nov 2020 04:21:48 +0000 (UTC) X-FDA: 77532528696.24.swing72_58079ee2738d Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin24.hostedemail.com (Postfix) with ESMTP id BF5561A4A0 for ; Sat, 28 Nov 2020 04:21:48 +0000 (UTC) X-HE-Tag: swing72_58079ee2738d X-Filterd-Recvd-Size: 4816 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf23.hostedemail.com (Postfix) with ESMTP for ; Sat, 28 Nov 2020 04:21:48 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 65013D6E; Fri, 27 Nov 2020 20:21:47 -0800 (PST) Received: from [10.163.85.48] (unknown [10.163.85.48]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0B5113F23F; Fri, 27 Nov 2020 20:21:45 -0800 (PST) Subject: Re: [RFC 1/3] mm/hotplug: Pre-validate the address range with platform To: David Hildenbrand , linux-mm@kvack.org, akpm@linux-foundation.org Cc: linux-kernel@vger.kernel.org References: <1606098529-7907-1-git-send-email-anshuman.khandual@arm.com> <1606098529-7907-2-git-send-email-anshuman.khandual@arm.com> <13392308-45a8-f85d-b25e-4a728e1e0730@redhat.com> <0c13a221-570a-0d64-fce9-d28e52cbdd6c@arm.com> <8886f78e-28cf-ded0-1629-d4206205be96@redhat.com> From: Anshuman Khandual Message-ID: <1d0224a2-1edb-6a43-bf2d-0a6f3ef4746e@arm.com> Date: Sat, 28 Nov 2020 09:51:41 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <8886f78e-28cf-ded0-1629-d4206205be96@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 11/27/20 2:31 PM, David Hildenbrand wrote: >>> >>> "arch_get_mappable_range(void)" (or similar) ? >> >> The current name seems bit better (I guess). Because we are asking for >> max addressable range with or without the linear mapping. >> >>> >>> AFAIKs, both implementations (arm64/s390x) simply do the exact same >>> thing as memhp_get_pluggable_range() for !need_mapping. >> >> That is for now. Even the range without requiring linear mapping might not >> be the same (like now) for every platform as some might have constraints. >> So asking the platform ranges with or without linear mapping seems to be >> better and could accommodate special cases going forward. Anyways, there >> is an always an "all allowing" fallback option nonetheless. > > Let's keep it simple as long as we don't have a real scenario where this > would apply. Sure, will have the arch callback only when the range needs linear mapping. Otherwise, memhp_get_pluggable_range() can just fallback on [0...max_phys] for non linear mapping requests. > > [...] > >> >>> >>>> + return true; >>>> + >>>> + WARN(1, "Hotplug memory [%#llx-%#llx] exceeds maximum addressable range [%#llx-%#llx]\n", >>>> + start, end, memhp_range.start, memhp_range.end); >>> >>> pr_warn() (or even pr_warn_once()) >>> >>> while we're at it. No reason to eventually crash a system :) >> >> Didn't quite get it. How could this crash the system ? > > With panic_on_warn, which some distributions started to enable. Okay, got it. > > [...] > >>>> /* >>>> * Validate altmap is within bounds of the total request >>>> @@ -1109,6 +1089,9 @@ int __ref __add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags) >>>> struct resource *res; >>>> int ret; >>>> >>>> + if (!memhp_range_allowed(start, size, 1)) >>>> + return -ERANGE; >>> >>> We used to return -E2BIG, no? Maybe better keep that. >> >> ERANGE seems to be better as the range can overrun on either side. > > Did you check all callers that they can handle it? Should mention that > in the patch description then. Hmm, okay then. Lets keep -E2BIG to be less disruptive for the callers. > >> >>> >>>> + >>>> res = register_memory_resource(start, size, "System RAM"); >>>> if (IS_ERR(res)) >>>> return PTR_ERR(res); >>>> @@ -1123,6 +1106,9 @@ int add_memory(int nid, u64 start, u64 size, mhp_t mhp_flags) >>>> { >>>> int rc; >>>> >>>> + if (!memhp_range_allowed(start, size, 1)) >>>> + return -ERANGE; >>>> + >>>> lock_device_hotplug(); >>>> rc = __add_memory(nid, start, size, mhp_flags); >>>> unlock_device_hotplug(); >>>> @@ -1163,6 +1149,9 @@ int add_memory_driver_managed(int nid, u64 start, u64 size, >>>> resource_name[strlen(resource_name) - 1] != ')') >>>> return -EINVAL; >>>> >>>> + if (!memhp_range_allowed(start, size, 0)) >>>> + return -ERANGE; >>>> + >>>> lock_device_hotplug(); >>> >>> For all 3 cases, doing a single check in register_memory_resource() is >>> sufficient. >> >> Will replace with a single check in register_memory_resource(). But does >> add_memory_driver_managed() always require linear mapping ? The proposed >> check here did not ask for linear mapping in add_memory_driver_managed(). > > Yes, in that regard, it behaves just like add_memory(). Sure.