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

[Emil Velikov]

On 13 March 2018 at 11:46, Tomasz Figa <tfiga at chromium.org> wrote:
> On Thu, Mar 8, 2018 at 7:39 AM, 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     | 21 ++++++++++++-------
>>  1 file changed, 14 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..7fc40488c2e 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;
>> @@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
>>        return NULL;
>>
>>     prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
>> -   kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
>> -                            kms_sw->fd, map_req.offset);
>> -
>> -   if (kms_sw_dt->mapped == MAP_FAILED)
>> -      return NULL;
>> +   void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
>> +   if (!*ptr) {
>> +      void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
>> +                       kms_sw->fd, map_req.offset);
>> +      if (tmp == MAP_FAILED)
>> +         return NULL;
>> +      *ptr = tmp;
>> +   }
>>
>>     DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
>> -         kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
>> +         kms_sw_dt->handle, kms_sw_dt->size, *ptr);
>>
>> -   return kms_sw_dt->mapped;
>> +   return *ptr;
>>  }
>>
>>  static struct kms_sw_displaytarget *
>> @@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
>>     struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
>>
>>     DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
>> +   DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
>>
>>     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;
>>  }
>
> If user calls map twice, wouldn't they also call unmap twice?
> Moreover, wouldn't the pointer be expected to be still valid between
> first and second unmap?
>
> The answer obviously depends on how the API is designed, but i feels
> really weird being asymmetrical like that. Typically the mapping would
> be refcounted and maps would have to be balanced with unmaps to free
> the mapping.
>
Valid points.

If you guys prefer we could land 2/2 (multiplane support), since it
has no dependency of the mapping work.
And polish out ro/rw mappings (even the leaks) at later stage, as time permits?

-Emil