Re: [Qemu-devel] [FIX PATCH v1] spapr: Fix QEMU abort during memory unplug

[David Gibson]

On Thu, Jul 20, 2017 at 09:41:19AM +0530, Bharata B Rao wrote:
> Commit 0cffce56 (hw/ppc/spapr.c: adding pending_dimm_unplugs to
> sPAPRMachineState) introduced a new way to track pending LMBs of DIMM
> device that is marked for removal. Since this commit we can hit the
> assert in spapr_pending_dimm_unplugs_add() in the following situation:
> 
> - DIMM device removal fails as the guest doesn't allow the removal.
> - Subsequent attempt to remove the same DIMM would hit the assert
>   as the corresponding sPAPRDIMMState is still part of the
>   pending_dimm_unplugs list.
> 
> Fix this by removing the assert and conditionally adding the
> sPAPRDIMMState to pending_dimm_unplugs list only when it is not
> already present.
> 
> Fixes: 0cffce56ae3501c5783d779f97993ce478acf856
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
> Changes in v1:
> - Added comment (David Gibson)
> - Ensured we free sPAPRDIMMState when corresonding entry already
>   exists (Daniel Henrique Barboza)
> 
> Daniel had shown another alternative, we can switch over to that
> if preferred.
> 
>  hw/ppc/spapr.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1cb09e7..c6091e2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2853,8 +2853,17 @@ static sPAPRDIMMState 
> *spapr_pending_dimm_unplugs_find(sPAPRMachineState *s,
>  static void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
>                                             sPAPRDIMMState *dimm_state)
>  {
> -    g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->dimm));
> -    QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
> +    /*
> +     * If this request is for a DIMM whose removal had failed earlier
> +     * (due to guest's refusal to remove the LMBs), we would have this
> +     * dimm_state already in the pending_dimm_unplugs list. In that
> +     * case don't add again.
> +     */
> +    if (!spapr_pending_dimm_unplugs_find(spapr, dimm_state->dimm)) {
> +        QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
> +    } else {
> +        g_free(dimm_state);

This is dangerous.  You're freeing a pointer passed in by the caller
under conditions that the caller can't know, so it can't know if it
has a valid pointer afterwards or not.

It so happens that we don't use the pointer again from the caller in
spapr_memory_unplug_request() and I suspect this situation can't
occur for the call in spapr_recover_pending_dimm_state().  Still,
that's way more subtle that I'm comfortable with.

I think the way to handle this is to change unplugs_add() so that it
takes the relevant fields of the DIMMState rather than a pre-allocated
structure.  It will then return either an existing state if available
or a newly allocated and indexed one otherwise.

> +    }
>  }
>  
>  static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature