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;
MIME-Version: 1.0
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>
 <227465a5-c6e6-5b4d-abbd-7789727843a6@ti.com> <29a21e64-a63f-6721-c938-d713488767c1@samsung.com>
 <CAPDyKFq8-JYA_tKZmUZOY3mT-jeoWMHNpdj8SDGkqYmX7jJHVQ@mail.gmail.com> <9f72aeab-8d63-7fc1-d5ff-7d5c4f11012b@samsung.com>
In-Reply-To: <9f72aeab-8d63-7fc1-d5ff-7d5c4f11012b@samsung.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 20 May 2020 17:14:29 +0200
Message-ID: <CAPDyKFpgzYJzWR3mc-4XQDijtHVibDrFXazY0=P7TUs6rvE_hQ@mail.gmail.com>
Subject: Re: Bad kfree of dma_parms in v5.7-rc5
To:     Marek Szyprowski <m.szyprowski@samsung.com>
Cc:     Tomi Valkeinen <tomi.valkeinen@ti.com>,
        Linux Media Mailing List <linux-media@vger.kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 20 May 2020 at 15:28, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Ulf,
>
> On 20.05.2020 15:12, Ulf Hansson wrote:
> > + Greg
> >
> > On Wed, 20 May 2020 at 14:54, Marek Szyprowski <m.szyprowski@samsung.com> 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/

Alright, thanks for helping out!

Please add a fixes/stable tag to it.

Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for
platform devices")
Cc: stable@vger.kernel.org

Kind regards
Uffe