nvmem: core: fix leaks on registration errors

PATCH In review Johan Hovold 3 participants 5 messages May 16, 2017 – Jun 7, 2017

Subsystems (2)

Stable branch NVMEM framework

lore

Share & tools

lore

Johan Hovold

author

2017-05-16 13:45 UTC #0 lore raw

PATCH 1/1 nvmem: core: fix leaks on registration errors

Make sure to deregister and release the nvmem device and underlying memory on registration errors.

Note that the private data must be freed using put_device() once the struct device has been initialised.

Also note that there's a related reference leak in the deregistration function as reported by Mika Westerberg which is being fixed separately.

Fixes b6c217ab9be6 ("nvmem: Add backwards compatibility support for older EEPROM drivers.")

Fixes eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers")

Cc stable <[email protected]> # 4.3

Cc Andrew Lunn <[email protected]>

Cc Srinivas Kandagatla <[email protected]>

Cc Mika Westerberg <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/nvmem/core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/nvmem/core.c +8 -5

index 8c830a80a648..6cf916d9db6d 100644
@@ -489,21 +489,24 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 	rval = device_add(&nvmem->dev);
 	if (rval)
-		goto out;
+		goto err_put_device;
 
 	if (config->compat) {
 		rval = nvmem_setup_compat(nvmem, config);
 		if (rval)
-			goto out;
+			goto err_device_del;
 	}
 
 	if (config->cells)
 		nvmem_add_cells(nvmem, config);
 
 	return nvmem;
-out:
-	ida_simple_remove(&nvmem_ida, nvmem->id);
-	kfree(nvmem);
+
+err_device_del:
+	device_del(&nvmem->dev);
+err_put_device:
+	put_device(&nvmem->dev);
+
 	return ERR_PTR(rval);
 }
 EXPORT_SYMBOL_GPL(nvmem_register);

Signature · 1 line

2.13.0

Johan Hovold

author

2017-05-17 08:51 UTC #2 lore raw

Quoted 19 lines · Andrey Smirnov

Quoted 9 lines · Johan Hovold
Make sure to deregister and release the nvmem device and underlying memory on registration errors.
Note that the private data must be freed using put_device() once the struct device has been initialised.
Also note that there's a related reference leak in the deregistration function as reported by Mika Westerberg which is being fixed separately.
Quoted 8 lines
Fixes b6c217ab9be6 ("nvmem: Add backwards compatibility support for older EEPROM drivers.")
Fixes eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers")
Cc stable <[email protected]> # 4.3
Cc Andrew Lunn <[email protected]>
Cc Srinivas Kandagatla <[email protected]>
Cc Mika Westerberg <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---

Quoted 3 lines

I submitted identical patch to fix this about a month ago here:
lkml.kernel.org/r/[email protected]

Oh, and you fixed up both the registration and deregistration leaks in your series too. Looks like the maintainer is on vacation now so perhaps we'll see a third independent fix for these bugs soon. ;)

As for your series, I prefer this version for nvmem_register due to the difference in error label naming (named after what they do rather than where they are used), but as I also mentioned to Mika, I think just adding the missing put_device to nvmem_unregister as you did is preferred.

But the important thing is that this gets fixed.

Quoted 3 lines

so, FWIW,
Acked-by Andrey Smirnov <[email protected]>

Thanks,
Johan

Quoted 40 lines

 drivers/nvmem/core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/nvmem/core.c +8 -6

index 8c830a80a648..6cf916d9db6d 100644
@@ -489,21 +489,24 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
        rval = device_add(&nvmem->dev);
        if (rval)
-               goto out;
+               goto err_put_device;
 
        if (config->compat) {
                rval = nvmem_setup_compat(nvmem, config);
                if (rval)
-                       goto out;
+                       goto err_device_del;
        }
 
        if (config->cells)
                nvmem_add_cells(nvmem, config);
 
        return nvmem;
-out:
-       ida_simple_remove(&nvmem_ida, nvmem->id);
-       kfree(nvmem);
+
+err_device_del:
+       device_del(&nvmem->dev);
+err_put_device:
+       put_device(&nvmem->dev);
+
        return ERR_PTR(rval);
 }
 EXPORT_SYMBOL_GPL(nvmem_register);
--

2.13.0

Andrey Smirnov

2017-05-17 13:06 UTC #3 lore raw

Quoted 20 lines · Johan Hovold

Quoted 19 lines · Andrey Smirnov
Quoted 17 lines · Johan Hovold
Make sure to deregister and release the nvmem device and underlying memory on registration errors.
Note that the private data must be freed using put_device() once the struct device has been initialised.
Also note that there's a related reference leak in the deregistration function as reported by Mika Westerberg which is being fixed separately.
Fixes b6c217ab9be6 ("nvmem: Add backwards compatibility support for older EEPROM drivers.")
Fixes eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers")
Cc stable <[email protected]> # 4.3
Cc Andrew Lunn <[email protected]>
Cc Srinivas Kandagatla <[email protected]>
Cc Mika Westerberg <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---

Quoted 8 lines

Quoted 3 lines
I submitted identical patch to fix this about a month ago here:
lkml.kernel.org/r/[email protected]
Oh, and you fixed up both the registration and deregistration leaks in your series too. Looks like the maintainer is on vacation now so perhaps we'll see a third independent fix for these bugs soon. ;)

Heh, let's hope not :-)

Quoted 8 lines

As for your series, I prefer this version for nvmem_register due to the difference in error label naming (named after what they do rather than where they are used), but as I also mentioned to Mika, I think just adding the missing put_device to nvmem_unregister as you did is preferred.
But the important thing is that this gets fixed.

Yup, agreed, as long a version of both fixes ends up being applied it's all good.

Thanks,
Andrey

Srinivas Kandagatla

2017-06-07 14:57 UTC #4 lore raw

In reply to Johan Hovold

Quoted 16 lines · Johan Hovold

Make sure to deregister and release the nvmem device and underlying memory on registration errors.
Note that the private data must be freed using put_device() once the struct device has been initialised.
Also note that there's a related reference leak in the deregistration function as reported by Mika Westerberg which is being fixed separately.
Fixes b6c217ab9be6 ("nvmem: Add backwards compatibility support for older EEPROM drivers.")
Fixes eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers")
Cc stable <[email protected]> # 4.3
Cc Andrew Lunn <[email protected]>
Cc Srinivas Kandagatla <[email protected]>
Cc Mika Westerberg <[email protected]>
Signed-off-by Johan Hovold <[email protected]>

Quoted 3 lines

---
 drivers/nvmem/core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Thanks for the patch,
Will queue this one along with the "[PATCH 2/2] nvmem: core: Call put_device() in nvmem_unregister()" patch from Andrey Smirnov

thanks
srini

Quoted 36 lines

drivers/nvmem/core.c +8 -5

index 8c830a80a648..6cf916d9db6d 100644
@@ -489,21 +489,24 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 	rval = device_add(&nvmem->dev);
 	if (rval)
-		goto out;
+		goto err_put_device;
 
 	if (config->compat) {
 		rval = nvmem_setup_compat(nvmem, config);
 		if (rval)
-			goto out;
+			goto err_device_del;
 	}
 
 	if (config->cells)
 		nvmem_add_cells(nvmem, config);
 
 	return nvmem;
-out:
-	ida_simple_remove(&nvmem_ida, nvmem->id);
-	kfree(nvmem);
+
+err_device_del:
+	device_del(&nvmem->dev);
+err_put_device:
+	put_device(&nvmem->dev);
+
 	return ERR_PTR(rval);
 }
 EXPORT_SYMBOL_GPL(nvmem_register);