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

[Marek Szyprowski]

Hi Tomi,

On 20.05.2020 14:43, Tomi Valkeinen wrote:
> On 20/05/2020 12:22, Marek Szyprowski wrote:
>> 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?

# git grep vb2_dma_contig_set_max_seg_size | wc -l

18

I've checked all clients of the vb2_dma_contig_set_max_seg_size 
function. There are only 9 drivers, all of them are platform device 
drivers. We don't care about off-tree users, so the proposed approach is 
imho fine.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland