lkml.org 
[lkml]   [2020]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: Bad kfree of dma_parms in v5.7-rc5
    From
    Date
    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

    \
     
     \ /
      Last update: 2020-05-20 14:55    [W:3.566 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site