Re: [PATCH v6 19/24] iio: buffer: introduce support for attaching more IIO buffers

2021-02-28 Thread Alexandru Ardelean 
On Sun, Feb 28, 2021 at 10:31 AM Lars-Peter Clausen  wrote:
>
> On 2/15/21 11:40 AM, Alexandru Ardelean wrote:
> >   static ssize_t iio_show_scan_index(struct device *dev,
> >  struct device_attribute *attr,
> >  char *buf)
> > @@ -1451,11 +1465,13 @@ static void __iio_buffer_free_sysfs_and_mask(struct 
> > iio_buffer *buffer)
> >   iio_free_chan_devattr_list(>buffer_attr_list);
> >   }
> >
> > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > +int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >   {
> > [...]
> > +error_unwind_sysfs_and_mask:
> > + for (; unwind_idx >= 0; unwind_idx--) {
> > + buffer = iio_dev_opaque->attached_buffers[unwind_idx];
> > + __iio_buffer_free_sysfs_and_mask(buffer);
> > + }
> > + kfree(iio_dev_opaque->attached_buffers);
> > + return ret;
> >   }
> >
> > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > +void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev)
> >   {
> > [...]
> > + for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
> > + buffer = iio_dev_opaque->attached_buffers[i];
> > + __iio_buffer_free_sysfs_and_mask(buffer);
> > + }
> > +
> > + kfree(iio_dev_opaque->attached_buffers);
> >   }
> > [...]
> > diff --git a/drivers/iio/industrialio-core.c 
> > b/drivers/iio/industrialio-core.c
> > index 1d500ea246af..f7f785431106 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1583,7 +1583,7 @@ static void iio_dev_release(struct device *device)
> >   iio_device_unregister_eventset(indio_dev);
> >   iio_device_unregister_sysfs(indio_dev);
> >
> > - iio_buffer_put(indio_dev->buffer);
> > + iio_buffers_put(indio_dev);
> We do call kfree(iio_dev_opaque->attached_buffers) before we get here. I
> think we need to keep the array around, otherwise we end of up with a
> use after free.
>

Good catch.
Will send an update here.

Re: [PATCH v6 19/24] iio: buffer: introduce support for attaching more IIO buffers

2021-02-28 Thread Lars-Peter Clausen 

On 2/15/21 11:40 AM, Alexandru Ardelean wrote:

  static ssize_t iio_show_scan_index(struct device *dev,
   struct device_attribute *attr,
   char *buf)
@@ -1451,11 +1465,13 @@ static void __iio_buffer_free_sysfs_and_mask(struct 
iio_buffer *buffer)
iio_free_chan_devattr_list(>buffer_attr_list);
  }
  
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)

+int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
  {
[...]
+error_unwind_sysfs_and_mask:
+   for (; unwind_idx >= 0; unwind_idx--) {
+   buffer = iio_dev_opaque->attached_buffers[unwind_idx];
+   __iio_buffer_free_sysfs_and_mask(buffer);
+   }
+   kfree(iio_dev_opaque->attached_buffers);
+   return ret;
  }
  
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)

+void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev)
  {
[...]
+   for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
+   buffer = iio_dev_opaque->attached_buffers[i];
+   __iio_buffer_free_sysfs_and_mask(buffer);
+   }
+
+   kfree(iio_dev_opaque->attached_buffers);
  }
[...]
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 1d500ea246af..f7f785431106 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1583,7 +1583,7 @@ static void iio_dev_release(struct device *device)
iio_device_unregister_eventset(indio_dev);
iio_device_unregister_sysfs(indio_dev);
  
-	iio_buffer_put(indio_dev->buffer);

+   iio_buffers_put(indio_dev);
We do call kfree(iio_dev_opaque->attached_buffers) before we get here. I 
think we need to keep the array around, otherwise we end of up with a 
use after free.

[PATCH v6 19/24] iio: buffer: introduce support for attaching more IIO buffers

2021-02-15 Thread Alexandru Ardelean 
With this change, calling iio_device_attach_buffer() will actually attach
more buffers.
Right now this doesn't do any validation of whether a buffer is attached
twice; maybe that can be added later (if needed). Attaching a buffer more
than once should yield noticeably bad results.

The first buffer is the legacy buffer, so a reference is kept to it.

At this point, accessing the data for the extra buffers (that are added
after the first one) isn't possible yet.

The iio_device_attach_buffer() is also changed to return an error code,
which for now is -ENOMEM if the array could not be realloc-ed for more
buffers.
To adapt to this new change iio_device_attach_buffer() is called last in
all place where it's called. The realloc failure is a bit difficult to
handle during un-managed calls when unwinding, so it's better to have this
as the last error in the setup_buffer calls.

At this point, no driver should call iio_device_attach_buffer() directly,
it should call one of the {devm_}iio_triggered_buffer_setup() or
devm_iio_kfifo_buffer_setup() or devm_iio_dmaengine_buffer_setup()
functions. This makes iio_device_attach_buffer() a bit easier to handle.

Signed-off-by: Alexandru Ardelean 
---
 .../buffer/industrialio-buffer-dmaengine.c|   4 +-
 .../buffer/industrialio-triggered-buffer.c|  10 +-
 drivers/iio/buffer/kfifo_buf.c|   4 +-
 drivers/iio/iio_core.h|  10 +-
 drivers/iio/industrialio-buffer.c | 100 ++
 drivers/iio/industrialio-core.c   |  12 +--
 include/linux/iio/buffer.h|   4 +-
 include/linux/iio/buffer_impl.h   |   3 +
 include/linux/iio/iio-opaque.h|   4 +
 9 files changed, 111 insertions(+), 40 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c 
b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index a64b89be..d76179878ff9 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -290,9 +290,7 @@ int devm_iio_dmaengine_buffer_setup(struct device *dev,
 
indio_dev->modes |= INDIO_BUFFER_HARDWARE;
 
-   iio_device_attach_buffer(indio_dev, buffer);
-
-   return 0;
+   return iio_device_attach_buffer(indio_dev, buffer);
 }
 EXPORT_SYMBOL_GPL(devm_iio_dmaengine_buffer_setup);
 
diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c 
b/drivers/iio/buffer/industrialio-triggered-buffer.c
index 92b8aea3e063..b2b1b7d27af4 100644
--- a/drivers/iio/buffer/industrialio-triggered-buffer.c
+++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
@@ -50,8 +50,6 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
goto error_ret;
}
 
-   iio_device_attach_buffer(indio_dev, buffer);
-
indio_dev->pollfunc = iio_alloc_pollfunc(h,
 thread,
 IRQF_ONESHOT,
@@ -72,10 +70,16 @@ int iio_triggered_buffer_setup_ext(struct iio_dev 
*indio_dev,
 
buffer->attrs = buffer_attrs;
 
+   ret = iio_device_attach_buffer(indio_dev, buffer);
+   if (ret < 0)
+   goto error_dealloc_pollfunc;
+
return 0;
 
+error_dealloc_pollfunc:
+   iio_dealloc_pollfunc(indio_dev->pollfunc);
 error_kfifo_free:
-   iio_kfifo_free(indio_dev->buffer);
+   iio_kfifo_free(buffer);
 error_ret:
return ret;
 }
diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index c35a625280b1..34289ce12f20 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -235,12 +235,10 @@ int devm_iio_kfifo_buffer_setup(struct device *dev,
if (!buffer)
return -ENOMEM;
 
-   iio_device_attach_buffer(indio_dev, buffer);
-
indio_dev->modes |= mode_flags;
indio_dev->setup_ops = setup_ops;
 
-   return 0;
+   return iio_device_attach_buffer(indio_dev, buffer);
 }
 EXPORT_SYMBOL_GPL(devm_iio_kfifo_buffer_setup);
 
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 87868fff7d37..7990c759f1f5 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -69,29 +69,31 @@ __poll_t iio_buffer_poll(struct file *filp,
 ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
  size_t n, loff_t *f_ps);
 
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
+int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
+void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
 
 #define iio_buffer_poll_addr (_buffer_poll)
 #define iio_buffer_read_outer_addr (_buffer_read_outer)
 
 void iio_disable_all_buffers(struct iio_dev *indio_dev);
 void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
+void iio_buffers_put(struct iio_dev *indio_dev);
 
 #else
 
 #define iio_buffer_poll_addr NULL
 #define