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:     Tomi Valkeinen <tomi.valkeinen@ti.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>
From:   Marek Szyprowski <m.szyprowski@samsung.com>
Message-ID: <e9674719-0c86-63be-04a3-ee98bd884901@samsung.com>
Date:   Wed, 20 May 2020 11:13:24 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
        Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <a9df7155-dd7a-752b-6d1c-3426837756b1@ti.com>
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20200520090109eucas1p17270805f81f6958cd5084a7b910efc6c@eucas1p1.samsung.com>
        <a9df7155-dd7a-752b-6d1c-3426837756b1@ti.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Tomi,

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.

Would you like to send a patch for that?

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