Re: [PATCH] dmaengine: fsl_raid: fix 64-bit ring base address programming

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] The return value of fsl_re_chan_probe() is ignored, leading to a NULL pointer dereference on driver removal.
- [High] The driver calls request_irq() without a corresponding free_irq(), causing a Use-After-Free panic on module removal.
- [High] Channels are never added to the DMA device's channel list, resulting in a non-functional driver.
- [High] The interrupt handler is registered before critical driver data is initialized, leading to crashes from early interrupts.
- [High] The driver invokes the DMA client completion callback while holding desc_lock, causing an AB-BA self-deadlock if the client submits a new transaction.
- [High] fsl_re_free_chan_resources() modifies the free_q list without holding desc_lock and improperly loops based on alloc_count.
--

commit ac27e4641c6966f6e1eb9d43776e03a8a1dcc825
Author: Rosen Penev [off-list ref]

dmaengine: fsl_raid: fix 64-bit ring base address programming

The driver claims a 40-bit DMA mask via dma_set_mask(40), but the
inbound/outbound ring base addresses were programmed by masking the
physical address with FSL_RE_ADDR_BIT_MASK (0xF) into the high register
and shifting it right by FSL_RE_ADDR_BIT_SHIFT (4) into the low register.

[Severity: High]
This is a pre-existing issue, but I noticed the return value of
fsl_re_chan_probe() is ignored, which can lead to a NULL pointer dereference
on driver removal.

drivers/dma/fsl_raid.c:fsl_re_probe() {
    ...
			if (rc) {
				fsl_re_chan_probe(ofdev, child, ridx++, off);
				re_priv->total_chans++;
			}
    ...
}

If fsl_re_chan_probe() fails, re_priv->total_chans is still incremented,
and re_priv->re_jrs[q] remains NULL.  During teardown, fsl_re_remove() calls
fsl_re_remove_chan() which unconditionally dereferences it:

drivers/dma/fsl_raid.c:fsl_re_remove_chan() {
	tasklet_kill(&chan->irqtask);
    ...
}

Will this cause a kernel panic on driver unbind if any channel failed to
probe?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed request_irq() is
called without a corresponding free_irq(), which can cause a use-after-free
panic on module removal.

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
	if (ret) {
    ...
err_free:
	return ret;
}

The interrupt handler doesn't appear to be freed in error paths or during
teardown in fsl_re_remove_chan():

drivers/dma/fsl_raid.c:fsl_re_remove_chan() {
	tasklet_kill(&chan->irqtask);

	dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
		      chan->inb_phys_addr);
    ...
}

If a hardware interrupt fires after the module is unloaded, will the kernel
jump into freed module memory?

[Severity: High]
This is a pre-existing issue, but it looks like channels are never added to
the DMA device's channel list, which might result in a non-functional driver.

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
	re_priv->re_jrs[q] = chan;
	chan->chan.device = dma_dev;
	chan->chan.private = chan;
	chan->dev = chandev;
    ...
}

The global channel list is initialized via INIT_LIST_HEAD(&dma_dev->channels),
but list_add_tail(&chan->chan.device_node, &dma_dev->channels) is never
called.  Will dma_async_device_register() iterate over an empty list and
register zero channels?

[Severity: High]
This isn't a bug introduced by this patch, but the interrupt handler is
registered before critical driver data is initialized, which could lead to
crashes from early interrupts.

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
    ...
	dev_set_drvdata(chandev, chan);
    ...
}

If a spurious or early interrupt triggers immediately after request_irq(),
the ISR will execute dev_get_drvdata() and receive NULL:

drivers/dma/fsl_raid.c:fsl_re_isr() {
    ...
	re_chan = dev_get_drvdata((struct device *)data);

	irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
    ...
}

Could this cause a NULL pointer dereference panic while reading
jr_interrupt_status?

[Severity: High]
This is a pre-existing issue, but the driver invokes the DMA client completion
callback while holding desc_lock, which can cause an AB-BA self-deadlock.

drivers/dma/fsl_raid.c:fsl_re_dequeue() {
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
		if (found) {
			fsl_re_desc_done(desc);
    ...
}

If the client submits a chained DMA request within the callback,
fsl_re_tx_submit() attempts to re-acquire the exact same desc_lock:

drivers/dma/fsl_raid.c:fsl_re_tx_submit() {
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
}

Will this instantly deadlock the CPU?

[Severity: High]
This isn't a bug introduced by this patch, but fsl_re_free_chan_resources()
modifies the free_q list without holding desc_lock and improperly loops based
on alloc_count.

drivers/dma/fsl_raid.c:fsl_re_free_chan_resources() {
    ...
	while (re_chan->alloc_count--) {
		desc = list_first_entry(&re_chan->free_q,
					struct fsl_re_desc,
					node);

		list_del(&desc->node);
    ...
		kfree(desc);
	}
}

If any descriptors are still in flight, free_q will be prematurely empty,
causing list_first_entry() to return an invalid pointer which is then freed.
Should this be protected by desc_lock and use list_empty() checks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910201717.44891-1-rosenp@gmail.com?part=1