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

[Marek Szyprowski]

Hi Ulf,

On 20.05.2020 15:12, Ulf Hansson wrote:
> + Greg
>
> On Wed, 20 May 2020 at 14:54, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
>> 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.
> That seems like a good idea, in the long run.
>
>>> 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.
> Thanks for reporting and for looking into this. I apologize for the mess!
>
> There is one case, where the above solution could be a problem (unless
> I am wrong). That is, s5p_mfc_configure_2port_memory() that calls
> s5p_mfc_alloc_memdev(), which allocates/initializes an internal struct
> *device. Thus, this doesn't have the dev->dma_parms
> allocated/assigned.
Indeed, this one will fail.
> In other words, we would need to manage alloc/free for the
> dev->dma_parms to have a complete fix. Maybe in
> s5p_mfc_configure|unconfigure_2port_memory()!?
That would be the best place to allocate it.
> Additionally, I think reverting the offending commit, as discussed
> above, could cause even more issues, as it's even included for
> v5.6-stable kernels. I will go through all cases, more carefully this
> time, of how ->dma_parms is managed, to be sure there are no more
> conflicting cases.

I've already posted a fix for ExynosDRM driver, which is also affected: 
https://patchwork.kernel.org/patch/11559965/


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