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 <bhar...@linux.vnet.ibm.com>
> ---
> 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
