Messages in this thread | | | Subject | Re: Bad kfree of dma_parms in v5.7-rc5 | From | Tomi Valkeinen <> | Date | Wed, 20 May 2020 15:43:36 +0300 |
| |
On 20/05/2020 12:22, Marek Szyprowski wrote: > Hi Tomi, > > On 20.05.2020 11:18, Tomi Valkeinen wrote: >> On 20/05/2020 12:13, Marek Szyprowski wrote: >>> On 20.05.2020 11:00, Tomi Valkeinen wrote: >>>> Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: >>>> platform: Initialize dma_parms for platform devices") v5.7-rc5 causes >>>> at least some v4l2 platform drivers to break when freeing resources. >>>> >>>> E.g. drivers/media/platform/ti-vpe/cal.c uses >>>> vb2_dma_contig_set_max_seg_size() and >>>> vb2_dma_contig_clear_max_seg_size() to manage the dma_params, and >>>> similar pattern is seen in other drivers too. >>>> >>>> After 9495b7e92f716ab2, vb2_dma_contig_set_max_seg_size() will not >>>> allocate anything, but vb2_dma_contig_clear_max_seg_size() will still >>>> kfree the dma_params. >>>> >>>> I'm not sure what's the proper fix here. A flag somewhere to indicate >>>> that vb2_dma_contig_set_max_seg_size() did allocate, and thus >>>> vb2_dma_contig_clear_max_seg_size() must free? >>>> >>>> Or drop the kzalloc and kfree totally, if dma_params is now supposed >>>> to always be there? >>> >>> Thanks for reporting this issue! >>> >>> Once the mentioned commit has been merged, the code should assume that >>> the platform devices does have a struct dma_params allocated, so the >>> proper fix is to alloc dma_params only if the bus is not a platform bus: >>> >>> if (!dev_is_platform(dev) && !dev->dma_parms) { >>> dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); >>> >>> same check for the free path. >> >> There is also "amba: Initialize dma_parms for amba devices". And the >> commit message says PCI devices do this too. >> >> Guessing this based on the device type doesn't sound like a good idea >> to me. > > Indeed. Then replace the allocation with a simple check for NULL > dma_parms and return an error in such case. This should be enough for > v5.8. Later we can simply get rid of those helpers and inline setting > max segment size directly to the drivers.
Is that valid either? Then we assume that dma_parms is always set up by someone else. That's true for platform devices and apparently some other devices, but is it true for all devices now?
Tomi
-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
| |