linux-kernel - Re: [RFC v2 01/11] iio: introduce iio backend device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7ec3fe6183409c218b97a3359e951731b47fe16d.camel@gmail.com>
Date:   Fri, 28 Jul 2023 10:42:31 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Olivier Moysan <olivier.moysan@...s.st.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [RFC v2 01/11] iio: introduce iio backend device

Hi Olivier,

On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> Add a new device type in IIO framework.
> This backend device does not compute channel attributes and does not expose
> them through sysfs, as done typically in iio-rescale frontend device.
> Instead, it allows to report information applying to channel
> attributes through callbacks. These backend devices can be cascaded
> to represent chained components.
> An IIO device configured as a consumer of a backend device can compute
> the channel attributes of the whole chain.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@...s.st.com>
> ---
>  drivers/iio/Makefile               |   1 +
>  drivers/iio/industrialio-backend.c | 107 +++++++++++++++++++++++++++++
>  include/linux/iio/backend.h        |  56 +++++++++++++++
>  3 files changed, 164 insertions(+)
>  create mode 100644 drivers/iio/industrialio-backend.c
>  create mode 100644 include/linux/iio/backend.h
> 
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 9622347a1c1b..9b59c6ab1738 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -5,6 +5,7 @@
>  
>  obj-$(CONFIG_IIO) += industrialio.o
>  industrialio-y := industrialio-core.o industrialio-event.o inkern.o
> +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
>  industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>  
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> backend.c
> new file mode 100644
> index 000000000000..7d0625889873
> --- /dev/null
> +++ b/drivers/iio/industrialio-backend.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* The industrial I/O core, backend handling functions
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/property.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/backend.h>
> +
> +static DEFINE_IDA(iio_backend_ida);
> +
> +#define to_iio_backend(_device) container_of((_device), struct iio_backend,
> dev)
> +
> +static void iio_backend_release(struct device *device)
> +{
> +       struct iio_backend *backend = to_iio_backend(device);
> +
> +       kfree(backend->name);
> +       kfree(backend);
> +}
> +
> +static const struct device_type iio_backend_type = {
> +       .release = iio_backend_release,
> +       .name = "iio_backend_device",
> +};
> +
> +struct iio_backend *iio_backend_alloc(struct device *parent)
> +{
> +       struct iio_backend *backend;
> +
> +       backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
> 

No error checking. 

I guess a lot of cleanings are still missing but the important thing I wanted to
notice is that the above pattern is not ok. 
Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
refcounted object. Nevertheless, you're binding the lifetime of your object to
the parent device and that is wrong. The reason is that as soon as your parent
device get's released or just unbinded from it's driver, all the devres stuff
(including your 'struct iio_backend' object) will be released independent of
your 'struct device' refcount value...

So, you might argue this won't ever be an issue in here but the pattern is still
wrong. There are some talks about this, the last one was given at the latest
EOSS:

https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation

- Nuno Sá