Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231031210521.1661552-1-dlechner@baylibre.com> <3ea3d92db5c4c077a76b29dc5a89c4d491695752.camel@gmail.com>
In-Reply-To: <3ea3d92db5c4c077a76b29dc5a89c4d491695752.camel@gmail.com>
From:   David Lechner <dlechner@baylibre.com>
Date:   Thu, 2 Nov 2023 09:53:13 -0500
Message-ID: <CAMknhBEDp1baTDPrAfAv_gZ5o6LxNSt8bgS8wcGUmXdaCHq_Jw@mail.gmail.com>
Subject: Re: [PATCH] iio: triggered-buffer: prevent possible freeing of wrong buffer
To:     =?UTF-8?B?TnVubyBTw6E=?= <noname.nuno@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Nov 2, 2023 at 3:59=E2=80=AFAM Nuno S=C3=A1 <noname.nuno@gmail.com>=
 wrote:
>
> On Tue, 2023-10-31 at 16:05 -0500, David Lechner wrote:
> > Commit ee708e6baacd ("iio: buffer: introduce support for attaching more
> > IIO buffers") introduced support for multiple buffers per indio_dev but
> > left indio_dev->buffer for a few legacy use cases.
> >
> > In the case of the triggered buffer, iio_triggered_buffer_cleanup()
> > still assumes that indio_dev->buffer points to the buffer allocated by
> > iio_triggered_buffer_setup_ext(). However, since
> > iio_triggered_buffer_setup_ext() now calls iio_device_attach_buffer()
> > to attach the buffer, indio_dev->buffer will only point to the buffer
> > allocated by iio_device_attach_buffer() if it the first buffer attached=
.
> >
> > This adds a check to make sure that no other buffer has been attached
> > yet to ensure that indio_dev->buffer will be assigned when
> > iio_device_attach_buffer() is called.
> >
> > Fixes: ee708e6baacd ("iio: buffer: introduce support for attaching more=
 IIO
> > buffers")
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >  drivers/iio/buffer/industrialio-triggered-buffer.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c
> > b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > index c7671b1f5ead..c06515987e7a 100644
> > --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> > +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > @@ -46,6 +46,16 @@ int iio_triggered_buffer_setup_ext(struct iio_dev
> > *indio_dev,
> >       struct iio_buffer *buffer;
> >       int ret;
> >
> > +     /*
> > +      * iio_triggered_buffer_cleanup() assumes that the buffer allocat=
ed
> > here
> > +      * is assigned to indio_dev->buffer but this is only the case if =
this
> > +      * function is the first caller to iio_device_attach_buffer(). If
> > +      * indio_dev->buffer is already set then we can't proceed otherwi=
se
> > the
> > +      * cleanup function will try to free a buffer that was not alloca=
ted
> > here.
> > +      */
> > +     if (indio_dev->buffer)
> > +             return -EADDRINUSE;
> > +
>
> Hmmm, good catch! But I think this is just workarounding the real problem

Yes, I could have done a better job explaining my reason for this fix.
It seemed like the simplest fix that could be easily backported to
stable kernels. And then we can look at removing the legacy field
completely in the future.

> because like this, you can only have a triggered buffer by device. This s=
hould
> be fine as we don't really have any multi buffer user so far but ideally =
it
> should be possible.
>
> Long term we might want to think about moving 'pollfunc' to be a per buff=
er
> thing. Not sure how much trouble that would be given that a trigger is al=
so per
> device and I don't know if it would make sense to have a trigger per buff=
er?!
> Ideally, given the multi buffer concept, I would say it makes sense but i=
t might
> be difficult to accomplish. So better to think about it only if there's a=
 real
> usecase for it.
>
> On thing that I guess it could be done is to change the triggered API so =
it
> returns a buffer and so iio_triggered_buffer_cleanup() would also get a p=
ointer
> to the buffer it allocated (similar to what DMA buffer's are doing). But =
that's
> indeed also bigger change... Bahh, I'm likely over complicating things fo=
r now.

This sounds very much like the work I am doing on SPI Engine offload
support - having a trigger associated with a buffer. So maybe
something will come out of that. =C2=AF\_(=E3=83=84)_/=C2=AF

> Fell free to:
>
> Acked-by: Nuno Sa <nuno.sa@analog.com>
>
>