Re: [PULL 06/30] spapr: Do PHB hoplug sanity check at pre-plug

[Greg Kurz]

On Tue, 15 Dec 2020 16:56:36 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 14 Dec 2020 at 04:58, David Gibson <david@gibson.dropbear.id.au> 
> wrote:
> >
> > From: Greg Kurz <groug@kaod.org>
> >
> > We currently detect that a PHB index is already in use at plug time.
> > But this can be decteted at pre-plug in order to error out earlier.
> >
> > This allows to pass &error_abort to spapr_drc_attach() and to end
> > up with a plug handler that doesn't need to report errors anymore.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Message-Id: <20201120234208.683521-8-groug@kaod.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Hi; this change seems to have nudged one of Coverity's
> heuristics into deciding that spapr_drc_by_id() can return
> NULL (because its return value is checked here, I suspect),
> so it reports CID 1437757, 1437758, where spapr_add_lmbs()
> and spapr_memory_unplug_request() both take the return value
> of spapr_drc_by_id() and pass it directly to spapr_drc_index(),
> which will crash if it is passed a NULL pointer.
> 
> Is it impossible for spapr_drc_by_id() to return NULL in
> those functions (ie Coverity false positive) or is there
> a missing error check ?
> 

No, all DRC objects are created before any of these two
functions are called. Each function happens to loop over
the full list of memory DRCs a few line above the offending
call sites and already assert spapr_drc_by_id() doesn't
return NULL. But I guess Coverity isn't smart enough to
detect that.

I'll post a patch to add some more assertions.

> thanks
> -- PMM