1. Patchew
  2. linux
  3. View series

[PATCH] iio: core: fix memleak in iio_device_register_sysfs

Dinghao Liu posted 1 patch 2 years ago 
drivers/iio/industrialio-core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
[PATCH] iio: core: fix memleak in iio_device_register_sysfs
Posted by Dinghao Liu 2 years ago 
When iio_device_register_sysfs_group() fails, we should
free iio_dev_opaque->chan_attr_group.attrs to prevent
potential memleak.

Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---
 drivers/iio/industrialio-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c77745b594bd..e6d3d07a4c83 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1581,10 +1581,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 	ret = iio_device_register_sysfs_group(indio_dev,
 					      &iio_dev_opaque->chan_attr_group);
 	if (ret)
-		goto error_clear_attrs;
+		goto error_free_chan_attrs;
 
 	return 0;
 
+error_free_chan_attrs:
+	kfree(iio_dev_opaque->chan_attr_group.attrs);
+	iio_dev_opaque->chan_attr_group.attrs = NULL;
 error_clear_attrs:
 	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
 
-- 
2.17.1
Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs
Posted by Jonathan Cameron 2 years ago 
On Fri,  8 Dec 2023 15:31:19 +0800
Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:

> When iio_device_register_sysfs_group() fails, we should
> free iio_dev_opaque->chan_attr_group.attrs to prevent
> potential memleak.
> 
> Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
Hi.

Looks good to me, but I'd like to leave this one on the list a little
longer to see if anyone else has comments.

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c77745b594bd..e6d3d07a4c83 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1581,10 +1581,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>  	ret = iio_device_register_sysfs_group(indio_dev,
>  					      &iio_dev_opaque->chan_attr_group);
>  	if (ret)
> -		goto error_clear_attrs;
> +		goto error_free_chan_attrs;
>  
>  	return 0;
>  
> +error_free_chan_attrs:
> +	kfree(iio_dev_opaque->chan_attr_group.attrs);
> +	iio_dev_opaque->chan_attr_group.attrs = NULL;
>  error_clear_attrs:
>  	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
>
Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs
Posted by Jonathan Cameron 1 year, 12 months ago 
On Sun, 10 Dec 2023 13:32:28 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri,  8 Dec 2023 15:31:19 +0800
> Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> 
> > When iio_device_register_sysfs_group() fails, we should
> > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > potential memleak.
> > 
> > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>  
> Hi.
> 
> Looks good to me, but I'd like to leave this one on the list a little
> longer to see if anyone else has comments.
> 
Guess no comments!

Applied to the fixes-togreg branch of iio.git.  I might not get another
fixes request out far enough in advance of the merge window in which case
this will go in around then instead.

Also marked for stable

Jonathan

> Jonathan
> 
> > ---
> >  drivers/iio/industrialio-core.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index c77745b594bd..e6d3d07a4c83 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1581,10 +1581,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
> >  	ret = iio_device_register_sysfs_group(indio_dev,
> >  					      &iio_dev_opaque->chan_attr_group);
> >  	if (ret)
> > -		goto error_clear_attrs;
> > +		goto error_free_chan_attrs;
> >  
> >  	return 0;
> >  
> > +error_free_chan_attrs:
> > +	kfree(iio_dev_opaque->chan_attr_group.attrs);
> > +	iio_dev_opaque->chan_attr_group.attrs = NULL;
> >  error_clear_attrs:
> >  	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
> >    
> 
>
Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs
Posted by andy.shevchenko@gmail.com 1 year, 10 months ago 
Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> On Sun, 10 Dec 2023 13:32:28 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri,  8 Dec 2023 15:31:19 +0800
> > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > 
> > > When iio_device_register_sysfs_group() fails, we should
> > > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > > potential memleak.
> > > 
> > > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>  
> > Hi.
> > 
> > Looks good to me, but I'd like to leave this one on the list a little
> > longer to see if anyone else has comments.
> > 
> Guess no comments!

This patch does not fix anything.

Yet, it might be considered as one that increases robustness, but with this applied the 
goto
https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/iio/industrialio-core.c#L2007
can be amended, right?

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs
Posted by Jonathan Cameron 1 year, 10 months ago 
On Sun, 11 Feb 2024 21:16:32 +0200
andy.shevchenko@gmail.com wrote:

> Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> > On Sun, 10 Dec 2023 13:32:28 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > On Fri,  8 Dec 2023 15:31:19 +0800
> > > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > >   
> > > > When iio_device_register_sysfs_group() fails, we should
> > > > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > > > potential memleak.
> > > > 
> > > > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>    
> > > Hi.
> > > 
> > > Looks good to me, but I'd like to leave this one on the list a little
> > > longer to see if anyone else has comments.
> > >   
> > Guess no comments!  
> 
> This patch does not fix anything.
> 
> Yet, it might be considered as one that increases robustness, but with this applied the 
> goto
> https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/iio/industrialio-core.c#L2007
> can be amended, right?
> 

I'm lost.  That goto results in a call to 
iio_buffers_free_sysfs_and_mask(indio_dev);
which continues to undo stuff from before that call.
Now if it did
iio_device_unregister_sysfs(indio_dev);
(as per the label above it in the cleanup) then I'd agree.

Perhaps I'm misreading the code flow here?

All this code is supposed to be side effect free on error so I'm
keen on the change even if there is another path where it gets cleaned
up that I'm missing.

Jonathan
Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs
Posted by Andy Shevchenko 1 year, 10 months ago 
On Sun, Feb 11, 2024 at 9:37 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 11 Feb 2024 21:16:32 +0200
> andy.shevchenko@gmail.com wrote:
>
> > Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> > > On Sun, 10 Dec 2023 13:32:28 +0000
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > > On Fri,  8 Dec 2023 15:31:19 +0800
> > > > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > > >
> > > > > When iio_device_register_sysfs_group() fails, we should
> > > > > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > > > > potential memleak.
> > > > >
> > > > > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> > > > Hi.
> > > >
> > > > Looks good to me, but I'd like to leave this one on the list a little
> > > > longer to see if anyone else has comments.
> > > >
> > > Guess no comments!
> >
> > This patch does not fix anything.
> >
> > Yet, it might be considered as one that increases robustness, but with this applied the
> > goto
> > https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/iio/industrialio-core.c#L2007
> > can be amended, right?
> >
>
> I'm lost.  That goto results in a call to
> iio_buffers_free_sysfs_and_mask(indio_dev);
> which continues to undo stuff from before that call.
> Now if it did
> iio_device_unregister_sysfs(indio_dev);
> (as per the label above it in the cleanup) then I'd agree.

Ah, it's me who hasn't noticed the word "buffer" in the goto label.
Yes, you are right!

> Perhaps I'm misreading the code flow here?
>
> All this code is supposed to be side effect free on error so I'm
> keen on the change even if there is another path where it gets cleaned
> up that I'm missing.

Everything is fine, sorry for the noise.

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.