| Message ID | 20210219085826.46622-2-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: core,buffer-dma: 2 fixes for the recent IIO buffer series | expand |
On Fri, 19 Feb 2021 10:58:25 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> 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 <alexandru.ardelean@analog.com> 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(&p->l); > kfree(p); > }
On Mon, Feb 22, 2021 at 6:06 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Fri, 19 Feb 2021 10:58:25 +0200 > Alexandru Ardelean <alexandru.ardelean@analog.com> 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 <alexandru.ardelean@analog.com> > > 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(&p->l); > > kfree(p); > > } >
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(&p->l); kfree(p); }
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 <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)