Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: Bad kfree of dma_parms in v5.7-rc5
To:     Marek Szyprowski <m.szyprowski@samsung.com>,
        Linux Media Mailing List <linux-media@vger.kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>
References: <CGME20200520090109eucas1p17270805f81f6958cd5084a7b910efc6c@eucas1p1.samsung.com>
 <a9df7155-dd7a-752b-6d1c-3426837756b1@ti.com>
 <e9674719-0c86-63be-04a3-ee98bd884901@samsung.com>
 <f3c58dcd-b806-95ef-2434-3084e65e1afb@ti.com>
 <e3fa0b35-7cca-1e37-c2fa-63cc07e6bfda@samsung.com>
From:   Tomi Valkeinen <tomi.valkeinen@ti.com>
Message-ID: <227465a5-c6e6-5b4d-abbd-7789727843a6@ti.com>
Date:   Wed, 20 May 2020 15:43:36 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <e3fa0b35-7cca-1e37-c2fa-63cc07e6bfda@samsung.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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