Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier

[Tyrel Datwyler <tyreld@linux.vnet.ibm.com>]

On 06/06/2019 10:04 PM, Nathan Lynch wrote:
> During post-migration device tree updates, we can oops in
> pseries_update_drconf_memory if the source device tree has an
> ibm,dynamic-memory-v2 property and the destination has a
> ibm,dynamic_memory (v1) property. The notifier processes an "update"
> for the ibm,dynamic-memory property but it's really an add in this
> scenario. So make sure the old property object is there before
> dereferencing it.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---

Ok, so I think there are a couple things to address here in regards to the nice
mess PFW has gotten us into.

Yes, this patch solves the oops, but I worry it just papers over some short
comings in our code.

After some poking around unless I'm mistaken our memory notifier only handles v1
versions of the ibm,dynamic-memory property. So, on newer firmware we aren't
doing any memory fixups if v2 (ibm,dynamic-memory-v2) of the property is updated.

For older PFW if we have source and target that only support v1 we will update
the memory in response to any update to ibm,dynamic-memory. It also appears to
be the case if we start with v1 and migrate to a target with newer PFW that
supports both v1 and v2 that the PFW will continue with v1 on the target and as
a result we update memory in accordance to a property update to ibm,dynamic-memory.

Now, if we have source and targets that both support v2 after a migration we
will do no update in response to ibm,dynamic-memory-v2 changes. And then there
is the case of a source with v2 support migrating to a target with only v1
support where we observe this oops. The oops is a result of ibm,dynamic-memory
being a new property that is added and there for no old property date exists.
However, simply returning in response also has the side effect that we do not
update memory in response to a device tree update of dynamic memory.

Maybe we are ok with this behavior as I haven't dug deep enough into the memory
subsystem here to really understand what the memory code is updating, but it is
concerning that we are doing it in some cases, but not all.

-Tyrel

>  arch/powerpc/platforms/pseries/hotplug-memory.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 47087832f8b2..e6bd172bcf30 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -980,6 +980,9 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr)
>  	if (!memblock_size)
>  		return -EINVAL;
> 
> +	if (!pr->old_prop)
> +		return 0;
> +
>  	p = (__be32 *) pr->old_prop->value;
>  	if (!p)
>  		return -EINVAL;
>