On Mon, Oct 09, 2017 at 04:28:37PM +0300, Jarkko Nikula wrote:
> Deletion of subdevice will remove device properties associated to parent
> when they share the same firmware node after commit 478573c93abd ("driver
> core: Don't leak secondary fwnode on device removal"). This was observed
> with a driver adding subdevice that driver wasn't able to read device
> properties after rmmod/modprobe cycle.
>
> Consider the lifecycle of it:
>
> parent device registration
> ACPI_COMPANION_SET()
> device_add_properties()
> pset_copy_set()
> set_secondary_fwnode(dev, &p->fwnode)
> device_add()
>
> parent probe
> read device properties
> ACPI_COMPANION_SET(subdevice, ACPI_COMPANION(parent))
> device_add(subdevice)
>
> parent remove
> device_del(subdevice)
> device_remove_properties()
> set_secondary_fwnode(dev, NULL);
> pset_free()
>
> Parent device will have its primary firmware node pointing to an ACPI node
> and secondary firmware node point to device properties.
> ACPI_COMPANION_SET() call in parent probe will set the subdevice's firmware
> node to point to the same 'struct fwnode_handle' and the associated
> secondary firmware node, i.e. the device properties as the parent.
>
> When subdevice is deleted in parent remove that will remove those device
> properties and attempt to read device properties in next parent probe call
> will fail.
>
> Fix this by tracking the owner device of device properties and delete them
> only when owner device is being deleted.
>
> Fixes: 478573c93abd ("driver core: Don't leak secondary fwnode on device removal")
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.9+
> Signed-off-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> ---
> drivers/base/property.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index d0b65bbe7e15..21fcc13013a5 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -21,6 +21,7 @@
> #include <linux/phy.h>
>
> struct property_set {
> + struct device *dev;
> struct fwnode_handle fwnode;
> const struct property_entry *properties;
> };
> @@ -891,6 +892,7 @@ static struct property_set *pset_copy_set(const struct property_set *pset)
> void device_remove_properties(struct device *dev)
> {
> struct fwnode_handle *fwnode;
> + struct property_set *pset;
>
> fwnode = dev_fwnode(dev);
> if (!fwnode)
> @@ -900,16 +902,16 @@ void device_remove_properties(struct device *dev)
> * the pset. If there is no real firmware node (ACPI/DT) primary
> * will hold the pset.
> */
> - if (is_pset_node(fwnode)) {
> + pset = to_pset_node(fwnode);
> + if (pset) {
> set_primary_fwnode(dev, NULL);
> - pset_free_set(to_pset_node(fwnode));
> } else {
> - fwnode = fwnode->secondary;
> - if (!IS_ERR(fwnode) && is_pset_node(fwnode)) {
> + pset = to_pset_node(fwnode->secondary);
> + if (pset && dev == pset->dev)
> set_secondary_fwnode(dev, NULL);
> - pset_free_set(to_pset_node(fwnode));
> - }
> }
> + if (pset && dev == pset->dev)
> + pset_free_set(pset);
> }
> EXPORT_SYMBOL_GPL(device_remove_properties);
>
> @@ -938,6 +940,7 @@ int device_add_properties(struct device *dev,
>
> p->fwnode.ops = &pset_fwnode_ops;
> set_secondary_fwnode(dev, &p->fwnode);
> + p->dev = dev;
Don't you also need to increment the reference counter here? Or how is
it assured that it will not go away?
thanks,
greg k-h