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 <[email protected]>
---
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
On Fri, 8 Dec 2023 15:31:19 +0800
Dinghao Liu <[email protected]> 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 <[email protected]>
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);
>
On Sun, 10 Dec 2023 13:32:28 +0000
Jonathan Cameron <[email protected]> wrote:
> On Fri, 8 Dec 2023 15:31:19 +0800
> Dinghao Liu <[email protected]> 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 <[email protected]>
> 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);
> >
>
>
Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> On Sun, 10 Dec 2023 13:32:28 +0000
> Jonathan Cameron <[email protected]> wrote:
>
> > On Fri, 8 Dec 2023 15:31:19 +0800
> > Dinghao Liu <[email protected]> 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 <[email protected]>
> > 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
On Sun, 11 Feb 2024 21:16:32 +0200
[email protected] wrote:
> Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> > On Sun, 10 Dec 2023 13:32:28 +0000
> > Jonathan Cameron <[email protected]> wrote:
> >
> > > On Fri, 8 Dec 2023 15:31:19 +0800
> > > Dinghao Liu <[email protected]> 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 <[email protected]>
> > > 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
On Sun, Feb 11, 2024 at 9:37 PM Jonathan Cameron <[email protected]> wrote:
> On Sun, 11 Feb 2024 21:16:32 +0200
> [email protected] wrote:
>
> > Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> > > On Sun, 10 Dec 2023 13:32:28 +0000
> > > Jonathan Cameron <[email protected]> wrote:
> > >
> > > > On Fri, 8 Dec 2023 15:31:19 +0800
> > > > Dinghao Liu <[email protected]> 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 <[email protected]>
> > > > 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