[PATCH 0/3] powerpc/pseries/memhp: Fix minor bugs and improve error logging

[PATCH 0/3] powerpc/pseries/memhp: Fix minor bugs and improve error logging

[Nathan Lynch via B4 Relay]

This includes a fix for an array bounds read overrun that can be
triggered when debug messages are enabled.

---
Nathan Lynch (3):
      powerpc/pseries/memhp: Fix access beyond end of drmem array
      powerpc/pseries/memhp: Remove unbalanced dlpar_release_drc() call
      powerpc/pseries/memhp: Log more error conditions in add path

 arch/powerpc/platforms/pseries/hotplug-memory.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
---
base-commit: 707df298cbde200b939c70be2577b20775fe3345
change-id: 20231114-pseries-memhp-fixes-185988dde2a0

Best regards,
-- 
Nathan Lynch <nathanl@linux.ibm.com>

[PATCH 1/3] powerpc/pseries/memhp: Fix access beyond end of drmem array

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathanl@linux.ibm.com>

dlpar_memory_remove_by_index() may access beyond the bounds of the
drmem lmb array when the LMB lookup fails to match an entry with the
given DRC index. When the search fails, the cursor is left pointing to
&drmem_info->lmbs[drmem_info->n_lmbs], which is one element past the
last valid entry in the array. The debug message at the end of the
function then dereferences this pointer:

        pr_debug("Failed to hot-remove memory at %llx\n",
                 lmb->base_addr);

This was found by inspection and confirmed with KASAN:

  pseries-hotplug-mem: Attempting to hot-remove LMB, drc index 1234
  ==================================================================
  BUG: KASAN: slab-out-of-bounds in dlpar_memory+0x298/0x1658
  Read of size 8 at addr c000000364e97fd0 by task bash/949

  dump_stack_lvl+0xa4/0xfc (unreliable)
  print_report+0x214/0x63c
  kasan_report+0x140/0x2e0
  __asan_load8+0xa8/0xe0
  dlpar_memory+0x298/0x1658
  handle_dlpar_errorlog+0x130/0x1d0
  dlpar_store+0x18c/0x3e0
  kobj_attr_store+0x68/0xa0
  sysfs_kf_write+0xc4/0x110
  kernfs_fop_write_iter+0x26c/0x390
  vfs_write+0x2d4/0x4e0
  ksys_write+0xac/0x1a0
  system_call_exception+0x268/0x530
  system_call_vectored_common+0x15c/0x2ec

  Allocated by task 1:
   kasan_save_stack+0x48/0x80
   kasan_set_track+0x34/0x50
   kasan_save_alloc_info+0x34/0x50
   __kasan_kmalloc+0xd0/0x120
   __kmalloc+0x8c/0x320
   kmalloc_array.constprop.0+0x48/0x5c
   drmem_init+0x2a0/0x41c
   do_one_initcall+0xe0/0x5c0
   kernel_init_freeable+0x4ec/0x5a0
   kernel_init+0x30/0x1e0
   ret_from_kernel_user_thread+0x14/0x1c

  The buggy address belongs to the object at c000000364e80000
   which belongs to the cache kmalloc-128k of size 131072
  The buggy address is located 0 bytes to the right of
   allocated 98256-byte region [c000000364e80000, c000000364e97fd0)

  ==================================================================
  pseries-hotplug-mem: Failed to hot-remove memory at 0

Log failed lookups with a separate message and dereference the
cursor only when it points to a valid entry.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
Fixes: 51925fb3c5c9 ("powerpc/pseries: Implement memory hotplug remove in the kernel")
---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index a43bfb01720a..6f2eebae7bee 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -436,14 +436,15 @@ static int dlpar_memory_remove_by_index(u32 drc_index)
 		}
 	}
 
-	if (!lmb_found)
+	if (!lmb_found) {
+		pr_debug("Failed to look up LMB for drc index %x\n", drc_index);
 		rc = -EINVAL;
-
-	if (rc)
+	} else if (rc) {
 		pr_debug("Failed to hot-remove memory at %llx\n",
 			 lmb->base_addr);
-	else
+	} else {
 		pr_debug("Memory at %llx was hot-removed\n", lmb->base_addr);
+	}
 
 	return rc;
 }

-- 
2.41.0

[PATCH 2/3] powerpc/pseries/memhp: Remove unbalanced dlpar_release_drc() call

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathanl@linux.ibm.com>

Callers of dlpar_add_lmb() are responsible for first acquiring the DRC
and releasing it if dlpar_add_lmb() fails.

However, dlpar_add_lmb() performs a dlpar_release_drc() in one error
branch.  There is no corresponding dlpar_acquire_drc() in the
function, nor is there any stated justification. None of the other
error paths in dlpar_add_lmb() release the DRC.

This is a potential source of redundant attempts to release DRCs,
which is likely benign, but is confusing and inconsistent. Remove it.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 6f2eebae7bee..ba883c1b9f6d 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -575,7 +575,6 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
 
 	rc = update_lmb_associativity_index(lmb);
 	if (rc) {
-		dlpar_release_drc(lmb->drc_index);
 		return rc;
 	}
 

-- 
2.41.0

[PATCH 3/3] powerpc/pseries/memhp: Log more error conditions in add path

[Nathan Lynch via B4 Relay]

From: Nathan Lynch <nathanl@linux.ibm.com>

When an add operation for multiple LMBs fails, there is currently
little indication from the kernel of what went wrong. Be a little more
verbose about error conditions in the add paths.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index ba883c1b9f6d..4896920514f3 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -208,8 +208,10 @@ static int dlpar_change_lmb_state(struct drmem_lmb *lmb, bool online)
 	int rc;
 
 	mem_block = lmb_to_memblock(lmb);
-	if (!mem_block)
+	if (!mem_block) {
+		pr_err("Failed memory block lookup for LMB 0x%x\n", lmb->drc_index);
 		return -EINVAL;
+	}
 
 	if (online && mem_block->dev.offline)
 		rc = device_online(&mem_block->dev);
@@ -575,6 +577,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
 
 	rc = update_lmb_associativity_index(lmb);
 	if (rc) {
+		pr_err("Failed to configure LMB 0x%x\n", lmb->drc_index);
 		return rc;
 	}
 
@@ -588,12 +591,14 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
 	/* Add the memory */
 	rc = __add_memory(nid, lmb->base_addr, block_sz, MHP_MEMMAP_ON_MEMORY);
 	if (rc) {
+		pr_err("Failed to add LMB 0x%x to node %u", lmb->drc_index, nid);
 		invalidate_lmb_associativity_index(lmb);
 		return rc;
 	}
 
 	rc = dlpar_online_lmb(lmb);
 	if (rc) {
+		pr_err("Failed to online LMB 0x%x on node %u\n", lmb->drc_index, nid);
 		__remove_memory(lmb->base_addr, block_sz);
 		invalidate_lmb_associativity_index(lmb);
 	} else {

-- 
2.41.0

Re: [PATCH 2/3] powerpc/pseries/memhp: Remove unbalanced dlpar_release_drc() call

[Nick Child]

Hi Nathan,
Patches 1 and 3 LGTM

Regarding this patch, dlpar_memory_remove_by_count() calls 
dlpar_add_lmb() and does not free drc on add error.
dlpar_add_lmb() is called here in error recovery so probably
not a big deal.

This is all new code to me but it looks like if the requested
number of lmbs cannot be removed then it attempts to add back
the ones that were successfully removed. So if you cannot add
an lmb that WAS successfully removed, it seems sane to also
release the drc.


On 11/14/23 11:01, Nathan Lynch via B4 Relay wrote:
> From: Nathan Lynch <nathanl@linux.ibm.com>
> 
> Callers of dlpar_add_lmb() are responsible for first acquiring the DRC
> and releasing it if dlpar_add_lmb() fails.
> 
> However, dlpar_add_lmb() performs a dlpar_release_drc() in one error
> branch.  There is no corresponding dlpar_acquire_drc() in the
> function, nor is there any stated justification. None of the other
> error paths in dlpar_add_lmb() release the DRC.
> 
> This is a potential source of redundant attempts to release DRCs,
> which is likely benign, but is confusing and inconsistent. Remove it.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>   arch/powerpc/platforms/pseries/hotplug-memory.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 6f2eebae7bee..ba883c1b9f6d 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -575,7 +575,6 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
>   
>   	rc = update_lmb_associativity_index(lmb);
>   	if (rc) {
> -		dlpar_release_drc(lmb->drc_index);
>   		return rc;
>   	}
>   
> 

Re: [PATCH 2/3] powerpc/pseries/memhp: Remove unbalanced dlpar_release_drc() call

[Nathan Lynch]

Nick Child <nnac123@linux.ibm.com> writes:
> Hi Nathan,
> Patches 1 and 3 LGTM

thanks.

> Regarding this patch, dlpar_memory_remove_by_count() calls 
> dlpar_add_lmb() and does not free drc on add error.
> dlpar_add_lmb() is called here in error recovery so probably
> not a big deal.
>
> This is all new code to me but it looks like if the requested
> number of lmbs cannot be removed then it attempts to add back
> the ones that were successfully removed. So if you cannot add
> an lmb that WAS successfully removed, it seems sane to also
> release the drc.

Maybe I'll drop this one for now and turn my attention to removing all
the high-level rollback logic in this code. There's no reliable way to
accomplish what it's trying to do (i.e. restore the original quantity of
LMBs) and it just complicates things.


> On 11/14/23 11:01, Nathan Lynch via B4 Relay wrote:
>> From: Nathan Lynch <nathanl@linux.ibm.com>
>> 
>> Callers of dlpar_add_lmb() are responsible for first acquiring the DRC
>> and releasing it if dlpar_add_lmb() fails.
>> 
>> However, dlpar_add_lmb() performs a dlpar_release_drc() in one error
>> branch.  There is no corresponding dlpar_acquire_drc() in the
>> function, nor is there any stated justification. None of the other
>> error paths in dlpar_add_lmb() release the DRC.
>> 
>> This is a potential source of redundant attempts to release DRCs,
>> which is likely benign, but is confusing and inconsistent. Remove it.
>> 
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> ---
>>   arch/powerpc/platforms/pseries/hotplug-memory.c | 1 -
>>   1 file changed, 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index 6f2eebae7bee..ba883c1b9f6d 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -575,7 +575,6 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
>>   
>>   	rc = update_lmb_associativity_index(lmb);
>>   	if (rc) {
>> -		dlpar_release_drc(lmb->drc_index);
>>   		return rc;
>>   	}
>>   
>> 

Re: [PATCH 2/3] powerpc/pseries/memhp: Remove unbalanced dlpar_release_drc() call

[Scott Cheloha]

> On Nov 28, 2023, at 9:21 AM, Nathan Lynch <nathanl@linux.ibm.com> wrote:
> 
> Nick Child <nnac123@linux.ibm.com> writes:
>> Hi Nathan,
>> Patches 1 and 3 LGTM
> 
> thanks.
> 
>> Regarding this patch, dlpar_memory_remove_by_count() calls 
>> dlpar_add_lmb() and does not free drc on add error.
>> dlpar_add_lmb() is called here in error recovery so probably
>> not a big deal.
>> 
>> This is all new code to me but it looks like if the requested
>> number of lmbs cannot be removed then it attempts to add back
>> the ones that were successfully removed. So if you cannot add
>> an lmb that WAS successfully removed, it seems sane to also
>> release the drc.
> 
> Maybe I'll drop this one for now and turn my attention to removing all
> the high-level rollback logic in this code. There's no reliable way to
> accomplish what it's trying to do (i.e. restore the original quantity of
> LMBs) and it just complicates things.

Removing all of the rollback code is a wonderful idea.

Re: [PATCH 0/3] powerpc/pseries/memhp: Fix minor bugs and improve error logging

[Michael Ellerman]

On Tue, 14 Nov 2023 11:01:52 -0600, Nathan Lynch wrote:
> This includes a fix for an array bounds read overrun that can be
> triggered when debug messages are enabled.
> 

Patches 1 and 3 applied to powerpc/next.

[1/3] powerpc/pseries/memhp: Fix access beyond end of drmem array
      https://git.kernel.org/powerpc/c/bd68ffce69f6cf8ddd3a3c32549d1d2275e49fc5
[3/3] powerpc/pseries/memhp: Log more error conditions in add path
      https://git.kernel.org/powerpc/c/27951e1d8274e9f9a2925b069e4492939a3f2099

cheers