On Monday, February 22, 2016 05:04:04 PM Shevchenko, Andriy wrote:
> On Mon, 2016-02-22 at 17:40 +0200, Andy Shevchenko wrote:
> > On Mon, 2016-02-22 at 16:50 +0200, Heikki Krogerus wrote:
> > > In device_remove_property_set(), if the primary fwnode is
> > > of type "pset", it has to be set pointing to NULL before
> > > calling set_secondary_fwnode(). Otherwise
> > > set_secondary_fwnode() will attempt to set the
> > > fwnode->secondary member after the fwnode has been freed.
> > >
> > > Reported-by: John Youn <john.y...@synopsys.com>
> > > Signed-off-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
> > > ---
> > > drivers/base/property.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > index a163f2c..ddf2987 100644
> > > --- a/drivers/base/property.c
> > > +++ b/drivers/base/property.c
> > > @@ -820,7 +820,9 @@ void device_remove_property_set(struct device
> > > *dev)
> > > * the pset. If there is no real firmware node (ACPI/DT)
> > > primary
> > > * will hold the pset.
> > > */
> > > - if (!is_pset_node(fwnode))
> > > + if (is_pset_node(fwnode))
> > > + dev->fwnode = NULL;
> > > + else
> > > fwnode = fwnode->secondary;
> > > if (!IS_ERR(fwnode) && is_pset_node(fwnode))
> > > pset_free_set(to_pset_node(fwnode));
> >
> >
> > What if we do the following
> >
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -818,9 +818,13 @@ void device_remove_property_set(struct device
> > *dev)
> > */
> > if (!is_pset_node(fwnode))
> > fwnode = fwnode->secondary;
> > +
> > + /* Set device fwnode to NULL before we free it */
> > + set_secondary_fwnode(dev, NULL);
> > +
> > + /* Free property set for the given device */
> > if (!IS_ERR(fwnode) && is_pset_node(fwnode))
> > pset_free_set(to_pset_node(fwnode));
> > - set_secondary_fwnode(dev, NULL);
> > }
> > EXPORT_SYMBOL_GPL(device_remove_property_set);
> >
> > ?
> >
>
> Just noticed that there is another potential bug is hidden, if we call
> this function on non-pset fwnode we will silently get fwnode set to
> NULL.
>
> Considering this, perhaps better solution is to convert last lines to
>
> if (!IS_ERR(fwnode) && is_pset_node(fwnode)) {
> set_secondary_fwnode(dev, NULL);
> pset_free_set(to_pset_node(fwnode));
> }
>
> I didn't check if we do serialize access to fwnode. It might be more
> bugs with access to it in racing manner.
The core doesn't serialize those.
If the callers need serialization, they should use some locking around
those calls.
Thanks,
Rafael
