Re: [PATCH v2] hw/core/qdev-clock: add a reference on aliased clocks

[Luc Michel]

On 15:23 Fri 23 Oct     , Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 20, 2020 at 1:11 PM Luc Michel <luc@lmichel.fr> wrote:
> 
> > When aliasing a clock with the qdev_alias_clock() function, a new link
> > property is created on the device aliasing the clock. The link points
> > to the aliased clock and use the OBJ_PROP_LINK_STRONG flag. This
> > property is read only since it does not provide a check callback for
> > modifications.
> >
> > The object_property_add_link() documentation stats that with
> > OBJ_PROP_LINK_STRONG properties, the linked object reference count get
> > decremented when the property is deleted. But it is _not_ incremented on
> > creation (object_property_add_link() does not actually know the link).
> >
> > This commit increments the reference count on the aliased clock to
> > ensure the aliased clock stays alive during the property lifetime, and
> > to avoid a double-free memory error when the property gets deleted.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: Luc Michel <luc@lmichel.fr>
> > ---
> >
> 
> In principle, that makes sense. But I don't see any users of that API yet.
Yes, Peter encountered a double-free error because of this missing
object_ref when he applied my Raspberry PI CPRMAN series, which makes
use of qdev_alias_clock.

Peter: do you consider taking this patch to be able to pick up the
CPRMAN series again?

> 
> It would have been nice to have some unit tests for qdev-clock.h..
Yes I agree.

-- 
Luc

> 
>  hw/core/qdev-clock.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> > index 6a9a340d0f..eb05f2a13c 100644
> > --- a/hw/core/qdev-clock.c
> > +++ b/hw/core/qdev-clock.c
> > @@ -59,10 +59,18 @@ static NamedClockList *qdev_init_clocklist(DeviceState
> > *dev, const char *name,
> >      } else {
> >          object_property_add_link(OBJECT(dev), name,
> >                                   object_get_typename(OBJECT(clk)),
> >                                   (Object **) &ncl->clock,
> >                                   NULL, OBJ_PROP_LINK_STRONG);
> > +        /*
> > +         * Since the link property has the OBJ_PROP_LINK_STRONG flag, the
> > clk
> > +         * object reference count gets decremented on property deletion.
> > +         * However object_property_add_link does not increment it since it
> > +         * doesn't know the linked object. Increment it here to ensure the
> > +         * aliased clock stays alive during this device life-time.
> > +         */
> > +        object_ref(OBJECT(clk));
> >      }
> >
> >      ncl->clock = clk;
> >
> >      QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
> > --
> > 2.28.0
> >
> >
> >
> 
> -- 
> Marc-André Lureau

--