Re: [PATCH 1/2] iio: core: use kfree_const in iio_free_chan_devattr_list() to free names

[Alexandru Ardelean]

On Mon, Feb 22, 2021 at 6:06 PM Jonathan Cameron
 wrote:
>
> On Fri, 19 Feb 2021 10:58:25 +0200
> Alexandru Ardelean  wrote:
>
> > When the buffer attributes were wrapped in iio_dev_attr types, I forgot to
> > duplicate the names, so that when iio_free_chan_devattr_list() gets called
> > on cleanup, these get free'd.
> > I stumbled over this while accidentally breaking a driver doing
> > iio_device_register(), and then the issue appeared.
> >
> > The fix can be just
> > 1. Just use kstrdup() during iio_buffer_wrap_attr()
> > 2. Just use kfree_const() during iio_free_chan_devattr_list
> > 3. Use both kstrdup_const() & kfree_const() (in the places mentioned above)
> >
> > Using kfree_const() should be sufficient, as the attribute names will
> > either be allocated or be stored in rodata.
> >
> > Fixes: a1a11142f66c ("iio: buffer: wrap all buffer attributes into 
> > iio_dev_attr")
> > Signed-off-by: Alexandru Ardelean 
>
> Thinking more on this...  It's fine for the users today, but there is
> nothing stopping a driver passing in names it allocated on the heap.  So
> I think we should revisit this.  Perhaps we need 1 or 3.

Ok.
Will re-send this as 3; that sounds like it gives the best of both worlds.

> > ---
> >  drivers/iio/industrialio-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c 
> > b/drivers/iio/industrialio-core.c
> > index 0d8c6e88d993..cb2735d2ae4b 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1358,7 +1358,7 @@ void iio_free_chan_devattr_list(struct list_head 
> > *attr_list)
> >   struct iio_dev_attr *p, *n;
> >
> >   list_for_each_entry_safe(p, n, attr_list, l) {
> > - kfree(p->dev_attr.attr.name);
> > + kfree_const(p->dev_attr.attr.name);
> >   list_del(>l);
> >   kfree(p);
> >   }
>

Re: [PATCH 1/2] iio: core: use kfree_const in iio_free_chan_devattr_list() to free names

[Jonathan Cameron]

On Fri, 19 Feb 2021 10:58:25 +0200
Alexandru Ardelean  wrote:

> When the buffer attributes were wrapped in iio_dev_attr types, I forgot to
> duplicate the names, so that when iio_free_chan_devattr_list() gets called
> on cleanup, these get free'd.
> I stumbled over this while accidentally breaking a driver doing
> iio_device_register(), and then the issue appeared.
> 
> The fix can be just
> 1. Just use kstrdup() during iio_buffer_wrap_attr()
> 2. Just use kfree_const() during iio_free_chan_devattr_list
> 3. Use both kstrdup_const() & kfree_const() (in the places mentioned above)
> 
> Using kfree_const() should be sufficient, as the attribute names will
> either be allocated or be stored in rodata.
> 
> Fixes: a1a11142f66c ("iio: buffer: wrap all buffer attributes into 
> iio_dev_attr")
> Signed-off-by: Alexandru Ardelean 

Thinking more on this...  It's fine for the users today, but there is
nothing stopping a driver passing in names it allocated on the heap.  So
I think we should revisit this.  Perhaps we need 1 or 3.
> ---
>  drivers/iio/industrialio-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 0d8c6e88d993..cb2735d2ae4b 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1358,7 +1358,7 @@ void iio_free_chan_devattr_list(struct list_head 
> *attr_list)
>   struct iio_dev_attr *p, *n;
>  
>   list_for_each_entry_safe(p, n, attr_list, l) {
> - kfree(p->dev_attr.attr.name);
> + kfree_const(p->dev_attr.attr.name);
>   list_del(>l);
>   kfree(p);
>   }

[PATCH 1/2] iio: core: use kfree_const in iio_free_chan_devattr_list() to free names

[Alexandru Ardelean]

When the buffer attributes were wrapped in iio_dev_attr types, I forgot to
duplicate the names, so that when iio_free_chan_devattr_list() gets called
on cleanup, these get free'd.
I stumbled over this while accidentally breaking a driver doing
iio_device_register(), and then the issue appeared.

The fix can be just
1. Just use kstrdup() during iio_buffer_wrap_attr()
2. Just use kfree_const() during iio_free_chan_devattr_list
3. Use both kstrdup_const() & kfree_const() (in the places mentioned above)

Using kfree_const() should be sufficient, as the attribute names will
either be allocated or be stored in rodata.

Fixes: a1a11142f66c ("iio: buffer: wrap all buffer attributes into 
iio_dev_attr")
Signed-off-by: Alexandru Ardelean 
---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 0d8c6e88d993..cb2735d2ae4b 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1358,7 +1358,7 @@ void iio_free_chan_devattr_list(struct list_head 
*attr_list)
struct iio_dev_attr *p, *n;
 
list_for_each_entry_safe(p, n, attr_list, l) {
-   kfree(p->dev_attr.attr.name);
+   kfree_const(p->dev_attr.attr.name);
list_del(>l);
kfree(p);
}
-- 
2.27.0