Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Improper cleanup on `device_register()` failure leads to memory leaks and potential Use-After-Free. - [High] Missing `ida_free()` in the error teardown path of `dma_async_device_register()` leads to permanent leakage of the device ID and successfully registered channel IDs. - [High] Channel devices are modified after they have been globally registered and exposed to the system. -- commit bdff887fbc44608c64e51be1b2c615d4ee5aaa43 Author: Frank Li <[email protected]> dmaengine: add union chan_dev for dma_chan::dev for clarity This commit introduces a union chan_dev in the dma_chan structure to explicitly separate the per-channel device from the DMA engine device. It updates various DMA drivers to use chan_dev instead of dev to improve clarity, with no intended functional changes. > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 6ffd8bd82154a..c00641366c4dc 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c [ ... ] > @@ -1122,7 +1122,7 @@ static int __dma_async_device_channel_register(struct > dma_device *device, > err_out_ida: > ida_free(&device->chan_ida, chan->chan_id); > err_free_dev: > - kfree(chan->dev); > + kfree(chan->chan_dev); > err_free_local: > free_percpu(chan->local); [Severity: High] This isn't a bug introduced by this patch, but does this error path properly surrender the device reference? If device_register() fails earlier in __dma_async_device_channel_register(), the driver core API requires calling put_device() to surrender the initialized reference. Instead, the error path jumps to err_free_dev and directly calls kfree() on the structure. Can this leak resources allocated during initialization, such as the device name string, or risk a use-after-free if other subsystems obtained a temporary reference? [ ... ] > @@ -1290,9 +1290,9 @@ int dma_async_device_register(struct dma_device *device) > if (chan->local == NULL) > continue; > mutex_lock(&dma_list_mutex); > - chan->dev->chan = NULL; > + chan->chan_dev->chan = NULL; > mutex_unlock(&dma_list_mutex); > - device_unregister(&chan->dev->device); > + device_unregister(&chan->chan_dev->device); > free_percpu(chan->local); > } > return rc; [Severity: High] This is a pre-existing issue, but does the error teardown path in dma_async_device_register() completely release the IDAs? If channel registration fails after registering at least one channel, the error path triggers with device->chancnt > 0. This bypasses the earlier block that calls ida_free(&dma_ida, device->dev_id). Furthermore, this loop unregisters previously successful channels but appears to miss calling ida_free(&device->chan_ida, chan->chan_id). Might this permanently leak the device ID and successfully registered channel IDs? > diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c > index 64cedef1050ae..e36111e8a76dc 100644 > --- a/drivers/dma/tegra186-gpc-dma.c > +++ b/drivers/dma/tegra186-gpc-dma.c > @@ -1537,7 +1537,7 @@ static int tegra_dma_probe(struct platform_device *pdev) > * the channels available and registered for the DMA device are used. > */ > list_for_each_entry(chan, &tdma->dma_dev.channels, device_node) { > - chdev = &chan->dev->device; > + chdev = &chan->chan_dev->device; > tdc = to_tegra_dma_chan(chan); > > if (use_iommu_map) { [Severity: High] This is a pre-existing issue, but is it safe to modify fundamental device properties after the device has already been published to the system? In tegra_dma_probe(), dmaenginem_async_device_register() is called prior to this loop, which globally registers the channel devices via device_register(). Here in the loop, when the devices are already visible to sysfs, udev, and the driver core, chdev->bus is modified and the IOMMU is configured. Could this create a race condition where the driver core or userspace might interact with a partially initialized device? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
