[Mesa-dev] [PATCH v6 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

On Tue, Mar 20, 2018 at 9:26 AM, Tomasz Figa <tfiga at chromium.org> wrote:
> On Wed, Mar 21, 2018 at 12:58 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 20 March 2018 at 14:24, Tomasz Figa <tfiga at chromium.org> wrote:
>>> On Tue, Mar 20, 2018 at 10:44 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> On 20 March 2018 at 04:40, Tomasz Figa <tfiga at chromium.org> wrote:
>>>>> On Tue, Mar 20, 2018 at 2:55 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>> Hi Lepton,
>>>>>>
>>>>>> On 19 March 2018 at 17:33, Lepton Wu <lepton at chromium.org> wrote:
>>>>>>> If user calls map twice for kms_sw_displaytarget, the first mapped
>>>>>>> buffer could get leaked. Instead of calling mmap every time, just
>>>>>>> reuse previous mapping. Since user could map same displaytarget with
>>>>>>> different flags, we have to keep two different pointers, one for rw
>>>>>>> mapping and one for ro mapping. Also introduce reference count for
>>>>>>> mapped buffer so we can unmap them at right time.
>>>>>>>
>>>>>>> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>>>>>>> Reviewed-by: Tomasz Figa <tfiga at chromium.org>
>>>>>>> Signed-off-by: Lepton Wu <lepton at chromium.org>
>>>>>>
>>>>>> Nit: normally it's a good idea to have brief revision log when sending
>>>>>> new version:
>>>>>> v2:
>>>>>>  - split from larger patch (Emil)
>>>>>> v3:
>>>>>>  - remove munmap w/a from dt_destory(Emil)
>>>>>> ...
>>>>>>
>>>>>>> @@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>>>>>>>     if (kms_sw_dt->ref_count > 0)
>>>>>>>        return;
>>>>>>>
>>>>>>> +   if (kms_sw_dt->map_count > 0) {
>>>>>>> +      DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n", kms_sw_dt->handle);
>>>>>>> +      munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>>>>>>> +      kms_sw_dt->mapped = NULL;
>>>>>>> +      munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
>>>>>>> +      kms_sw_dt->ro_mapped = NULL;
>>>>>>> +   }
>>>>>>> +
>>>>>> I could swear this workaround was missing in earlier revisions. I
>>>>>> don't see anything in Tomasz' reply that suggesting we should bring it
>>>>>> back?
>>>>>> AFAICT the added refcounting makes no difference - the driver isn't
>>>>>> cleaning up after itself.
>>>>>>
>>>>>> Am I missing something?
>>>>>
>>>>> I think this is actually consistent with what other winsys
>>>>> implementations do. They free the map (or shadow malloc/shm buffer) in
>>>>> _destroy() callback, so we should probably do the same.
>>>>>
>>>> Looking at the SW winsys - none of them seem to unmap at destroy time.
>>>> Perhaps you meant that the HW ones do?
>>>
>>> dri:
>>> https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/dri/dri_sw_winsys.c#n128
>>>
>>> gdi:
>>> https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/gdi/gdi_sw_winsys.c#n116
>>>
>>> hgl:
>>> https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/hgl/hgl_sw_winsys.c#n152
>>>
>>> xlib:
>>> https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n260
>>> https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n271
>>>
>>> The don't do real mapping - they all work on locally allocated memory.
>>> However, after destroy, no resources are leaked and the pointers
>>> returned from _map() are not valid anymore.
>>>
>> As mentioned before - zero objections against changing that, but keep
>> it separate patch.
>> Pretty please?
>
> SGTM.
Thanks all for review. Is there anything else missing for getting this
committed?
>
> Best regards,
> Tomasz