Re: [PATCH] batman-adv: fix DAT purge use-after-free on teardown

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]

On Tuesday, 26 May 2026 08:48:35 CEST Soowan Park wrote:
> batadv_dat_purge() is a periodic delayed work that re-queues itself via
> batadv_dat_start_timer() at the end of each run. When the mesh interface
> is torn down, batadv_dat_free() calls cancel_delayed_work_sync() to stop
> the purge work before freeing the DAT hash table.
> 
> However, cancel_delayed_work_sync() leaves the work in an enabled state.
> If the purge work is currently executing and re-queues itself before
> cancel_delayed_work_sync() internally marks it for cancellation, the
> newly queued work escapes cancellation. This re-queued work then fires
> after batadv_dat_hash_free() has already freed the hash table but before
> the pointer is set to NULL, causing __batadv_dat_purge() to operate on a
> dangling pointer that passes the NULL check, and spin indefinitely on a
> spinlock in freed memory.


You are talking about a re-queue by batadv_dat_start_timer(). This only 
happens when the DAT gets initialized or via the worker (batadv_dat_purge) 
itself. How can the worker which is cancelled (with sync) re-queue itself? 
Isn't this breaking a guarantee of cancel_delayed_work_sync() or did I 
misunderstand this part of the documentation?


"This is cancel_work_sync() for delayed works." [1]

"Cancel work and wait for its execution to finish. This function can be used 
even if the work re-queues itself or migrates to another workqueue. On return 
from this function, work is guaranteed to be not pending or executing on any 
CPU as long as there aren’t racing enqueues." [2]

(the part "This function can be used even if the work re-queues itself" is
the important part here).


> Replace cancel_delayed_work_sync() with disable_delayed_work_sync(),
> which additionally disables the work so that any concurrent
> queue_delayed_work() call from the running batadv_dat_purge() is
> silently rejected. This guarantees no re-queued work can fire after
> disable_delayed_work_sync() returns.

I have no problem with using "disabled_*" everywhere (I even have a pending 
patchset to use it - just to avoid problems with code changes in the future). 
But since this is a fix which I don't get in the moment, I would like to 
understand the problem you are describing better before applying it.

Regards,
	Sven


[1] https://www.kernel.org/doc/html/v7.0/core-api/workqueue.html#c.cancel_delayed_work_sync
[2] https://www.kernel.org/doc/html/v7.0/core-api/workqueue.html#c.cancel_work_sync

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]