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

[Emil Velikov]

On 19 March 2018 at 20:00, Lepton Wu <lepton at chromium.org> wrote:
> On Mon, Mar 19, 2018 at 10: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)
> Thanks, will send out new version to address this comment. One more question:
> I have 2 CL, the later 1 actually depends on the first one, and you
> can see, the actual
> patch of the later one doesn't change across versions. Should I also
> increase version
> number for later one? If I don't increase version for later one, this
> will looks inconsistent: v6 [1/2]  and
> v3 [2/2]...
Feel free to say consistent (v6 [1/2], v6 [2/2]) and if there's no v6
changes for 2/2 then there's nothing to add.
Either way - it's a small nitpick.

>> ...
>>
>>> @@ -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.
> How about just keeping the DEBUG_PRINT check?

Sure thing.

Thanks
Emil