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

[Lepton Wu]

Actually the reason why I need this CL:

In multi plane patch I'd like to only mmap once for different planes
of same buffer. So actually I need some way
to reuse same mmap for different planes. Then it's natural to have
this CL.  The fix to leak is a side effect of this CL.
dt_unmap still works with this CL,  if user call dt_map for single
plane buffer multiple times,  they will get same pointer, and if they
call dt_unmap
,  with this CL, it still get unmapped.

On Wed, Mar 7, 2018 at 7:14 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 6 March 2018 at 22:43, 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.
>>
>> Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
>> Signed-off-by: Lepton Wu <lepton at chromium.org>
>> ---
>>  .../winsys/sw/kms-dri/kms_dri_sw_winsys.c     | 26 ++++++++++++++-----
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> index 22e1c936ac5..30343222470 100644
>> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> @@ -70,6 +70,7 @@ struct kms_sw_displaytarget
>>
>>     uint32_t handle;
>>     void *mapped;
>> +   void *ro_mapped;
>>
>>     int ref_count;
>>     struct list_head link;
>> @@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>>     if (kms_sw_dt->ref_count > 0)
>>        return;
>>
>> +   if (kms_sw_dt->ro_mapped)
>> +      munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
>> +   if (kms_sw_dt->mapped)
>> +      munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>> +
> I'm not 100% sure about this. There's a reason why dt_unmap exists.
>
> Skimming through the existing code [1] - there's a handful of cases
> that indicate the leaks you're hitting.
> I'd look into addressing those properly because as-is this looks alike
> a duck-taping the problem.
>
> With the hunk gone the patch is
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>
> -Emil
>
> [1] $ git grep -20 -w displaytarget_[a-z]*map