Before linedisp_unregister(), call disable_delayed_work_sync(),
not just cancel_delayed_work_sync().

While cancel_delayed_work_sync() cancels queued work and
awaits running work, it doesn't reject future work, such as
scheduled by the timer which is set up in linedisp_register().
This timer may fire just before linedisp_unregister() stops it
and cause kind of use after free.

In contrast, disable_delayed_work_sync() also prevents future work.

Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
---
  drivers/auxdisplay/max6959.c      | 2 +-
  drivers/auxdisplay/seg-led-gpio.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/max6959.c b/drivers/auxdisplay/max6959.c
index 6bbc8d48fb..a123077945 100644
--- a/drivers/auxdisplay/max6959.c
+++ b/drivers/auxdisplay/max6959.c
@@ -148,7 +148,7 @@ static void max6959_i2c_remove(struct i2c_client *client)
  {
  	struct max6959_priv *priv = i2c_get_clientdata(client);

-	cancel_delayed_work_sync(&priv->work);
+	disable_delayed_work_sync(&priv->work);
  	linedisp_unregister(&priv->linedisp);
  }

diff --git a/drivers/auxdisplay/seg-led-gpio.c b/drivers/auxdisplay/seg-led-gpio.c
index dfb62e9ce9..83a2b3ff13 100644
--- a/drivers/auxdisplay/seg-led-gpio.c
+++ b/drivers/auxdisplay/seg-led-gpio.c
@@ -84,7 +84,7 @@ static void seg_led_remove(struct platform_device *pdev)
  {
  	struct seg_led_priv *priv = platform_get_drvdata(pdev);

-	cancel_delayed_work_sync(&priv->work);
+	disable_delayed_work_sync(&priv->work);
  	linedisp_unregister(&priv->linedisp);
  }

-- 
2.54.0

On Sun, May 10, 2026 at 06:06:46PM +0200, Alexander A. Klimov wrote:
> Before linedisp_unregister(), call disable_delayed_work_sync(),
> not just cancel_delayed_work_sync().
> 
> While cancel_delayed_work_sync() cancels queued work and
> awaits running work, it doesn't reject future work, such as
> scheduled by the timer which is set up in linedisp_register().
> This timer may fire just before linedisp_unregister() stops it
> and cause kind of use after free.
> 
> In contrast, disable_delayed_work_sync() also prevents future work.

First of all, split on per-driver basis.
Second, there are not so many users of this API

$ git grep -lw disable_delayed_work_sync | wc
     29      29     882

$ git grep -lw cancel_delayed_work_sync | wc
    826     826   29389

Are many of them prone to the same issue? I don't think so. As far as I read
the code the original variant is fine as long as we guarantee that there may
not be any new work scheduling. In this case the scheduling done whenever we
update the line on the display. This can be initiated via user space.
So the question here is the user space is quiet at that time or not. Seems
not to me. Now, shouldn't just ordering of the unregister and cancellation
of the work fix the issue? (It doesn't seem that the works are doing anything
with the linedisp device.)

Also note there is a bigger issue that had been reported a week or so ago.
The problem is that driver removal may lead to UAF. Fixing that problem may
help to address this one as well for all of the linedisp users (switching to
use managed work cancellation that will be done in time, id est after device
is gone).

-- 
With Best Regards,
Andy Shevchenko

On 5/11/26 12:38, Andy Shevchenko wrote:
> On Sun, May 10, 2026 at 06:06:46PM +0200, Alexander A. Klimov wrote:
>> Before linedisp_unregister(), call disable_delayed_work_sync(),
>> not just cancel_delayed_work_sync().
>>
>> While cancel_delayed_work_sync() cancels queued work and
>> awaits running work, it doesn't reject future work, such as
>> scheduled by the timer which is set up in linedisp_register().
>> This timer may fire just before linedisp_unregister() stops it
>> and cause kind of use after free.
>>
>> In contrast, disable_delayed_work_sync() also prevents future work.
> 
> First of all, split on per-driver basis.
> Second, there are not so many users of this API
> 
> $ git grep -lw disable_delayed_work_sync | wc
>       29      29     882
> 
> $ git grep -lw cancel_delayed_work_sync | wc
>      826     826   29389
> 
> Are many of them prone to the same issue? I don't think so. As far as I read

Why not?
Maybe I just yet had not the time to check all of them?

> the code the original variant is fine as long as we guarantee that there may

Compared to "fine as long as", wouldn't a simple "fine" be an upgrade?

> not be any new work scheduling. In this case the scheduling done whenever we
> update the line on the display. This can be initiated via user space.
> So the question here is the user space is quiet at that time or not. Seems

But the timer may fire just before linedisp_unregister() stops it.
It also schedules work.

> not to me. Now, shouldn't just ordering of the unregister and cancellation
> of the work fix the issue? (It doesn't seem that the works are doing anything
> with the linedisp device.)

To me it seems the opposite, that's why I didn't just reorder.
But I may be wrong.

> 
> Also note there is a bigger issue that had been reported a week or so ago.
> The problem is that driver removal may lead to UAF. Fixing that problem may
> help to address this one as well for all of the linedisp users (switching to

So I'll wait.

> use managed work cancellation that will be done in time, id est after device
> is gone).
>

On Mon, May 11, 2026 at 11:56 PM Alexander A. Klimov
<grandmaster@al2klimov.de> wrote:
> On 5/11/26 12:38, Andy Shevchenko wrote:
> > On Sun, May 10, 2026 at 06:06:46PM +0200, Alexander A. Klimov wrote:
> >> Before linedisp_unregister(), call disable_delayed_work_sync(),
> >> not just cancel_delayed_work_sync().
> >>
> >> While cancel_delayed_work_sync() cancels queued work and
> >> awaits running work, it doesn't reject future work, such as
> >> scheduled by the timer which is set up in linedisp_register().
> >> This timer may fire just before linedisp_unregister() stops it
> >> and cause kind of use after free.
> >>
> >> In contrast, disable_delayed_work_sync() also prevents future work.
> >
> > First of all, split on per-driver basis.
> > Second, there are not so many users of this API
> >
> > $ git grep -lw disable_delayed_work_sync | wc
> >       29      29     882
> >
> > $ git grep -lw cancel_delayed_work_sync | wc
> >      826     826   29389
> >
> > Are many of them prone to the same issue? I don't think so. As far as I read
>
> Why not?
> Maybe I just yet had not the time to check all of them?

Maybe, but from my experience I tend to believe that this is just
about misunderstanding the object lifetime ranges in each of the cases
(either when disable_*() is used when cancel_*() is okay and vice
versa).

> > the code the original variant is fine as long as we guarantee that there may
>
> Compared to "fine as long as", wouldn't a simple "fine" be an upgrade?

The same can be applied to the use of atomic versus spin lock. Yes, we
can do spin locks mostly everywhere, with a cost. I expect a deeper
analysis in the commit message.

> > not be any new work scheduling. In this case the scheduling done whenever we
> > update the line on the display. This can be initiated via user space.
> > So the question here is the user space is quiet at that time or not. Seems
>
> But the timer may fire just before linedisp_unregister() stops it.
> It also schedules work.
>
> > not to me. Now, shouldn't just ordering of the unregister and cancellation
> > of the work fix the issue? (It doesn't seem that the works are doing anything
> > with the linedisp device.)
>
> To me it seems the opposite, that's why I didn't just reorder.
> But I may be wrong.

> > Also note there is a bigger issue that had been reported a week or so ago.
> > The problem is that driver removal may lead to UAF. Fixing that problem may
> > help to address this one as well for all of the linedisp users (switching to
>
> So I'll wait.

That one is not in progress, it's just a known issue. In case you want
to help with that, it will be much appreciated.

> > use managed work cancellation that will be done in time, id est after device
> > is gone).

-- 
With Best Regards,
Andy Shevchenko