When a virtuser device is created via configfs and the probe fails due
to an incorrect lookup table, the table is not removed. This prevents
subsequent probe attempts from succeeding, even if the issue is
corrected, unless the device is released. Additionally, cleanup is also
needed in the less likely case of platform_device_register_full()
failure.

Besides, a consistent memory leak in lookup_table->dev_id was spotted
using kmemleak by toggling the live state between 0 and 1 with a correct
lookup table.

Introduce gpio_virtuser_remove_lookup_table() as the counterpart to the
existing gpio_virtuser_make_lookup_table() and call it from all
necessary points to ensure proper cleanup.

Fixes: 91581c4b3f29 ("gpio: virtuser: new virtual testing driver for the GPIO API")
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
 drivers/gpio/gpio-virtuser.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
index 91b6352c957c..e89b1239b635 100644
--- a/drivers/gpio/gpio-virtuser.c
+++ b/drivers/gpio/gpio-virtuser.c
@@ -1439,6 +1439,15 @@ gpio_virtuser_make_lookup_table(struct gpio_virtuser_device *dev)
 	return 0;
 }
 
+static void
+gpio_virtuser_remove_lookup_table(struct gpio_virtuser_device *dev)
+{
+	gpiod_remove_lookup_table(dev->lookup_table);
+	kfree(dev->lookup_table->dev_id);
+	kfree(dev->lookup_table);
+	dev->lookup_table = NULL;
+}
+
 static struct fwnode_handle *
 gpio_virtuser_make_device_swnode(struct gpio_virtuser_device *dev)
 {
@@ -1487,10 +1496,8 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev)
 	pdevinfo.fwnode = swnode;
 
 	ret = gpio_virtuser_make_lookup_table(dev);
-	if (ret) {
-		fwnode_remove_software_node(swnode);
-		return ret;
-	}
+	if (ret)
+		goto err_remove_swnode;
 
 	reinit_completion(&dev->probe_completion);
 	dev->driver_bound = false;
@@ -1498,23 +1505,31 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev)
 
 	pdev = platform_device_register_full(&pdevinfo);
 	if (IS_ERR(pdev)) {
+		ret = PTR_ERR(pdev);
 		bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
-		fwnode_remove_software_node(swnode);
-		return PTR_ERR(pdev);
+		goto err_remove_lookup_table;
 	}
 
 	wait_for_completion(&dev->probe_completion);
 	bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
 
 	if (!dev->driver_bound) {
-		platform_device_unregister(pdev);
-		fwnode_remove_software_node(swnode);
-		return -ENXIO;
+		ret = -ENXIO;
+		goto err_unregister_pdev;
 	}
 
 	dev->pdev = pdev;
 
 	return 0;
+
+err_unregister_pdev:
+	platform_device_unregister(pdev);
+err_remove_lookup_table:
+	gpio_virtuser_remove_lookup_table(dev);
+err_remove_swnode:
+	fwnode_remove_software_node(swnode);
+
+	return ret;
 }
 
 static void
@@ -1526,10 +1541,9 @@ gpio_virtuser_device_deactivate(struct gpio_virtuser_device *dev)
 
 	swnode = dev_fwnode(&dev->pdev->dev);
 	platform_device_unregister(dev->pdev);
+	gpio_virtuser_remove_lookup_table(dev);
 	fwnode_remove_software_node(swnode);
 	dev->pdev = NULL;
-	gpiod_remove_lookup_table(dev->lookup_table);
-	kfree(dev->lookup_table);
 }
 
 static ssize_t
-- 
2.43.0

On Fri, Jan 3, 2025 at 3:18 PM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> When a virtuser device is created via configfs and the probe fails due
> to an incorrect lookup table, the table is not removed. This prevents
> subsequent probe attempts from succeeding, even if the issue is
> corrected, unless the device is released. Additionally, cleanup is also
> needed in the less likely case of platform_device_register_full()
> failure.
>
> Besides, a consistent memory leak in lookup_table->dev_id was spotted
> using kmemleak by toggling the live state between 0 and 1 with a correct
> lookup table.
>
> Introduce gpio_virtuser_remove_lookup_table() as the counterpart to the
> existing gpio_virtuser_make_lookup_table() and call it from all
> necessary points to ensure proper cleanup.
>
> Fixes: 91581c4b3f29 ("gpio: virtuser: new virtual testing driver for the GPIO API")
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
>  drivers/gpio/gpio-virtuser.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
> index 91b6352c957c..e89b1239b635 100644
> --- a/drivers/gpio/gpio-virtuser.c
> +++ b/drivers/gpio/gpio-virtuser.c
> @@ -1439,6 +1439,15 @@ gpio_virtuser_make_lookup_table(struct gpio_virtuser_device *dev)
>         return 0;
>  }
>
> +static void
> +gpio_virtuser_remove_lookup_table(struct gpio_virtuser_device *dev)
> +{
> +       gpiod_remove_lookup_table(dev->lookup_table);
> +       kfree(dev->lookup_table->dev_id);

Ah, this has been here all along as well. :/


> +       kfree(dev->lookup_table);
> +       dev->lookup_table = NULL;
> +}
> +
>  static struct fwnode_handle *
>  gpio_virtuser_make_device_swnode(struct gpio_virtuser_device *dev)
>  {
> @@ -1487,10 +1496,8 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev)
>         pdevinfo.fwnode = swnode;
>
>         ret = gpio_virtuser_make_lookup_table(dev);
> -       if (ret) {
> -               fwnode_remove_software_node(swnode);
> -               return ret;
> -       }
> +       if (ret)
> +               goto err_remove_swnode;
>
>         reinit_completion(&dev->probe_completion);
>         dev->driver_bound = false;
> @@ -1498,23 +1505,31 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev)
>
>         pdev = platform_device_register_full(&pdevinfo);
>         if (IS_ERR(pdev)) {
> +               ret = PTR_ERR(pdev);
>                 bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
> -               fwnode_remove_software_node(swnode);
> -               return PTR_ERR(pdev);
> +               goto err_remove_lookup_table;
>         }
>
>         wait_for_completion(&dev->probe_completion);
>         bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
>
>         if (!dev->driver_bound) {
> -               platform_device_unregister(pdev);
> -               fwnode_remove_software_node(swnode);
> -               return -ENXIO;
> +               ret = -ENXIO;
> +               goto err_unregister_pdev;
>         }
>
>         dev->pdev = pdev;
>
>         return 0;
> +
> +err_unregister_pdev:
> +       platform_device_unregister(pdev);
> +err_remove_lookup_table:
> +       gpio_virtuser_remove_lookup_table(dev);
> +err_remove_swnode:
> +       fwnode_remove_software_node(swnode);
> +
> +       return ret;
>  }
>
>  static void
> @@ -1526,10 +1541,9 @@ gpio_virtuser_device_deactivate(struct gpio_virtuser_device *dev)
>
>         swnode = dev_fwnode(&dev->pdev->dev);
>         platform_device_unregister(dev->pdev);
> +       gpio_virtuser_remove_lookup_table(dev);

Any reason for moving it earlier?

Bart

>         fwnode_remove_software_node(swnode);
>         dev->pdev = NULL;
> -       gpiod_remove_lookup_table(dev->lookup_table);
> -       kfree(dev->lookup_table);
>  }
>
>  static ssize_t
> --
> 2.43.0
>

On Fri, Jan 03, 2025 at 03:38:46PM +0100, Bartosz Golaszewski wrote:
> On Fri, Jan 3, 2025 at 3:18 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > When a virtuser device is created via configfs and the probe fails due
> > to an incorrect lookup table, the table is not removed. This prevents
> > subsequent probe attempts from succeeding, even if the issue is
> > corrected, unless the device is released. Additionally, cleanup is also
> > needed in the less likely case of platform_device_register_full()
> > failure.
> >
> > Besides, a consistent memory leak in lookup_table->dev_id was spotted
> > using kmemleak by toggling the live state between 0 and 1 with a correct
> > lookup table.
> >
> > Introduce gpio_virtuser_remove_lookup_table() as the counterpart to the
> > existing gpio_virtuser_make_lookup_table() and call it from all
> > necessary points to ensure proper cleanup.
> >
> > Fixes: 91581c4b3f29 ("gpio: virtuser: new virtual testing driver for the GPIO API")
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
> >  drivers/gpio/gpio-virtuser.c | 36 +++++++++++++++++++++++++-----------
> >  1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
> > index 91b6352c957c..e89b1239b635 100644
> > --- a/drivers/gpio/gpio-virtuser.c
> > +++ b/drivers/gpio/gpio-virtuser.c
> > @@ -1439,6 +1439,15 @@ gpio_virtuser_make_lookup_table(struct gpio_virtuser_device *dev)
> >         return 0;
> >  }
> >
> > +static void
> > +gpio_virtuser_remove_lookup_table(struct gpio_virtuser_device *dev)
> > +{
> > +       gpiod_remove_lookup_table(dev->lookup_table);
> > +       kfree(dev->lookup_table->dev_id);
> 
> Ah, this has been here all along as well. :/
> 
> 
> > +       kfree(dev->lookup_table);
> > +       dev->lookup_table = NULL;
> > +}
> > +
> >  static struct fwnode_handle *
> >  gpio_virtuser_make_device_swnode(struct gpio_virtuser_device *dev)
> >  {
> > @@ -1487,10 +1496,8 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev)
> >         pdevinfo.fwnode = swnode;
> >
> >         ret = gpio_virtuser_make_lookup_table(dev);
> > -       if (ret) {
> > -               fwnode_remove_software_node(swnode);
> > -               return ret;
> > -       }
> > +       if (ret)
> > +               goto err_remove_swnode;
> >
> >         reinit_completion(&dev->probe_completion);
> >         dev->driver_bound = false;
> > @@ -1498,23 +1505,31 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev)
> >
> >         pdev = platform_device_register_full(&pdevinfo);
> >         if (IS_ERR(pdev)) {
> > +               ret = PTR_ERR(pdev);
> >                 bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
> > -               fwnode_remove_software_node(swnode);
> > -               return PTR_ERR(pdev);
> > +               goto err_remove_lookup_table;
> >         }
> >
> >         wait_for_completion(&dev->probe_completion);
> >         bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
> >
> >         if (!dev->driver_bound) {
> > -               platform_device_unregister(pdev);
> > -               fwnode_remove_software_node(swnode);
> > -               return -ENXIO;
> > +               ret = -ENXIO;
> > +               goto err_unregister_pdev;
> >         }
> >
> >         dev->pdev = pdev;
> >
> >         return 0;
> > +
> > +err_unregister_pdev:
> > +       platform_device_unregister(pdev);
> > +err_remove_lookup_table:
> > +       gpio_virtuser_remove_lookup_table(dev);
> > +err_remove_swnode:
> > +       fwnode_remove_software_node(swnode);
> > +
> > +       return ret;
> >  }
> >
> >  static void
> > @@ -1526,10 +1541,9 @@ gpio_virtuser_device_deactivate(struct gpio_virtuser_device *dev)
> >
> >         swnode = dev_fwnode(&dev->pdev->dev);
> >         platform_device_unregister(dev->pdev);
> > +       gpio_virtuser_remove_lookup_table(dev);
> 
> Any reason for moving it earlier?

Just to make the resources teardown order more natural and better align
with the error path in gpio_virtuser_device_activate() (i.e., the new goto
labels part I added). IIUC, it can be safely moved as such under dev->lock.

Thanks.

-Koichiro

> 
> Bart
> 
> >         fwnode_remove_software_node(swnode);
> >         dev->pdev = NULL;
> > -       gpiod_remove_lookup_table(dev->lookup_table);
> > -       kfree(dev->lookup_table);
> >  }
> >
> >  static ssize_t
> > --
> > 2.43.0
> >