Bad kfree of dma_parms in v5.7-rc5

Bad kfree of dma_parms in v5.7-rc5

[Tomi Valkeinen]

Hi,

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?

Or revert 9495b7e92f716ab2, as it was added so late?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[Marek Szyprowski]

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

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

[Tomi Valkeinen]

Hi Marek,

On 20/05/2020 12:13, Marek Szyprowski wrote:
> 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.

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.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[Tomi Valkeinen]

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

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

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

[Ulf Hansson]

+ Greg

On Wed, 20 May 2020 at 14:54, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> 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.

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.

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()!?

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.

Kind regards
Uffe

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@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/


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

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

[Ulf Hansson]

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

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

[Ulf Hansson]

Marek, Tomi, Greg

On Wed, 20 May 2020 at 17:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> 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
>

FYI:

I have now double checked all cases where ->dma_params are being
allocated/freed. Besides those you (Marek/Tomi) you have found and
sent fixes for (many thanks!)  - I haven't found any additional cases
to worry about.

However, of course there are cleanups and removal of redundant code
that can be made, for some drivers/devices, which are based upon a
platform device. For example, some have their own "struct
device_dma_parameters", such as drivers/dma/dma-axi-dmac.c for
example. This is not a problem, but deserves to be cleaned up. I have
started to prepare patches for it.

Kind regards
Uffe