[PATCH] iio: light: gp2ap020a00f: drain irq_work after free_irq

[PATCH] iio: light: gp2ap020a00f: drain irq_work after free_irq

[Fan Wu]

The threaded IRQ handler queues data->work through irq_work_queue() so
the trigger is polled from a per-CPU context.  free_irq() does not flush
an irq_work the handler already queued, so after gp2ap020a00f_remove()
returns that work may still run and call iio_trigger_poll() on data->trig,
which the devm cleanup has already freed, causing a use-after-free.

Add irq_work_sync(&data->work) after free_irq() in remove() and in the
probe error path, mirroring commit 78601726d4a5 ("iio: trigger: sysfs:
fix use-after-free on remove").

Found by an in-house static analysis tool, confirmed by manual review.

Fixes: bf29fbeaa13d ("iio: gp2ap020a00f: Add a driver for the device")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/iio/light/gp2ap020a00f.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index c7df4b258e2c..20d1aa9e99d7 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -1560,6 +1560,7 @@ static int gp2ap020a00f_probe(struct i2c_client *client)
 	iio_trigger_unregister(data->trig);
 error_free_irq:
 	free_irq(client->irq, indio_dev);
+	irq_work_sync(&data->work);
 error_uninit_buffer:
 	iio_triggered_buffer_cleanup(indio_dev);
 error_regulator_disable:
@@ -1582,6 +1583,7 @@ static void gp2ap020a00f_remove(struct i2c_client *client)
 	iio_device_unregister(indio_dev);
 	iio_trigger_unregister(data->trig);
 	free_irq(client->irq, indio_dev);
+	irq_work_sync(&data->work);
 	iio_triggered_buffer_cleanup(indio_dev);
 	regulator_disable(data->vled_reg);
 }

Re: [PATCH] iio: light: gp2ap020a00f: drain irq_work after free_irq

[Jonathan Cameron]

On Thu,  6 Aug 2026 13:29:23 +0000
Fan Wu <fanwu01@zju.edu.cn> wrote:

> The threaded IRQ handler queues data->work through irq_work_queue() so
> the trigger is polled from a per-CPU context.  free_irq() does not flush
> an irq_work the handler already queued, so after gp2ap020a00f_remove()
> returns that work may still run and call iio_trigger_poll() on data->trig,
> which the devm cleanup has already freed, causing a use-after-free.
> 
> Add irq_work_sync(&data->work) after free_irq() in remove() and in the
> probe error path, mirroring commit 78601726d4a5 ("iio: trigger: sysfs:
> fix use-after-free on remove").
> 
> Found by an in-house static analysis tool, confirmed by manual review.
> 
> Fixes: bf29fbeaa13d ("iio: gp2ap020a00f: Add a driver for the device")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
Hi,

Thanks for the patch. It looks correct to me but this is an old driver
and we'd not use an irq work at all these days. The reason it was used
was to ensure we could use the trigger from this device to drive capture
on another one that needed a top half.  However the top half (non threaded
bit) of trigger handlers only ever gets a timestamp, so we end up dancing
through
threaded_irq->irq_work->iio_pollfunc_get_timestamp->actualhander.
which almost certainly gives a less useful timestamp than
threaded_irq->actuallhandler without the irq_work part.
The slight risk is someone is actually using that timestamp in
a consumer and will find pf->timestamp is now always 0.  However
I think that's very unlikely with a light sensor (sensor fusion
doesn't really occur much like it does for inertial sensors)

So I think a better solution given you've identified a bug here, would be
to rip out the irq_work dance in favour of a direct call to
iio_trigger_poll_nested() instead of irq_work_queue() in 
gp2ap020a00f_thresh_event_handler()

That would mean also grabbing a timestamp locally in
gp2ap020a00f_trigger_handler() rather than using pf->timestamp()
+ dropping the use of iio_pollfunc_store_time.

A more invasive change, but one that lands us with a more modern solution
and removes the problematic code entirely.

Do you feel like taking that challenge on?  If so do it on top
of this patch because this one is going to be easier to backport.

Applied to the fixes-togreg branch of iio.git but note that won't
go upstream until after rc1 is released (and I'll rebase on that)

Thanks,

Jonathan


> ---
>  drivers/iio/light/gp2ap020a00f.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
> index c7df4b258e2c..20d1aa9e99d7 100644
> --- a/drivers/iio/light/gp2ap020a00f.c
> +++ b/drivers/iio/light/gp2ap020a00f.c
> @@ -1560,6 +1560,7 @@ static int gp2ap020a00f_probe(struct i2c_client *client)
>  	iio_trigger_unregister(data->trig);
>  error_free_irq:
>  	free_irq(client->irq, indio_dev);
> +	irq_work_sync(&data->work);
>  error_uninit_buffer:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  error_regulator_disable:
> @@ -1582,6 +1583,7 @@ static void gp2ap020a00f_remove(struct i2c_client *client)
>  	iio_device_unregister(indio_dev);
>  	iio_trigger_unregister(data->trig);
>  	free_irq(client->irq, indio_dev);
> +	irq_work_sync(&data->work);
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	regulator_disable(data->vled_reg);
>  }
> 

Re: [PATCH] iio: light: gp2ap020a00f: drain irq_work after free_irq

[Fan Wu]

Hi Jonathan,

Thanks for the review and for applying the fix.

Yes, happy to take that on. I agree iio_trigger_poll_nested() is a
better fit here, given the device IRQ handler is already threaded.

I'll prepare a follow-up on top of this patch.

Thanks,
Fan

> On Aug 17, 2026, at 01:46, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> Hi,
> 
> Thanks for the patch. It looks correct to me but this is an old driver
> and we'd not use an irq work at all these days. The reason it was used
> was to ensure we could use the trigger from this device to drive capture
> on another one that needed a top half.  However the top half (non threaded
> bit) of trigger handlers only ever gets a timestamp, so we end up dancing
> through
> threaded_irq->irq_work->iio_pollfunc_get_timestamp->actualhander.
> which almost certainly gives a less useful timestamp than
> threaded_irq->actuallhandler without the irq_work part.
> The slight risk is someone is actually using that timestamp in
> a consumer and will find pf->timestamp is now always 0.  However
> I think that's very unlikely with a light sensor (sensor fusion
> doesn't really occur much like it does for inertial sensors)
> 
> So I think a better solution given you've identified a bug here, would be
> to rip out the irq_work dance in favour of a direct call to
> iio_trigger_poll_nested() instead of irq_work_queue() in 
> gp2ap020a00f_thresh_event_handler()
> 
> That would mean also grabbing a timestamp locally in
> gp2ap020a00f_trigger_handler() rather than using pf->timestamp()
> + dropping the use of iio_pollfunc_store_time.
> 
> A more invasive change, but one that lands us with a more modern solution
> and removes the problematic code entirely.
> 
> Do you feel like taking that challenge on?  If so do it on top
> of this patch because this one is going to be easier to backport.
> 
> Applied to the fixes-togreg branch of iio.git but note that won't
> go upstream until after rc1 is released (and I'll rebase on that)
> 
> Thanks,
> 
> Jonathan
>> 

[PATCH] iio: light: gp2ap020a00f: use iio_trigger_poll_nested()

[Fan Wu]

The threaded IRQ handler queues an irq_work only so that
iio_trigger_poll() can run from a hardirq-like context.  The driver's
own buffer consumer does not need that context: the only thing a
standard IIO pollfunc top half does is store a timestamp in
pf->timestamp, and deferring through the per-CPU irq_work queue moves
that timestamp further away from the actual event.

The device IRQ handler is already threaded, so call
iio_trigger_poll_nested() directly from
gp2ap020a00f_thresh_event_handler() and drop the irq_work, its
callback, its init and the irq_work_sync() drains added by the
preceding use-after-free fix.

As the trigger is now dispatched via handle_nested_irq(), a consumer's
top half no longer runs and its threaded handler executes
synchronously in this device's IRQ thread.  For consumers still using
iio_pollfunc_store_time() this means pf->timestamp is no longer
populated; a consumer that needs a timestamp should take it locally,
as this driver now does in gp2ap020a00f_trigger_handler().

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/iio/light/gp2ap020a00f.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index 330d597..00164fd 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -37,7 +37,6 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
-#include <linux/irq_work.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
@@ -245,7 +244,6 @@ struct gp2ap020a00f_data {
 	struct iio_trigger *trig;
 	struct regmap *regmap;
 	unsigned int thresh_val[4];
-	struct irq_work work;
 	wait_queue_head_t data_ready_queue;
 };
 
@@ -802,14 +800,6 @@ static void gp2ap020a00f_output_to_lux(struct gp2ap020a00f_data *data,
 		*output_val *= 16;
 }
 
-static void gp2ap020a00f_iio_trigger_work(struct irq_work *work)
-{
-	struct gp2ap020a00f_data *data =
-		container_of(work, struct gp2ap020a00f_data, work);
-
-	iio_trigger_poll(data->trig);
-}
-
 static irqreturn_t gp2ap020a00f_prox_sensing_handler(int irq, void *data)
 {
 	struct iio_dev *indio_dev = data;
@@ -932,8 +922,7 @@ static irqreturn_t gp2ap020a00f_thresh_event_handler(int irq, void *data)
 	if (test_bit(GP2AP020A00F_FLAG_ALS_CLEAR_TRIGGER, &priv->flags) ||
 	    test_bit(GP2AP020A00F_FLAG_ALS_IR_TRIGGER, &priv->flags) ||
 	    test_bit(GP2AP020A00F_FLAG_PROX_TRIGGER, &priv->flags))
-		/* This fires off the trigger. */
-		irq_work_queue(&priv->work);
+		iio_trigger_poll_nested(priv->trig);
 
 done:
 	return IRQ_HANDLED;
@@ -944,9 +933,12 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
 	struct iio_poll_func *pf = data;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct gp2ap020a00f_data *priv = iio_priv(indio_dev);
+	s64 timestamp;
 	size_t d_size = 0;
 	int i, out_val, ret;
 
+	timestamp = iio_get_time_ns(indio_dev);
+
 	iio_for_each_active_channel(indio_dev, i) {
 		ret = regmap_bulk_read(priv->regmap, GP2AP020A00F_DATA_REG(i),
 				       &priv->buffer[d_size], 2);
@@ -964,7 +956,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
 		}
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, priv->buffer, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, priv->buffer, timestamp);
 done:
 	iio_trigger_notify_done(indio_dev->trig);
 
@@ -1455,8 +1447,9 @@ static int gp2ap020a00f_probe(struct i2c_client *client)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	/* Allocate buffer */
-	err = iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
-		&gp2ap020a00f_trigger_handler, &gp2ap020a00f_buffer_setup_ops);
+	err = iio_triggered_buffer_setup(indio_dev, NULL,
+					 &gp2ap020a00f_trigger_handler,
+					 &gp2ap020a00f_buffer_setup_ops);
 	if (err < 0)
 		goto error_regulator_disable;
 
@@ -1480,8 +1473,6 @@ static int gp2ap020a00f_probe(struct i2c_client *client)
 		goto error_uninit_buffer;
 	}
 
-	init_irq_work(&data->work, gp2ap020a00f_iio_trigger_work);
-
 	err = iio_trigger_register(data->trig);
 	if (err < 0) {
 		dev_err(dev, "Failed to register iio trigger.\n");
@@ -1498,7 +1489,6 @@ error_trigger_unregister:
 	iio_trigger_unregister(data->trig);
 error_free_irq:
 	free_irq(client->irq, indio_dev);
-	irq_work_sync(&data->work);
 error_uninit_buffer:
 	iio_triggered_buffer_cleanup(indio_dev);
 error_regulator_disable:
@@ -1521,7 +1511,6 @@ static void gp2ap020a00f_remove(struct i2c_client *client)
 	iio_device_unregister(indio_dev);
 	iio_trigger_unregister(data->trig);
 	free_irq(client->irq, indio_dev);
-	irq_work_sync(&data->work);
 	iio_triggered_buffer_cleanup(indio_dev);
 	regulator_disable(data->vled_reg);
 }

Re: [PATCH] iio: light: gp2ap020a00f: use iio_trigger_poll_nested()

[Jonathan Cameron]

On Mon, 17 Aug 2026 02:40:23 +0000
Fan Wu <fanwu01@zju.edu.cn> wrote:


> The threaded IRQ handler queues an irq_work only so that
> iio_trigger_poll() can run from a hardirq-like context.  The driver's

A lot of spaces before that The!

> own buffer consumer does not need that context: the only thing a
> standard IIO pollfunc top half does is store a timestamp in

Call out what function is.  It isn't obviously a 'standard' even
though there is only one that anyone actually uses.

> pf->timestamp, and deferring through the per-CPU irq_work queue moves
> that timestamp further away from the actual event.

This should mention in passing the possibility that this trigger is
being used to trigger capture on another device that is relying on
that pollfunc timestamp.  Then add something about that being considered
unlikely because there are few uses for synchronising data capture
between a light sensor and any other sensors.  Something like

"Whilst in theory there can be other consumers of this trigger relying on
 the pollfunc top half running, given this is a light sensor those are
 considered unlikely to exist in practice."



> 
> The device IRQ handler is already threaded, so call
> iio_trigger_poll_nested() directly from
> gp2ap020a00f_thresh_event_handler() and drop the irq_work, its
> callback, its init and the irq_work_sync() drains added by the
> preceding use-after-free fix.

We don't need all this detail in the commit description. Something like
"
Switch from irq_work to direct call of iio_trigger_poll_nested().
Remove now unnecessary irq_work related infrastructure and ensure a
local time stamp is acquired.
"
Covers the key stuff from this paragraph and the next.

> 
> As the trigger is now dispatched via handle_nested_irq(), a consumer's
> top half no longer runs and its threaded handler executes
> synchronously in this device's IRQ thread.  For consumers still using
> iio_pollfunc_store_time() this means pf->timestamp is no longer
> populated; a consumer that needs a timestamp should take it locally,
> as this driver now does in gp2ap020a00f_trigger_handler().
This hints at the possibility of other consumers - I'd call that out
above.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

> ---

Down here add a note on the dependency so I don't forget about it!

Patch looks good to me. For v2, don't reply to the earlier patches.
That can rapidly get very confusing, so for IIO (and most / possibly
all of the rest of the kernel) separate threads preferred. Ideally
include a link instead to earlier versions as part of the change log.

Thanks for tidying this up!

Jonathan

Re: [PATCH] iio: light: gp2ap020a00f: use iio_trigger_poll_nested()

[Andy Shevchenko]

On Sat, Aug 22, 2026 at 12:56:39AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Aug 2026 02:40:23 +0000
> Fan Wu <fanwu01@zju.edu.cn> wrote:
> 
> > The threaded IRQ handler queues an irq_work only so that
> > iio_trigger_poll() can run from a hardirq-like context.  The driver's
> 
> A lot of spaces before that The!

You mean two? This is an (old) standard. See also '  For' below
or '  Something'. I haven't commented on this, because it's consistent
over the text.

> > own buffer consumer does not need that context:

-- 
With Best Regards,
Andy Shevchenko