linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: David Hildenbrand <david@redhat.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>,
	linux-mm@kvack.org, akpm@linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Subject: Re: [RFC 1/3] mm/hotplug: Pre-validate the address range with platform
Date: Fri, 27 Nov 2020 10:01:12 +0100	[thread overview]
Message-ID: <8886f78e-28cf-ded0-1629-d4206205be96@redhat.com> (raw)
In-Reply-To: <0c13a221-570a-0d64-fce9-d28e52cbdd6c@arm.com>

>>
>> "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.

[...]

> 
>>
>>> +		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.

[...]

>>>  		/*
>>>  		 * 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.

> 
>>
>>> +
>>>  	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().



-- 
Thanks,

David / dhildenb



  reply	other threads:[~2020-11-27  9:01 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-23  2:28 [RFC 0/3] mm/hotplug: Pre-validate the address range with platform Anshuman Khandual
2020-11-23  2:28 ` [RFC 1/3] " Anshuman Khandual
2020-11-25 17:04   ` David Hildenbrand
2020-11-26 13:43     ` Anshuman Khandual
2020-11-27  9:01       ` David Hildenbrand [this message]
2020-11-28  4:21         ` Anshuman Khandual
2020-11-23  2:28 ` [RFC 2/3] arm64/mm: Define arch_get_addressable_range() Anshuman Khandual
2020-11-23  2:28 ` [RFC 3/3] s390/mm: " Anshuman Khandual
2020-11-25 17:27   ` David Hildenbrand
2020-11-26 13:45     ` Anshuman Khandual

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=8886f78e-28cf-ded0-1629-d4206205be96@redhat.com \
    --to=david@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=anshuman.khandual@arm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).