Hello and thanks for the patch.
Libor Pechacek <lpecha...@suse.cz> writes:
> In KVM guests drmem structure is only zero initialized. Trying to
> manipulate DLPAR parameters results in a crash in this environment.
I think this statement needs qualification. Unless I'm mistaken, this
happens only when you boot a guest without any hotpluggable memory
configured, and then try to add or remove memory.
> diff --git a/arch/powerpc/include/asm/drmem.h
> b/arch/powerpc/include/asm/drmem.h
> index 3d76e1c388c2..28c3d936fdf3 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -27,12 +27,12 @@ struct drmem_lmb_info {
> extern struct drmem_lmb_info *drmem_info;
>
> #define for_each_drmem_lmb_in_range(lmb, start, end) \
> - for ((lmb) = (start); (lmb) <= (end); (lmb)++)
> + for ((lmb) = (start); (lmb) < (end); (lmb)++)
>
> #define for_each_drmem_lmb(lmb) \
> for_each_drmem_lmb_in_range((lmb), \
> &drmem_info->lmbs[0], \
> - &drmem_info->lmbs[drmem_info->n_lmbs - 1])
> + &drmem_info->lmbs[drmem_info->n_lmbs])
>
> /*
> * The of_drconf_cell_v1 struct defines the layout of the LMB data
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c
> b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c126b94d1943..4ea6af002e27 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -236,9 +236,9 @@ static int get_lmb_range(u32 drc_index, int n_lmbs,
> if (!start)
> return -EINVAL;
>
> - end = &start[n_lmbs - 1];
> + end = &start[n_lmbs];
>
> - last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs - 1];
> + last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs];
> if (end > last_lmb)
> return -EINVAL;
Is this not undefined behavior? I'd rather do this in a way that does
not involve forming out-of-bounds pointers. Even if it's safe, naming
that pointer "last_lmb" now actively hinders understanding of the code;
it should be named "limit" or something.
Anyway, I confess I've had the following patch sitting unsubmitted due
to a combination of perceived low urgency and lack of time. I've
verified it addresses your reported problem but I need a day or two to
check that it doesn't regress memory add/remove on suitably configured
guests:
================================================================
powerpc/drmem: make drmem list traversal safe on non-drmem systems
A user has reported a crash when attempting to remove an LMB on a KVM
guest where the device tree lacks the nodes and properties
(ibm,dynamic-reconfiguration-memory etc) which allow the drmem code to
support dynamic memory removal:
pseries-hotplug-mem: Attempting to hot-remove 1 LMB(s)
Unable to handle kernel paging request for data at address 0x00000010
Faulting instruction address: 0xc0000000000f0a30
Oops: Kernel access of bad area, sig: 11 [#1]
[... regs etc ...]
NIP [c0000000000f0a30] dlpar_memory+0x510/0xb50
LR [c0000000000f09e8] dlpar_memory+0x4c8/0xb50
Call Trace:
[c0000001edf97ba0] [c0000000000f09e8] dlpar_memory+0x4c8/0xb50 (unreliable)
[c0000001edf97c20] [c0000000000e8390] handle_dlpar_errorlog+0x60/0x140
[c0000001edf97c90] [c0000000000e85e0] dlpar_store+0x170/0x490
[c0000001edf97d40] [c000000000cb0c90] kobj_attr_store+0x30/0x50
[c0000001edf97d60] [c000000000591598] sysfs_kf_write+0x68/0xa0
[c0000001edf97d80] [c00000000058fec4] kernfs_fop_write+0x104/0x270
[c0000001edf97dd0] [c0000000004a1078] sys_write+0x128/0x380
[c0000001edf97e30] [c00000000000b388] system_call+0x5c/0x70
Make for_each_drmem_lmb() safe for the case where the list of drmem
LMBs is unpopulated.
Signed-off-by: Nathan Lynch <nath...@linux.ibm.com>
1 file changed, 36 insertions(+), 7 deletions(-)
arch/powerpc/include/asm/drmem.h | 43 +++++++++++++++++++++++++++++++++-------
modified arch/powerpc/include/asm/drmem.h
@@ -20,19 +20,48 @@ struct drmem_lmb {
struct drmem_lmb_info {
struct drmem_lmb *lmbs;
- int n_lmbs;
+ unsigned int n_lmbs;
u32 lmb_size;
};
extern struct drmem_lmb_info *drmem_info;
-#define for_each_drmem_lmb_in_range(lmb, start, end) \
- for ((lmb) = (start); (lmb) <= (end); (lmb)++)
+static inline bool drmem_present(void)
+{
+ return drmem_info->lmbs != NULL;
+}
+
+static inline struct drmem_lmb *drmem_lmb_index(unsigned int index)
+{
+ if (!drmem_present())
+ return NULL;
-#define for_each_drmem_lmb(lmb) \
- for_each_drmem_lmb_in_range((lmb), \
- &drmem_info->lmbs[0], \
- &drmem_info->lmbs[drmem_info->n_lmbs - 1])
+ if (WARN_ON(index >= drmem_info->n_lmbs))
+ return NULL;
+
+ return &drmem_info->lmbs[index];
+}
+
+static inline struct drmem_lmb *drmem_first_lmb(void)
+{
+ return drmem_lmb_index(0);
+}
+
+static inline struct drmem_lmb *drmem_last_lmb(void)
+{
+ if (!drmem_present())
+ return NULL;
+
+ return drmem_lmb_index(drmem_info->n_lmbs - 1);
+}
+
+#define for_each_drmem_lmb(lmb)
\
+ for ((lmb) = drmem_first_lmb(); \
+ (lmb) != NULL && (lmb) <= drmem_last_lmb(); \
+ (lmb)++)
+
+#define for_each_drmem_lmb_in_range(lmb, start, end) \
+ for ((lmb) = (start); (lmb) <= (end); (lmb)++)
/*
* The of_drconf_cell_v1 struct defines the layout of the LMB data
