Re: Bad kfree of dma_parms in v5.7-rc5

[Tomi Valkeinen]

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