| Message ID | 20200407145918.6833-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: buffer: remove null-checks for 'indio_dev->info' | expand |
On Tue, 7 Apr 2020 17:59:18 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > Checking for 'indio_dev->info' is an impossible condition, since an IIO > device should NOT be able to register without that information. > The iio_device_register() function won't allow an IIO device to register if > 'indio_dev->info' is NULL. > > If that information somehow becomes NULL, then we're likely busted anyway > and we should crash the system, if we haven't already. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> I'm glad there was a comment in there to remind me of what was going on here. This is the result of an ancient set from (I think) Lars hardening IIO against forced removal. The indio_dev->info == NULL is deliberately set to true by the IIO core during device remove in order to deal with any inflight data. Reference counting should ensure the device doesn't go away but we need to avoid actually doing anything if this occurs. That pointer was a convenient option to avoid having to add an explicit flag or 'going away'. Now, it's possible we don't need this any more due to other changes but I certainly don't want to remove it without that being very thoroughly verified! Thanks, Jonathan > --- > drivers/iio/industrialio-buffer.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index e6fa1a4e135d..c96071bfada8 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -54,10 +54,6 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf, > size_t avail; > int flushed = 0; > > - /* wakeup if the device was unregistered */ This comment makes it clear this isn't as 'obvious' as it might at first seem ;) > - if (!indio_dev->info) > - return true; > - > /* drain the buffer if it was disabled */ > if (!iio_buffer_is_active(buf)) { > to_wait = min_t(size_t, to_wait, 1); > @@ -109,9 +105,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf, > size_t to_wait; > int ret = 0; > > - if (!indio_dev->info) > - return -ENODEV; > - > if (!rb || !rb->access->read) > return -EINVAL; > > @@ -131,11 +124,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf, > > add_wait_queue(&rb->pollq, &wait); > do { > - if (!indio_dev->info) { > - ret = -ENODEV; > - break; > - } > - > if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) { > if (signal_pending(current)) { > ret = -ERESTARTSYS; > @@ -171,7 +159,7 @@ __poll_t iio_buffer_poll(struct file *filp, > struct iio_dev *indio_dev = filp->private_data; > struct iio_buffer *rb = indio_dev->buffer; > > - if (!indio_dev->info || rb == NULL) > + if (rb == NULL) > return 0; > > poll_wait(filp, &rb->pollq, wait); > @@ -1100,11 +1088,6 @@ int iio_update_buffers(struct iio_dev *indio_dev, > goto out_unlock; > } > > - if (indio_dev->info == NULL) { > - ret = -ENODEV; > - goto out_unlock; > - } > - > ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer); > > out_unlock:
On Sun, 2020-04-12 at 14:30 +0100, Jonathan Cameron wrote: > [External] > > On Tue, 7 Apr 2020 17:59:18 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > Checking for 'indio_dev->info' is an impossible condition, since an IIO > > device should NOT be able to register without that information. > > The iio_device_register() function won't allow an IIO device to register if > > 'indio_dev->info' is NULL. > > > > If that information somehow becomes NULL, then we're likely busted anyway > > and we should crash the system, if we haven't already. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > I'm glad there was a comment in there to remind me of what was going on here. > > This is the result of an ancient set from (I think) Lars hardening IIO > against forced removal. > > The indio_dev->info == NULL is deliberately set to true by the IIO core > during device remove in order to deal with any inflight data. > > Reference counting should ensure the device doesn't go away but we need > to avoid actually doing anything if this occurs. That pointer was a > convenient option to avoid having to add an explicit flag or 'going away'. > > Now, it's possible we don't need this any more due to other changes but > I certainly don't want to remove it without that being very thoroughly > verified! > Agreed. Thanks for the info. Will think about this a bit later. Thanks Alex > Thanks, > > Jonathan > > > --- > > drivers/iio/industrialio-buffer.c | 19 +------------------ > > 1 file changed, 1 insertion(+), 18 deletions(-) > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > > buffer.c > > index e6fa1a4e135d..c96071bfada8 100644 > > --- a/drivers/iio/industrialio-buffer.c > > +++ b/drivers/iio/industrialio-buffer.c > > @@ -54,10 +54,6 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, > > struct iio_buffer *buf, > > size_t avail; > > int flushed = 0; > > > > - /* wakeup if the device was unregistered */ > This comment makes it clear this isn't as 'obvious' as it might at first seem > ;) > > > - if (!indio_dev->info) > > - return true; > > - > > /* drain the buffer if it was disabled */ > > if (!iio_buffer_is_active(buf)) { > > to_wait = min_t(size_t, to_wait, 1); > > @@ -109,9 +105,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char > > __user *buf, > > size_t to_wait; > > int ret = 0; > > > > - if (!indio_dev->info) > > - return -ENODEV; > > - > > if (!rb || !rb->access->read) > > return -EINVAL; > > > > @@ -131,11 +124,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char > > __user *buf, > > > > add_wait_queue(&rb->pollq, &wait); > > do { > > - if (!indio_dev->info) { > > - ret = -ENODEV; > > - break; > > - } > > - > > if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) { > > if (signal_pending(current)) { > > ret = -ERESTARTSYS; > > @@ -171,7 +159,7 @@ __poll_t iio_buffer_poll(struct file *filp, > > struct iio_dev *indio_dev = filp->private_data; > > struct iio_buffer *rb = indio_dev->buffer; > > > > - if (!indio_dev->info || rb == NULL) > > + if (rb == NULL) > > return 0; > > > > poll_wait(filp, &rb->pollq, wait); > > @@ -1100,11 +1088,6 @@ int iio_update_buffers(struct iio_dev *indio_dev, > > goto out_unlock; > > } > > > > - if (indio_dev->info == NULL) { > > - ret = -ENODEV; > > - goto out_unlock; > > - } > > - > > ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer); > > > > out_unlock:
On 4/12/20 4:31 PM, Ardelean, Alexandru wrote: > On Sun, 2020-04-12 at 14:30 +0100, Jonathan Cameron wrote: >> [External] >> >> On Tue, 7 Apr 2020 17:59:18 +0300 >> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: >> >>> Checking for 'indio_dev->info' is an impossible condition, since an IIO >>> device should NOT be able to register without that information. >>> The iio_device_register() function won't allow an IIO device to register if >>> 'indio_dev->info' is NULL. >>> >>> If that information somehow becomes NULL, then we're likely busted anyway >>> and we should crash the system, if we haven't already. >>> >>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> >> I'm glad there was a comment in there to remind me of what was going on here. >> >> This is the result of an ancient set from (I think) Lars hardening IIO >> against forced removal. >> >> The indio_dev->info == NULL is deliberately set to true by the IIO core >> during device remove in order to deal with any inflight data. >> >> Reference counting should ensure the device doesn't go away but we need >> to avoid actually doing anything if this occurs. That pointer was a >> convenient option to avoid having to add an explicit flag or 'going away'. >> >> Now, it's possible we don't need this any more due to other changes but >> I certainly don't want to remove it without that being very thoroughly >> verified! >> > Agreed. > Thanks for the info. > > Will think about this a bit later. > > Thanks > Alex I'm pretty sure we need this. It is to handle the case when the device is removed while there a still open file handles. This is necessary since we can't stop a physical device from being remove. E.g. thing a USB device, if somebody pulls it out you can't access it anymore. This means you can do any USB requests anymore, but there might still be an open file handle. So that means even while the physical device has already been removed the open file still has a reference to the iio_dev, which means it is kept around and it is possible to do read(), poll(), ... on it. In fact as soon as the remove() callback has finished the underlying device is gone, even the module that implements the code might have been removed. So we must make sure to not call into any of the callbacks anymore. For this reason we set info to NULL. - Lars > > >> Thanks, >> >> Jonathan >> >>> --- >>> drivers/iio/industrialio-buffer.c | 19 +------------------ >>> 1 file changed, 1 insertion(+), 18 deletions(-) >>> >>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- >>> buffer.c >>> index e6fa1a4e135d..c96071bfada8 100644 >>> --- a/drivers/iio/industrialio-buffer.c >>> +++ b/drivers/iio/industrialio-buffer.c >>> @@ -54,10 +54,6 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, >>> struct iio_buffer *buf, >>> size_t avail; >>> int flushed = 0; >>> >>> - /* wakeup if the device was unregistered */ >> This comment makes it clear this isn't as 'obvious' as it might at first seem >> ;) >> >>> - if (!indio_dev->info) >>> - return true; >>> - >>> /* drain the buffer if it was disabled */ >>> if (!iio_buffer_is_active(buf)) { >>> to_wait = min_t(size_t, to_wait, 1); >>> @@ -109,9 +105,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char >>> __user *buf, >>> size_t to_wait; >>> int ret = 0; >>> >>> - if (!indio_dev->info) >>> - return -ENODEV; >>> - >>> if (!rb || !rb->access->read) >>> return -EINVAL; >>> >>> @@ -131,11 +124,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char >>> __user *buf, >>> >>> add_wait_queue(&rb->pollq, &wait); >>> do { >>> - if (!indio_dev->info) { >>> - ret = -ENODEV; >>> - break; >>> - } >>> - >>> if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) { >>> if (signal_pending(current)) { >>> ret = -ERESTARTSYS; >>> @@ -171,7 +159,7 @@ __poll_t iio_buffer_poll(struct file *filp, >>> struct iio_dev *indio_dev = filp->private_data; >>> struct iio_buffer *rb = indio_dev->buffer; >>> >>> - if (!indio_dev->info || rb == NULL) >>> + if (rb == NULL) >>> return 0; >>> >>> poll_wait(filp, &rb->pollq, wait); >>> @@ -1100,11 +1088,6 @@ int iio_update_buffers(struct iio_dev *indio_dev, >>> goto out_unlock; >>> } >>> >>> - if (indio_dev->info == NULL) { >>> - ret = -ENODEV; >>> - goto out_unlock; >>> - } >>> - >>> ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer); >>> >>> out_unlock:
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index e6fa1a4e135d..c96071bfada8 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -54,10 +54,6 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf, size_t avail; int flushed = 0; - /* wakeup if the device was unregistered */ - if (!indio_dev->info) - return true; - /* drain the buffer if it was disabled */ if (!iio_buffer_is_active(buf)) { to_wait = min_t(size_t, to_wait, 1); @@ -109,9 +105,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf, size_t to_wait; int ret = 0; - if (!indio_dev->info) - return -ENODEV; - if (!rb || !rb->access->read) return -EINVAL; @@ -131,11 +124,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf, add_wait_queue(&rb->pollq, &wait); do { - if (!indio_dev->info) { - ret = -ENODEV; - break; - } - if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) { if (signal_pending(current)) { ret = -ERESTARTSYS; @@ -171,7 +159,7 @@ __poll_t iio_buffer_poll(struct file *filp, struct iio_dev *indio_dev = filp->private_data; struct iio_buffer *rb = indio_dev->buffer; - if (!indio_dev->info || rb == NULL) + if (rb == NULL) return 0; poll_wait(filp, &rb->pollq, wait); @@ -1100,11 +1088,6 @@ int iio_update_buffers(struct iio_dev *indio_dev, goto out_unlock; } - if (indio_dev->info == NULL) { - ret = -ENODEV; - goto out_unlock; - } - ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer); out_unlock:
Checking for 'indio_dev->info' is an impossible condition, since an IIO device should NOT be able to register without that information. The iio_device_register() function won't allow an IIO device to register if 'indio_dev->info' is NULL. If that information somehow becomes NULL, then we're likely busted anyway and we should crash the system, if we haven't already. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-buffer.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)