2013-07-17 21:46:03

by Toshi Kani

[permalink] [raw]
Subject: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
interface, which allows a given memory address to be hot-added as
follows. (See Documentation/memory-hotplug.txt for more detail.)

# echo start_address_of_new_memory > /sys/devices/system/memory/probe

This probe interface is required on powerpc. On x86, however, ACPI
notifies a memory hotplug event to the kernel, which performs its
hotplug operation as the result. Therefore, users should not be
required to use this interface on x86. This probe interface is also
error-prone that the kernel blindly adds a given memory address
without checking if the memory is present on the system; no probing
is done despite of its name. The kernel crashes when a user requests
to online a memory block that is not present on the system.

This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
and clarifies it in Documentation/memory-hotplug.txt.

Signed-off-by: Toshi Kani <[email protected]>
---
Documentation/memory-hotplug.txt | 7 ++++---
arch/x86/Kconfig | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
index 8e5eacb..396d871 100644
--- a/Documentation/memory-hotplug.txt
+++ b/Documentation/memory-hotplug.txt
@@ -214,9 +214,10 @@ In some environments, especially virtualized environment, firmware will not
notify memory hotplug event to the kernel. For such environment, "probe"
interface is supported. This interface depends on CONFIG_ARCH_MEMORY_PROBE.

-Now, CONFIG_ARCH_MEMORY_PROBE is supported only by powerpc but it does not
-contain highly architecture codes. Please add config if you need "probe"
-interface.
+CONFIG_ARCH_MEMORY_PROBE is supported on powerpc only. On x86, this config
+option is disabled by default since ACPI notifies a memory hotplug event to
+the kernel, which performs its hotplug operation as the result. Please
+enable this option if you need the "probe" interface on x86.

Probe interface is located at
/sys/devices/system/memory/probe
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b32ebf9..0729682 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1344,7 +1344,7 @@ config ARCH_SELECT_MEMORY_MODEL
depends on ARCH_SPARSEMEM_ENABLE

config ARCH_MEMORY_PROBE
- def_bool y
+ def_bool n
depends on X86_64 && MEMORY_HOTPLUG

config ARCH_PROC_KCORE_TEXT


2013-07-17 23:22:36

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
> CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
> interface, which allows a given memory address to be hot-added as
> follows. (See Documentation/memory-hotplug.txt for more detail.)
>
> # echo start_address_of_new_memory > /sys/devices/system/memory/probe
>
> This probe interface is required on powerpc. On x86, however, ACPI
> notifies a memory hotplug event to the kernel, which performs its
> hotplug operation as the result. Therefore, users should not be
> required to use this interface on x86. This probe interface is also
> error-prone that the kernel blindly adds a given memory address
> without checking if the memory is present on the system; no probing
> is done despite of its name. The kernel crashes when a user requests
> to online a memory block that is not present on the system.
>
> This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
> and clarifies it in Documentation/memory-hotplug.txt.

Why don't you completely remove it? Who should use this strange interface?

2013-07-17 23:30:39

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Wed, 2013-07-17 at 19:22 -0400, KOSAKI Motohiro wrote:
> On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
> > CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
> > interface, which allows a given memory address to be hot-added as
> > follows. (See Documentation/memory-hotplug.txt for more detail.)
> >
> > # echo start_address_of_new_memory > /sys/devices/system/memory/probe
> >
> > This probe interface is required on powerpc. On x86, however, ACPI
> > notifies a memory hotplug event to the kernel, which performs its
> > hotplug operation as the result. Therefore, users should not be
> > required to use this interface on x86. This probe interface is also
> > error-prone that the kernel blindly adds a given memory address
> > without checking if the memory is present on the system; no probing
> > is done despite of its name. The kernel crashes when a user requests
> > to online a memory block that is not present on the system.
> >
> > This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
> > and clarifies it in Documentation/memory-hotplug.txt.
>
> Why don't you completely remove it? Who should use this strange interface?

According to the comment below, this probe interface is used on powerpc.
So, we cannot remove it, but to disable it on x86.

/*
* Some architectures will have custom drivers to do this, and
* will not need to do it from userspace. The fake hot-add code
* as well as ppc64 will do all of their discovery in userspace
* and will require this interface.
*/
#ifdef CONFIG_ARCH_MEMORY_PROBE
static ssize_t
memory_probe_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
:

Thanks,
-Toshi

2013-07-17 23:33:59

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Wed, Jul 17, 2013 at 7:29 PM, Toshi Kani <[email protected]> wrote:
> On Wed, 2013-07-17 at 19:22 -0400, KOSAKI Motohiro wrote:
>> On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
>> > CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
>> > interface, which allows a given memory address to be hot-added as
>> > follows. (See Documentation/memory-hotplug.txt for more detail.)
>> >
>> > # echo start_address_of_new_memory > /sys/devices/system/memory/probe
>> >
>> > This probe interface is required on powerpc. On x86, however, ACPI
>> > notifies a memory hotplug event to the kernel, which performs its
>> > hotplug operation as the result. Therefore, users should not be
>> > required to use this interface on x86. This probe interface is also
>> > error-prone that the kernel blindly adds a given memory address
>> > without checking if the memory is present on the system; no probing
>> > is done despite of its name. The kernel crashes when a user requests
>> > to online a memory block that is not present on the system.
>> >
>> > This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
>> > and clarifies it in Documentation/memory-hotplug.txt.
>>
>> Why don't you completely remove it? Who should use this strange interface?
>
> According to the comment below, this probe interface is used on powerpc.
> So, we cannot remove it, but to disable it on x86.

I meant x86. Why can't we completely remove ARCH_MEMORY_PROBE section
from x86 Kconfig?

2013-07-17 23:52:33

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Wed, 2013-07-17 at 19:33 -0400, KOSAKI Motohiro wrote:
> On Wed, Jul 17, 2013 at 7:29 PM, Toshi Kani <[email protected]> wrote:
> > On Wed, 2013-07-17 at 19:22 -0400, KOSAKI Motohiro wrote:
> >> On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
> >> > CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
> >> > interface, which allows a given memory address to be hot-added as
> >> > follows. (See Documentation/memory-hotplug.txt for more detail.)
> >> >
> >> > # echo start_address_of_new_memory > /sys/devices/system/memory/probe
> >> >
> >> > This probe interface is required on powerpc. On x86, however, ACPI
> >> > notifies a memory hotplug event to the kernel, which performs its
> >> > hotplug operation as the result. Therefore, users should not be
> >> > required to use this interface on x86. This probe interface is also
> >> > error-prone that the kernel blindly adds a given memory address
> >> > without checking if the memory is present on the system; no probing
> >> > is done despite of its name. The kernel crashes when a user requests
> >> > to online a memory block that is not present on the system.
> >> >
> >> > This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
> >> > and clarifies it in Documentation/memory-hotplug.txt.
> >>
> >> Why don't you completely remove it? Who should use this strange interface?
> >
> > According to the comment below, this probe interface is used on powerpc.
> > So, we cannot remove it, but to disable it on x86.
>
> I meant x86. Why can't we completely remove ARCH_MEMORY_PROBE section
> from x86 Kconfig?

Oh, I see what you meant. I do not expect any need for end-users, but I
was not sure if someone working on the memory hotplug development might
use it for fake hot-add testing. Yes, if you folks do not see any need,
I will remove it from x86 Kconfig.

Thanks,
-Toshi

2013-07-18 00:25:15

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Wed, Jul 17, 2013 at 7:51 PM, Toshi Kani <[email protected]> wrote:
> On Wed, 2013-07-17 at 19:33 -0400, KOSAKI Motohiro wrote:
>> On Wed, Jul 17, 2013 at 7:29 PM, Toshi Kani <[email protected]> wrote:
>> > On Wed, 2013-07-17 at 19:22 -0400, KOSAKI Motohiro wrote:
>> >> On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
>> >> > CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
>> >> > interface, which allows a given memory address to be hot-added as
>> >> > follows. (See Documentation/memory-hotplug.txt for more detail.)
>> >> >
>> >> > # echo start_address_of_new_memory > /sys/devices/system/memory/probe
>> >> >
>> >> > This probe interface is required on powerpc. On x86, however, ACPI
>> >> > notifies a memory hotplug event to the kernel, which performs its
>> >> > hotplug operation as the result. Therefore, users should not be
>> >> > required to use this interface on x86. This probe interface is also
>> >> > error-prone that the kernel blindly adds a given memory address
>> >> > without checking if the memory is present on the system; no probing
>> >> > is done despite of its name. The kernel crashes when a user requests
>> >> > to online a memory block that is not present on the system.
>> >> >
>> >> > This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
>> >> > and clarifies it in Documentation/memory-hotplug.txt.
>> >>
>> >> Why don't you completely remove it? Who should use this strange interface?
>> >
>> > According to the comment below, this probe interface is used on powerpc.
>> > So, we cannot remove it, but to disable it on x86.
>>
>> I meant x86. Why can't we completely remove ARCH_MEMORY_PROBE section
>> from x86 Kconfig?
>
> Oh, I see what you meant. I do not expect any need for end-users, but I
> was not sure if someone working on the memory hotplug development might
> use it for fake hot-add testing. Yes, if you folks do not see any need,
> I will remove it from x86 Kconfig.

Then it's ok to submit your patch now.

Acked-by: KOSAKI Motohiro <[email protected]>

2013-07-18 00:31:28

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Wed, 2013-07-17 at 20:24 -0400, KOSAKI Motohiro wrote:
> On Wed, Jul 17, 2013 at 7:51 PM, Toshi Kani <[email protected]> wrote:
> > On Wed, 2013-07-17 at 19:33 -0400, KOSAKI Motohiro wrote:
> >> On Wed, Jul 17, 2013 at 7:29 PM, Toshi Kani <[email protected]> wrote:
> >> > On Wed, 2013-07-17 at 19:22 -0400, KOSAKI Motohiro wrote:
> >> >> On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
> >> >> > CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
> >> >> > interface, which allows a given memory address to be hot-added as
> >> >> > follows. (See Documentation/memory-hotplug.txt for more detail.)
> >> >> >
> >> >> > # echo start_address_of_new_memory > /sys/devices/system/memory/probe
> >> >> >
> >> >> > This probe interface is required on powerpc. On x86, however, ACPI
> >> >> > notifies a memory hotplug event to the kernel, which performs its
> >> >> > hotplug operation as the result. Therefore, users should not be
> >> >> > required to use this interface on x86. This probe interface is also
> >> >> > error-prone that the kernel blindly adds a given memory address
> >> >> > without checking if the memory is present on the system; no probing
> >> >> > is done despite of its name. The kernel crashes when a user requests
> >> >> > to online a memory block that is not present on the system.
> >> >> >
> >> >> > This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
> >> >> > and clarifies it in Documentation/memory-hotplug.txt.
> >> >>
> >> >> Why don't you completely remove it? Who should use this strange interface?
> >> >
> >> > According to the comment below, this probe interface is used on powerpc.
> >> > So, we cannot remove it, but to disable it on x86.
> >>
> >> I meant x86. Why can't we completely remove ARCH_MEMORY_PROBE section
> >> from x86 Kconfig?
> >
> > Oh, I see what you meant. I do not expect any need for end-users, but I
> > was not sure if someone working on the memory hotplug development might
> > use it for fake hot-add testing. Yes, if you folks do not see any need,
> > I will remove it from x86 Kconfig.
>
> Then it's ok to submit your patch now.

Great! I will submit an updated patch tomorrow.

> Acked-by: KOSAKI Motohiro <[email protected]>

Thanks!
-Toshi


2013-07-18 15:28:27

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On 07/17/2013 02:45 PM, Toshi Kani wrote:
> +CONFIG_ARCH_MEMORY_PROBE is supported on powerpc only. On x86, this config
> +option is disabled by default since ACPI notifies a memory hotplug event to
> +the kernel, which performs its hotplug operation as the result. Please
> +enable this option if you need the "probe" interface on x86.

There's no prompt for this and no way to override what you've done here
without hacking Kconfig/.config files.

It's also completely wrong to say "CONFIG_ARCH_MEMORY_PROBE is supported
on powerpc only." It works just fine on x86. In fact, I was just using
it today without ACPI being around.

I'd really prefer you don't do this. Do you really have random
processes on your system poking at random sysfs files and then
complaining when things break?

2013-07-18 16:03:11

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Thu, 2013-07-18 at 10:48 +0900, Yasuaki Ishimatsu wrote:
> (2013/07/18 8:51), Toshi Kani wrote:
> > On Wed, 2013-07-17 at 19:33 -0400, KOSAKI Motohiro wrote:
> >> On Wed, Jul 17, 2013 at 7:29 PM, Toshi Kani <[email protected]> wrote:
> >>> On Wed, 2013-07-17 at 19:22 -0400, KOSAKI Motohiro wrote:
> >>>> On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
> >>>>> CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
> >>>>> interface, which allows a given memory address to be hot-added as
> >>>>> follows. (See Documentation/memory-hotplug.txt for more detail.)
> >>>>>
> >>>>> # echo start_address_of_new_memory > /sys/devices/system/memory/probe
> >>>>>
> >>>>> This probe interface is required on powerpc. On x86, however, ACPI
> >>>>> notifies a memory hotplug event to the kernel, which performs its
> >>>>> hotplug operation as the result. Therefore, users should not be
> >>>>> required to use this interface on x86. This probe interface is also
> >>>>> error-prone that the kernel blindly adds a given memory address
> >>>>> without checking if the memory is present on the system; no probing
> >>>>> is done despite of its name. The kernel crashes when a user requests
> >>>>> to online a memory block that is not present on the system.
> >>>>>
> >>>>> This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
> >>>>> and clarifies it in Documentation/memory-hotplug.txt.
> >>>>
> >>>> Why don't you completely remove it? Who should use this strange interface?
> >>>
> >>> According to the comment below, this probe interface is used on powerpc.
> >>> So, we cannot remove it, but to disable it on x86.
> >>
> >> I meant x86. Why can't we completely remove ARCH_MEMORY_PROBE section
> >> from x86 Kconfig?
> >
> > Oh, I see what you meant. I do not expect any need for end-users, but I
> > was not sure if someone working on the memory hotplug development might
> > use it for fake hot-add testing. Yes, if you folks do not see any need,
> > I will remove it from x86 Kconfig.
>
> I do not think the interface is necessary. So I vote to Kosaki's opinion.

Thanks for the confirmation!
-Toshi


>
> Thanks,
> Yasuaki Ishimatsu
>
> >
> > Thanks,
> > -Toshi
> >
>
>

2013-07-18 16:27:47

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Thu, 2013-07-18 at 08:27 -0700, Dave Hansen wrote:
> On 07/17/2013 02:45 PM, Toshi Kani wrote:
> > +CONFIG_ARCH_MEMORY_PROBE is supported on powerpc only. On x86, this config
> > +option is disabled by default since ACPI notifies a memory hotplug event to
> > +the kernel, which performs its hotplug operation as the result. Please
> > +enable this option if you need the "probe" interface on x86.
>
> There's no prompt for this and no way to override what you've done here
> without hacking Kconfig/.config files.
>
> It's also completely wrong to say "CONFIG_ARCH_MEMORY_PROBE is supported
> on powerpc only." It works just fine on x86. In fact, I was just using
> it today without ACPI being around.

This statement has been there in the document (no change), and I
consider "supported" and "may work" are two different things.

> I'd really prefer you don't do this. Do you really have random
> processes on your system poking at random sysfs files and then
> complaining when things break?

I am afraid that the "probe" interface does not provide the level of
quality suitable for regular users. It takes any value and blindly
extends the page table. Also, we are not aware of the use of this
interface on x86. Would you elaborate why you need this interface on
x86? Is it for your testing, or is it necessary for end-users? If the
former, can you modify .config file to enable it?

Thanks,
-Toshi


2013-07-18 18:34:41

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On 07/18/2013 09:26 AM, Toshi Kani wrote:
> On Thu, 2013-07-18 at 08:27 -0700, Dave Hansen wrote:
>> I'd really prefer you don't do this. Do you really have random
>> processes on your system poking at random sysfs files and then
>> complaining when things break?
>
> I am afraid that the "probe" interface does not provide the level of
> quality suitable for regular users. It takes any value and blindly
> extends the page table.

That's like saying that /dev/sda takes any value and blindly writes it
to the disk.

> Also, we are not aware of the use of this
> interface on x86. Would you elaborate why you need this interface on
> x86? Is it for your testing, or is it necessary for end-users? If the
> former, can you modify .config file to enable it?

For me, it's testing. It allows testing of the memory hotplug software
stack without actual hardware, which is incredibly valuable. That
includes testing on distribution kernels which I do not want to modify.
I thought there were some hypervisor users which don't use ACPI for
hotplug event notifications too.

All that I'm asking is that you either leave it the way it is, or make a
Kconfig menu entry for it.

But, really, what's the problem that you're solving? Has this caused
you issues somehow? It's been there for, what, 10 years? Surely it's
part of the ABI.

2013-07-18 20:11:44

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Thu, 2013-07-18 at 11:34 -0700, Dave Hansen wrote:
> On 07/18/2013 09:26 AM, Toshi Kani wrote:
> > On Thu, 2013-07-18 at 08:27 -0700, Dave Hansen wrote:
> >> I'd really prefer you don't do this. Do you really have random
> >> processes on your system poking at random sysfs files and then
> >> complaining when things break?
> >
> > I am afraid that the "probe" interface does not provide the level of
> > quality suitable for regular users. It takes any value and blindly
> > extends the page table.
>
> That's like saying that /dev/sda takes any value and blindly writes it
> to the disk.

I do not think so. Using echo command to write a value to /dev/sda is
not how it is instructed to use in the document. I am not saying that
we need to protect from a privileged user doing something crazy.

> > Also, we are not aware of the use of this
> > interface on x86. Would you elaborate why you need this interface on
> > x86? Is it for your testing, or is it necessary for end-users? If the
> > former, can you modify .config file to enable it?
>
> For me, it's testing. It allows testing of the memory hotplug software
> stack without actual hardware, which is incredibly valuable. That
> includes testing on distribution kernels which I do not want to modify.
> I thought there were some hypervisor users which don't use ACPI for
> hotplug event notifications too.

I think such hypervisor relies on a balloon driver and does not use this
interface.

> All that I'm asking is that you either leave it the way it is, or make a
> Kconfig menu entry for it.
>
> But, really, what's the problem that you're solving? Has this caused
> you issues somehow? It's been there for, what, 10 years? Surely it's
> part of the ABI.

The problem is that the probe interface is documented as one of the
steps that may be necessary for memory hotplug. A typical user may or
may not know if his/her platform generates a hotplug notification to the
kernel to decide if this step is necessary. If the user performs this
step on x86, it will likely mess up the system. Since we do not need it
on x86, a prudent approach to protect such user is to disable or remove
the interface on x86 and document it accordingly. We have not seen this
issue yet because we do not have many platforms that support memory
hotplug today. Once memory hotplug support in KVM gets merged into the
mainline, anyone can start using this feature on their systems. At that
time, their choice of a stable kernel may be 3.12.x. This interface has
been there for while, but we need to fix it before the memory hotplug
feature becomes available for everyone.

Does it make sense? I understand that you are using this interface for
your testing. If I make a Kconfig menu entry, are you OK to disable
this option by default?

Motohiro, Yasuaki, do you have any suggestion?

Thanks,
-Toshi

2013-07-18 20:29:30

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On 07/18/2013 01:10 PM, Toshi Kani wrote:
> On Thu, 2013-07-18 at 11:34 -0700, Dave Hansen wrote:
> I do not think so. Using echo command to write a value to /dev/sda is
> not how it is instructed to use in the document. I am not saying that
> we need to protect from a privileged user doing something crazy.

If the document is the issue, then let's fix the document.

>> All that I'm asking is that you either leave it the way it is, or make a
>> Kconfig menu entry for it.
>>
>> But, really, what's the problem that you're solving? Has this caused
>> you issues somehow? It's been there for, what, 10 years? Surely it's
>> part of the ABI.
>
> The problem is that the probe interface is documented as one of the
> steps that may be necessary for memory hotplug. A typical user may or
> may not know if his/her platform generates a hotplug notification to the
> kernel to decide if this step is necessary.

A typical user will never see any of this stuff. It's buried deep under
the covers.

> If the user performs this
> step on x86, it will likely mess up the system. Since we do not need it
> on x86, a prudent approach to protect such user is to disable or remove
> the interface on x86 and document it accordingly. We have not seen this
> issue yet because we do not have many platforms that support memory
> hotplug today. Once memory hotplug support in KVM gets merged into the
> mainline, anyone can start using this feature on their systems. At that
> time, their choice of a stable kernel may be 3.12.x. This interface has
> been there for while, but we need to fix it before the memory hotplug
> feature becomes available for everyone.

It sounds like you're arguing that anyone using memory hotplug on x86
might be confused by the probe file. There's been a lot of hardware out
there that's supported memory hotplug for many, many years. I've never
heard a complaint about it in practice. Are KVM users more apt to be
confused than folks running on bare-metal? :)

> Does it make sense? I understand that you are using this interface for
> your testing. If I make a Kconfig menu entry, are you OK to disable
> this option by default?

I kinda wish you wouldn't mess with it. But, sure, put it in the memory
debugging, and make sure it stays enabled on powerpc by default.

Another method would be to just change the default permissions of the
file on x86 instead of disabling it completely:

# chmod u-w /sys/devices/system/memory/probe
# echo x > /sys/devices/system/memory/probe
bash: /sys/devices/system/memory/probe: Permission denied

That way folks can re-chmod it if they *really* want it back (me), and
they can still use it for testing.

2013-07-18 21:39:10

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Thu, 2013-07-18 at 13:29 -0700, Dave Hansen wrote:
> On 07/18/2013 01:10 PM, Toshi Kani wrote:
> > On Thu, 2013-07-18 at 11:34 -0700, Dave Hansen wrote:
> > I do not think so. Using echo command to write a value to /dev/sda is
> > not how it is instructed to use in the document. I am not saying that
> > we need to protect from a privileged user doing something crazy.
>
> If the document is the issue, then let's fix the document.

I will clarify the document as well.

> >> All that I'm asking is that you either leave it the way it is, or make a
> >> Kconfig menu entry for it.
> >>
> >> But, really, what's the problem that you're solving? Has this caused
> >> you issues somehow? It's been there for, what, 10 years? Surely it's
> >> part of the ABI.
> >
> > The problem is that the probe interface is documented as one of the
> > steps that may be necessary for memory hotplug. A typical user may or
> > may not know if his/her platform generates a hotplug notification to the
> > kernel to decide if this step is necessary.
>
> A typical user will never see any of this stuff. It's buried deep under
> the covers.

Users will need to use sysfs "memoryX/online" interface to online
hot-added memory, which is located in the same directory as "probe".
The name "probe" is also misleading that one would expect it checks if a
given memory address is present on the system.

> > If the user performs this
> > step on x86, it will likely mess up the system. Since we do not need it
> > on x86, a prudent approach to protect such user is to disable or remove
> > the interface on x86 and document it accordingly. We have not seen this
> > issue yet because we do not have many platforms that support memory
> > hotplug today. Once memory hotplug support in KVM gets merged into the
> > mainline, anyone can start using this feature on their systems. At that
> > time, their choice of a stable kernel may be 3.12.x. This interface has
> > been there for while, but we need to fix it before the memory hotplug
> > feature becomes available for everyone.
>
> It sounds like you're arguing that anyone using memory hotplug on x86
> might be confused by the probe file. There's been a lot of hardware out
> there that's supported memory hotplug for many, many years. I've never
> heard a complaint about it in practice. Are KVM users more apt to be
> confused than folks running on bare-metal? :)

I know ia64 (which Kconfig does not have this option) and powerpc (which
this interface is required, but I believe it is well-automated by LPAR
mgmt tools) platforms support memory hotplug for many years, but I did
not know it is also the case with x86 platforms. The fact that ia64
does not have this interface makes me wonder why we enabled it on x86.

> > Does it make sense? I understand that you are using this interface for
> > your testing. If I make a Kconfig menu entry, are you OK to disable
> > this option by default?
>
> I kinda wish you wouldn't mess with it. But, sure, put it in the memory
> debugging, and make sure it stays enabled on powerpc by default.

I understand and I appreciate your flexibility. Since it is defined in
x86/Kconfig, the prompt will be under "Processor type and features".
Yes, it stays enabled on powerpc as I won't touch powerpc/Kconfig.

> Another method would be to just change the default permissions of the
> file on x86 instead of disabling it completely:
>
> # chmod u-w /sys/devices/system/memory/probe
> # echo x > /sys/devices/system/memory/probe
> bash: /sys/devices/system/memory/probe: Permission denied
>
> That way folks can re-chmod it if they *really* want it back (me), and
> they can still use it for testing.

That's an interesting idea, but I'd prefer not to introduce #ifdef to
the common code for this.

Thanks,
-Toshi

2013-07-19 17:57:09

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH] mm/hotplug, x86: Disable ARCH_MEMORY_PROBE by default

On Wed, 2013-07-17 at 20:24 -0400, KOSAKI Motohiro wrote:
> On Wed, Jul 17, 2013 at 7:51 PM, Toshi Kani <[email protected]> wrote:
> > On Wed, 2013-07-17 at 19:33 -0400, KOSAKI Motohiro wrote:
> >> On Wed, Jul 17, 2013 at 7:29 PM, Toshi Kani <[email protected]> wrote:
> >> > On Wed, 2013-07-17 at 19:22 -0400, KOSAKI Motohiro wrote:
> >> >> On Wed, Jul 17, 2013 at 5:45 PM, Toshi Kani <[email protected]> wrote:
> >> >> > CONFIG_ARCH_MEMORY_PROBE enables /sys/devices/system/memory/probe
> >> >> > interface, which allows a given memory address to be hot-added as
> >> >> > follows. (See Documentation/memory-hotplug.txt for more detail.)
> >> >> >
> >> >> > # echo start_address_of_new_memory > /sys/devices/system/memory/probe
> >> >> >
> >> >> > This probe interface is required on powerpc. On x86, however, ACPI
> >> >> > notifies a memory hotplug event to the kernel, which performs its
> >> >> > hotplug operation as the result. Therefore, users should not be
> >> >> > required to use this interface on x86. This probe interface is also
> >> >> > error-prone that the kernel blindly adds a given memory address
> >> >> > without checking if the memory is present on the system; no probing
> >> >> > is done despite of its name. The kernel crashes when a user requests
> >> >> > to online a memory block that is not present on the system.
> >> >> >
> >> >> > This patch disables CONFIG_ARCH_MEMORY_PROBE by default on x86,
> >> >> > and clarifies it in Documentation/memory-hotplug.txt.
> >> >>
> >> >> Why don't you completely remove it? Who should use this strange interface?
> >> >
> >> > According to the comment below, this probe interface is used on powerpc.
> >> > So, we cannot remove it, but to disable it on x86.
> >>
> >> I meant x86. Why can't we completely remove ARCH_MEMORY_PROBE section
> >> from x86 Kconfig?
> >
> > Oh, I see what you meant. I do not expect any need for end-users, but I
> > was not sure if someone working on the memory hotplug development might
> > use it for fake hot-add testing. Yes, if you folks do not see any need,
> > I will remove it from x86 Kconfig.
>
> Then it's ok to submit your patch now.
>
> Acked-by: KOSAKI Motohiro <[email protected]>

I just sent v2 patch, but did not put your Ack since I did not remove
the option from x86 Kconfig per discussions with Dave. I'd really
appreciate if you can take a look at the v2 and re-ack it if it still
makes sense to you.

Thanks,
-Toshi