[PATCH 0/2] iio: core,buffer-dma: 2 fixes for the recent IIO buffer series

[PATCH 0/2] iio: core,buffer-dma: 2 fixes for the recent IIO buffer series

[Alexandru Ardelean]

Patchset contains 2 fixes for some patches that are present in the
iio/testing branch.

No idea what's best now, either to re-send the series or to just send these
fixes on their own.
For now I chose to send the fixes on their (due to lack of time).

These could be squashed into the original.

I can also re-send the series, but not from an Analog email; since I will
not have access to it.

Alexandru Ardelean (2):
  iio: core: use kfree_const in iio_free_chan_devattr_list() to free
    names
  iio: buffer-dma: fix type of 'i' in iio_dma_buffer_alloc_blocks()

 drivers/iio/buffer/industrialio-buffer-dma.c | 3 +--
 drivers/iio/industrialio-core.c              | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

-- 
2.27.0

[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 <alexandru.ardelean@analog.com>
---
 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);
 	}
-- 
2.27.0

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 <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);
>  	}

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
<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);
> >       }
>

[PATCH v2] iio: core: use kstrdup_const/kfree_const for buffer attributes

[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.

Some ways to fix this are:
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)

This implements the third option, as it allows some users/drivers to
specify some attributes allocated on the heap.

Fixes: a1a11142f66c ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 1 +
 drivers/iio/industrialio-core.c   | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 5d641f8adfbd..ac882e60c419 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1306,6 +1306,7 @@ static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer,
 		return NULL;
 
 	iio_attr->buffer = buffer;
+	iio_attr->dev_attr.attr.name = kstrdup_const(attr->name, GFP_KERNEL);
 	memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr));
 
 	list_add(&iio_attr->l, &buffer->buffer_attr_list);
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);
 	}
-- 
2.25.1

Re: [PATCH v2] iio: core: use kstrdup_const/kfree_const for buffer attributes

[Jonathan Cameron]

On Tue, 23 Feb 2021 09:29:26 +0200
Alexandru Ardelean <ardeleanalex@gmail.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.
> 
> Some ways to fix this are:
> 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)
> 
> This implements the third option, as it allows some users/drivers to
> specify some attributes allocated on the heap.
> 
> Fixes: a1a11142f66c ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-buffer.c | 1 +
>  drivers/iio/industrialio-core.c   | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 5d641f8adfbd..ac882e60c419 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1306,6 +1306,7 @@ static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer,
>  		return NULL;
>  
>  	iio_attr->buffer = buffer;
> +	iio_attr->dev_attr.attr.name = kstrdup_const(attr->name, GFP_KERNEL);
>  	memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr));

Doesn't this wipe out the duplicated string?  I swapped the two lines above
which I think should avoid that.

Merged into original patch

Jonathan


>  
>  	list_add(&iio_attr->l, &buffer->buffer_attr_list);
> 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);
>  	}

[PATCH 2/2] iio: buffer-dma: fix type of 'i' in iio_dma_buffer_alloc_blocks()

[Alexandru Ardelean]

When unwinding the cleanup of the blocks, the type of 'i' is important to
be signed, so that the 'i >= 0' condition doesn't underflow and infinitely
loop (or just cause an access violation).

Fixes: ab925b044cfb ("iio: buffer-dma: Add mmap support")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/buffer/industrialio-buffer-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index d04479194cb4..83074d060535 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -707,8 +707,7 @@ int iio_dma_buffer_alloc_blocks(struct iio_buffer *buffer,
 	struct iio_dma_buffer_queue *queue = iio_buffer_to_queue(buffer);
 	struct iio_dma_buffer_block **blocks;
 	unsigned int num_blocks;
-	unsigned int i;
-	int ret = 0;
+	int i, ret = 0;
 
 	mutex_lock(&queue->lock);
 
-- 
2.27.0

Re: [PATCH 0/2] iio: core,buffer-dma: 2 fixes for the recent IIO buffer series

[Jonathan Cameron]

On Fri, 19 Feb 2021 10:58:24 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Patchset contains 2 fixes for some patches that are present in the
> iio/testing branch.
> 
> No idea what's best now, either to re-send the series or to just send these
> fixes on their own.
> For now I chose to send the fixes on their (due to lack of time).
> 
> These could be squashed into the original.
>
That is the approach I've taken.

Tidier to do it this way as I'd not yet pushed the tree out other than as
testing.

Thanks,

Jonathan
 
> I can also re-send the series, but not from an Analog email; since I will
> not have access to it.
> 
> Alexandru Ardelean (2):
>   iio: core: use kfree_const in iio_free_chan_devattr_list() to free
>     names
>   iio: buffer-dma: fix type of 'i' in iio_dma_buffer_alloc_blocks()
> 
>  drivers/iio/buffer/industrialio-buffer-dma.c | 3 +--
>  drivers/iio/industrialio-core.c              | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
>