Re: [PATCH v2 1/5] media: ivtv: convert to kthread_run_worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kthread_queue_work() receives a double pointer instead of a pointer to kthread_worker, leading to a build error or runtime memory corruption.
- [High] kthread_destroy_worker() is called before free_irq(), creating a Use-After-Free race condition with the interrupt handler.
--

commit a504d2685cd2f4c4e6b93a81c847d55367eae1c0
Author: Bradley Morgan <brads@mainlining.org>

media: ivtv: convert to kthread_run_worker

Convert ivtv to use kthread_run_worker() instead of the deprecated
kthread_run(kthread_worker_fn) pattern. The new API sets worker->task
before the worker starts.

> diff --git a/drivers/media/pci/ivtv/ivtv-driver.c b/drivers/media/pci/ivtv/ivtv-driver.c
> index b9ea56ec9593f..6f0e0e46deb5b 100644
> --- a/drivers/media/pci/ivtv/ivtv-driver.c
> +++ b/drivers/media/pci/ivtv/ivtv-driver.c
[ ... ]
> @@ -1372,8 +1370,8 @@ static void ivtv_remove(struct pci_dev *pdev)
>  	timer_shutdown_sync(&itv->dma_timer);
>  
>  	/* Kill irq worker */
> -	kthread_flush_worker(&itv->irq_worker);
> -	kthread_stop(itv->irq_worker_task);
> +	kthread_flush_worker(itv->irq_worker);
> +	kthread_destroy_worker(itv->irq_worker);

[Severity: High]
Does this create a use-after-free race condition with the interrupt handler?

kthread_destroy_worker() frees the dynamically allocated worker structure here,
but free_irq() is called later in ivtv_remove():

drivers/media/pci/ivtv/ivtv-driver.c:ivtv_remove() {
	...
	kthread_destroy_worker(itv->irq_worker);
	...
	free_irq(itv->pdev->irq, (void *)itv);
	...
}

Because free_irq() is the synchronization point that guarantees no interrupt
handlers are still running, an in-flight ivtv_irq_handler() on another CPU
can survive past kthread_destroy_worker().

If the handler subsequently attempts to queue work, it will dereference the
already-freed irq_worker pointer, causing a kernel panic.

>  
>  	ivtv_streams_cleanup(itv);
>  	ivtv_udma_free(itv);
> diff --git a/drivers/media/pci/ivtv/ivtv-driver.h b/drivers/media/pci/ivtv/ivtv-driver.h
> index f1f18911332e8..41789a50d1ca6 100644
> --- a/drivers/media/pci/ivtv/ivtv-driver.h
> +++ b/drivers/media/pci/ivtv/ivtv-driver.h
> @@ -668,8 +668,7 @@ struct ivtv {
>  	/* Interrupts & DMA */
>  	u32 irqmask;                    /* active interrupts */
>  	u32 irq_rr_idx;                 /* round-robin stream index */
> -	struct kthread_worker irq_worker;		/* kthread worker for PIO/YUV/VBI actions */
> -	struct task_struct *irq_worker_task;		/* task for irq_worker */
> +	struct kthread_worker *irq_worker;		/* kthread worker for PIO/YUV/VBI actions */

[Severity: High]
Does this change from a struct to a pointer cause a build error or memory
corruption in ivtv_irq_handler()?

Because irq_worker is now a pointer, the unmodified call site in
ivtv_irq_handler() now passes a double pointer:

drivers/media/pci/ivtv/ivtv-irq.c:ivtv_irq_handler() {
    ...
    kthread_queue_work(&itv->irq_worker, &itv->irq_work);
    ...
}

This violates the API contract and will cause a build failure with modern
compilers due to incompatible pointer types.

If the warning is ignored or coerced, kthread_queue_work() will treat the
offset within struct ivtv as a kthread_worker object, corrupting adjacent
fields when it acquires the spinlock, leading to a kernel crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788513591.git.brads@mainlining.org?part=1