mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

  mm/memory_hotplug.c: don't fail hot unplug quite so eagerly

Some device drivers add memory to the system via memory hotplug. When
the driver is unloaded, that memory is hot-unplugged.

However, memory hot unplug can fail. And these days, it fails a little
too easily, with respect to the above case. Specifically, if a signal is
pending on the process, hot unplug fails. This leads directly to: the
user must reboot the machine in order to unload the driver, and
therefore the device is unusable until the machine is rebooted.

During teardown paths in the kernel, a higher tolerance for failures or
imperfections is often best. That is, it is often better to continue
with the teardown, than to error out too early.

So in this case, other things (unmovable pages, un-splittable huge
pages) can also cause the above problem. However, those are demonstrably
less common than simply having a pending signal. I've got bug reports
from users who can trivially reproduce this by killing their process
with a "kill -9", for example.

Fix this by soldering on with memory hot plug, even in the presence of
pending signals.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/memory_hotplug.c | 6 ------
 1 file changed, 6 deletions(-)
  

Hi,

This is v2 of a series that fixes up build errors and warnings for at
least the 64-bit builds on x86 with clang.

There are lots of changes since v1 [1], thanks to reviews from Peter Xu, David
Hildenbrand, and Muhammad Usama Anjum. These include:

* Using "make headers", and documenting that prerequisite as well.
* Better ways to avoid clang's Wformat-security warnings
* Added Cc's, ack-by's, reviewed-by's.
* Updated commit log messages.

The series also includes an optional "improvement" of moving some uffd
code into uffd-common.[ch], which is proving to be somewhat
controversial, and so if that doesn't get resolved, then patches 9 and
10 may just get dropped. They are not required in order to get a clean
build, now that "make headers" is happening.

[1]: https://lore.kernel.org/all/20230602013358.900637-1-jhubbard@nvidia.com/

thanks,

John Hubbard
NVIDIA

John Hubbard (11):
  selftests/mm: fix uffd-stress unused function warning
  selftests/mm: fix unused variable warnings in hugetlb-madvise.c,
    migration.c
  selftests/mm: fix "warning: expression which evaluates to zero..." in
    mlock2-tests.c
  selftests/mm: fix invocation of tests that are run via shell scripts
  selftests/mm: .gitignore: add mkdirty, va_high_addr_switch
  selftests/mm: fix two -Wformat-security warnings in uffd builds
  selftests/mm: fix a "possibly uninitialized" warning in pkey-x86.h
  selftests/mm: fix uffd-unit-tests.c build failure due to missing
    MADV_COLLAPSE
  selftests/mm: move psize(), pshift() into vm_utils.c
  selftests/mm: move uffd* routines from vm_util.c to uffd-common.c
  Documentation: kselftest: "make headers" is a prerequisite

 Documentation/dev-tools/kselftest.rst         |   1 +
 tools/testing/selftests/mm/.gitignore         |   2 +
 tools/testing/selftests/mm/Makefile           |   7 +-
 tools/testing/selftests/mm/cow.c              |   7 --
 tools/testing/selftests/mm/hugepage-mremap.c  |   2 +-
 tools/testing/selftests/mm/hugetlb-madvise.c  |   8 +-
 tools/testing/selftests/mm/khugepaged.c       |  10 --
 .../selftests/mm/ksm_functional_tests.c       |   2 +-
 tools/testing/selftests/mm/migration.c        |   5 +-
 tools/testing/selftests/mm/mlock2-tests.c     |   1 -
 tools/testing/selftests/mm/pkey-x86.h         |   2 +-
 tools/testing/selftests/mm/run_vmtests.sh     |   6 +-
 tools/testing/selftests/mm/uffd-common.c      | 105 +++++++++++++++++
 tools/testing/selftests/mm/uffd-common.h      |  12 +-
 tools/testing/selftests/mm/uffd-stress.c      |  10 --
 tools/testing/selftests/mm/uffd-unit-tests.c  |  16 +--
 tools/testing/selftests/mm/vm_util.c          | 106 ++----------------
 tools/testing/selftests/mm/vm_util.h          |  36 ++----
 18 files changed, 165 insertions(+), 173 deletions(-)


base-commit: 929ed21dfdb6ee94391db51c9eedb63314ef6847
  

On 6/19/23 18:17, John Hubbard wrote:

My script picked up stale patches! Apologies!

...the first one, "[PATCH] mm/memory_hotplug.c: don't fail
hot unplug quite so eagerly", is the only one I wanted to
send. arghhh


thanks,
  

On 20.06.23 03:17, John Hubbard wrote:
> mm/memory_hotplug.c: don't fail hot unplug quite so eagerly
> 
> Some device drivers add memory to the system via memory hotplug. When
> the driver is unloaded, that memory is hot-unplugged.

Which interfaces are they using to add/remove memory?

> 
> However, memory hot unplug can fail. And these days, it fails a little
> too easily, with respect to the above case. Specifically, if a signal is
> pending on the process, hot unplug fails. This leads directly to: the
> user must reboot the machine in order to unload the driver, and
> therefore the device is unusable until the machine is rebooted.

Why can't they retry in user space when offlining fails with -EINTR, or 
re-trigger driver unloading?

> 
> During teardown paths in the kernel, a higher tolerance for failures or
> imperfections is often best. That is, it is often better to continue
> with the teardown, than to error out too early.
> 
> So in this case, other things (unmovable pages, un-splittable huge
> pages) can also cause the above problem. However, those are demonstrably
> less common than simply having a pending signal. I've got bug reports
> from users who can trivially reproduce this by killing their process
> with a "kill -9", for example.
> 
> Fix this by soldering on with memory hot plug, even in the presence of
> pending signals.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>   mm/memory_hotplug.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 8e0fa209d533..57a46620a667 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>   	do {
>   		pfn = start_pfn;
>   		do {
> -			if (signal_pending(current)) {
> -				ret = -EINTR;
> -				reason = "signal backoff";
> -				goto failed_removal_isolated;
> -			}
> -
>   			cond_resched();
>   
>   			ret = scan_movable_pages(pfn, end_pfn, &pfn);

No, we can't remove that. It's documented behavior that exists precisely 
for that reason:

https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#id21

"
When offlining is triggered from user space, the offlining context can 
be terminated by sending a fatal signal. A timeout based offlining can 
easily be implemented via:

% timeout $TIMEOUT offline_block | failure_handling
"

Otherwise, there is no way to stop an userspace-triggered offline 
operation that loops forever in the kernel.

I guess switching to fatal_signal_pending() might help to some degree, 
it should keep the timeout trick working.

But it wouldn't help in your case because where root kills arbitrary 
processes. I'm not sure if that is something we should be paying 
attention to.
  

On 6/20/23 00:12, David Hildenbrand wrote:
> On 20.06.23 03:17, John Hubbard wrote:
>> mm/memory_hotplug.c: don't fail hot unplug quite so eagerly
>>
>> Some device drivers add memory to the system via memory hotplug. When
>> the driver is unloaded, that memory is hot-unplugged.
> 
> Which interfaces are they using to add/remove memory?

It's coming in from the kernel driver, like this:

offline_and_remove_memory()
     walk_memory_blocks()
         try_offline_memory_block()
             device_offline()
                 memory_subsys_offline()
                     offline_pages()

...and the above is getting invoked as part of killing a user space
process that was helping (for performance reasons) holding the device
nodes open. That triggers a final close of the file descriptors and
leads to tearing down the driver. The teardown succeeds even though
the memory was not offlined, and now everything is, to use a technical
term, "stuck". :)

More below...

> 
>>
>> However, memory hot unplug can fail. And these days, it fails a little
>> too easily, with respect to the above case. Specifically, if a signal is
>> pending on the process, hot unplug fails. This leads directly to: the
>> user must reboot the machine in order to unload the driver, and
>> therefore the device is unusable until the machine is rebooted.
> 
> Why can't they retry in user space when offlining fails with -EINTR, or re-trigger driver unloading?

If someone uses "kill -9" to kill that process, then we get here,
because user space cannot trap that signal.


...
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>       do {
>>           pfn = start_pfn;
>>           do {
>> -            if (signal_pending(current)) {
>> -                ret = -EINTR;
>> -                reason = "signal backoff";
>> -                goto failed_removal_isolated;
>> -            }
>> -
>>               cond_resched();
>>               ret = scan_movable_pages(pfn, end_pfn, &pfn);
> 
> No, we can't remove that. It's documented behavior that exists precisely for that reason:
> 
> https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#id21
> 
> "
> When offlining is triggered from user space, the offlining context can be terminated by sending a fatal signal. A timeout based offlining can easily be implemented via:
> 
> % timeout $TIMEOUT offline_block | failure_handling
> "
> 
> Otherwise, there is no way to stop an userspace-triggered offline operation that loops forever in the kernel.

OK yes, I see.

> 
> I guess switching to fatal_signal_pending() might help to some degree, it should keep the timeout trick working.
> 
> But it wouldn't help in your case because where root kills arbitrary processes. I'm not sure if that is something we should be paying attention to.
> 

Right. I think it would be more accurate perhaps, but it wouldn't help
this particular complaint.

Perhaps it is reasonable to claim that, "well, kill -9 *means* that you
end up here!" :) And the above patch clearly is not the way to go, but...

...what about discerning between "user initiated offline_pages" and
"offline pages as part of a driver shutdown/unload"?

thanks,
  

On 20.06.23 23:54, John Hubbard wrote:
> On 6/20/23 00:12, David Hildenbrand wrote:
>> On 20.06.23 03:17, John Hubbard wrote:
>>> mm/memory_hotplug.c: don't fail hot unplug quite so eagerly
>>>
>>> Some device drivers add memory to the system via memory hotplug. When
>>> the driver is unloaded, that memory is hot-unplugged.
>>
>> Which interfaces are they using to add/remove memory?
> 
> It's coming in from the kernel driver, like this:
> 
> offline_and_remove_memory()
>       walk_memory_blocks()
>           try_offline_memory_block()
>               device_offline()
>                   memory_subsys_offline()
>                       offline_pages()
> 
> ...and the above is getting invoked as part of killing a user space
> process that was helping (for performance reasons) holding the device
> nodes open. That triggers a final close of the file descriptors and
> leads to tearing down the driver. The teardown succeeds even though
> the memory was not offlined, and now everything is, to use a technical
> term, "stuck". :)
> 

Ah, I see, thanks! I thought it would just be offlining from user space.

> More below...
> 
>>
>>>
>>> However, memory hot unplug can fail. And these days, it fails a little
>>> too easily, with respect to the above case. Specifically, if a signal is
>>> pending on the process, hot unplug fails. This leads directly to: the
>>> user must reboot the machine in order to unload the driver, and
>>> therefore the device is unusable until the machine is rebooted.
>>
>> Why can't they retry in user space when offlining fails with -EINTR, or re-trigger driver unloading?
> 
> If someone uses "kill -9" to kill that process, then we get here,
> because user space cannot trap that signal.

Understood, thanks!

> 
> 
> ...
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>>        do {
>>>            pfn = start_pfn;
>>>            do {
>>> -            if (signal_pending(current)) {
>>> -                ret = -EINTR;
>>> -                reason = "signal backoff";
>>> -                goto failed_removal_isolated;
>>> -            }
>>> -
>>>                cond_resched();
>>>                ret = scan_movable_pages(pfn, end_pfn, &pfn);
>>
>> No, we can't remove that. It's documented behavior that exists precisely for that reason:
>>
>> https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#id21
>>
>> "
>> When offlining is triggered from user space, the offlining context can be terminated by sending a fatal signal. A timeout based offlining can easily be implemented via:
>>
>> % timeout $TIMEOUT offline_block | failure_handling
>> "
>>
>> Otherwise, there is no way to stop an userspace-triggered offline operation that loops forever in the kernel.
> 
> OK yes, I see.
> 
>>
>> I guess switching to fatal_signal_pending() might help to some degree, it should keep the timeout trick working.
>>
>> But it wouldn't help in your case because where root kills arbitrary processes. I'm not sure if that is something we should be paying attention to.
>>
> 
> Right. I think it would be more accurate perhaps, but it wouldn't help
> this particular complaint.
> 
> Perhaps it is reasonable to claim that, "well, kill -9 *means* that you
> end up here!" :) And the above patch clearly is not the way to go, but...
> 
> ...what about discerning between "user initiated offline_pages" and
> "offline pages as part of a driver shutdown/unload"?

Makes sense to me.

There are two ways for triggering it directly from user space:

1) drivers/base/core.c:online_store()
2) drivers/base/memory.c:state_store()

We cannot easily hook into 2) to indicate "we're offlining directly
from user space". SO we might have to do it the other way around.


Something along the following lines should do the trick (expect whitespace damage):


diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 53ee7654f009..acd4b739505a 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -152,6 +152,13 @@ void put_online_mems(void)
  
  bool movable_node_enabled = false;
  
+/*
+ * Protected by the device hotplug lock. Indicates whether device offlining
+ * is triggered from try_offline_memory_block() such that we don't fail memory
+ * offlining if a signal is pending.
+ */
+static bool mhp_in_try_offline_memory_block;
+
  #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
  int mhp_default_online_type = MMOP_OFFLINE;
  #else
@@ -1860,7 +1867,8 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
         do {
                 pfn = start_pfn;
                 do {
-                       if (signal_pending(current)) {
+                       if (!mhp_in_try_offline_memory_block &&
+                           signal_pending(current)) {
                                 ret = -EINTR;
                                 reason = "signal backoff";
                                 goto failed_removal_isolated;
@@ -2177,7 +2185,9 @@ static int try_offline_memory_block(struct memory_block *mem, void *arg)
         if (page && zone_idx(page_zone(page)) == ZONE_MOVABLE)
                 online_type = MMOP_ONLINE_MOVABLE;
  
+       mhp_in_try_offline_memory_block = true;
         rc = device_offline(&mem->dev);
+       mhp_in_try_offline_memory_block = false;
         /*
          * Default is MMOP_OFFLINE - change it only if offlining succeeded,
          * so try_reonline_memory_block() can do the right thing.



There is still arch/powerpc/platforms/pseries/hotplug-memory.c that calls
device_offline() and would fail on signals (not sure if relevant, like for virtio-mem it
shouldn't be that relevant).

I guess dlpar_remove_lmb() can now simply call offline_and_remove_memory().
[I might craft a patch later]
  

[...]

> 
> There is still arch/powerpc/platforms/pseries/hotplug-memory.c that calls
> device_offline() and would fail on signals (not sure if relevant, like for virtio-mem it
> shouldn't be that relevant).

Oh, and of course the ACPI-triggered device_offline().
  

On 6/21/23 01:11, David Hildenbrand wrote:
>> ...what about discerning between "user initiated offline_pages" and
>> "offline pages as part of a driver shutdown/unload"?
> 
> Makes sense to me.
> 
> There are two ways for triggering it directly from user space:
> 
> 1) drivers/base/core.c:online_store()
> 2) drivers/base/memory.c:state_store()
> 
> We cannot easily hook into 2) to indicate "we're offlining directly
> from user space". SO we might have to do it the other way around.
> 
> 
> Something along the following lines should do the trick (expect whitespace damage):
> 
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 53ee7654f009..acd4b739505a 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -152,6 +152,13 @@ void put_online_mems(void)
> 
>   bool movable_node_enabled = false;
> 
> +/*
> + * Protected by the device hotplug lock. Indicates whether device offlining
> + * is triggered from try_offline_memory_block() such that we don't fail memory
> + * offlining if a signal is pending.
> + */
> +static bool mhp_in_try_offline_memory_block;
> +
>   #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
>   int mhp_default_online_type = MMOP_OFFLINE;
>   #else
> @@ -1860,7 +1867,8 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>          do {
>                  pfn = start_pfn;
>                  do {
> -                       if (signal_pending(current)) {
> +                       if (!mhp_in_try_offline_memory_block &&
> +                           signal_pending(current)) {
>                                  ret = -EINTR;
>                                  reason = "signal backoff";
>                                  goto failed_removal_isolated;
> @@ -2177,7 +2185,9 @@ static int try_offline_memory_block(struct memory_block *mem, void *arg)
>          if (page && zone_idx(page_zone(page)) == ZONE_MOVABLE)
>                  online_type = MMOP_ONLINE_MOVABLE;
> 
> +       mhp_in_try_offline_memory_block = true;
>          rc = device_offline(&mem->dev);
> +       mhp_in_try_offline_memory_block = false;
>          /*
>           * Default is MMOP_OFFLINE - change it only if offlining succeeded,
>           * so try_reonline_memory_block() can do the right thing.
> 
> 
> 
> There is still arch/powerpc/platforms/pseries/hotplug-memory.c that calls
> device_offline() and would fail on signals (not sure if relevant, like for virtio-mem it
> shouldn't be that relevant).
> 
> I guess dlpar_remove_lmb() can now simply call offline_and_remove_memory().
> [I might craft a patch later]
> 

This direction looks good to me, I'd love to see a patch if you
put something together.


thanks,