Re: [PATCH 6/6] drm: Add four ioctls for managing drm mode object leases [v3]

[Dave Airlie]

On 5 October 2017 at 13:24, Dave Airlie <airlied@xxxxxxxxx> wrote:
> On 5 October 2017 at 13:17, Dave Airlie <airlied@xxxxxxxxx> wrote:
>>> ---
>>>  drivers/gpu/drm/drm_ioctl.c |   4 +
>>>  drivers/gpu/drm/drm_lease.c | 270 ++++++++++++++++++++++++++++++++++++++++++++
>>>  include/drm/drm_lease.h     |  12 ++
>>>  include/uapi/drm/drm.h      |   5 +
>>>  include/uapi/drm/drm_mode.h |  64 +++++++++++
>>>  5 files changed, 355 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>> index a5a259964c7d..0a43e82d3f06 100644
>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>> @@ -657,6 +657,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>>                       DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>         DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
>>>                       DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>> +       DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>>> +       DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>>> +       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>>> +       DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>>>  };
>>>
>>>  #define DRM_CORE_IOCTL_COUNT   ARRAY_SIZE( drm_ioctls )
>>> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
>>> index a8bd4bdd2977..f233d8b488f2 100644
>>> --- a/drivers/gpu/drm/drm_lease.c
>>> +++ b/drivers/gpu/drm/drm_lease.c
>>> @@ -23,6 +23,8 @@
>>>  #define drm_for_each_lessee(lessee, lessor) \
>>>         list_for_each_entry((lessee), &(lessor)->lessees, lessee_list)
>>>
>>> +static uint64_t drm_lease_idr_object;
>>
>> What is this for? ^^
>>
>>> +               ret = idr_alloc(&leases, &drm_lease_idr_object , object_id, object_id + 1, GFP_KERNEL);
>>
>> You can just pass NULL here.
>>
>
> Just read the comment, this smells a bit of black magic, I'll spend
> some time staring at it :-)

I think a comment in here is definitely warranted with what you expect
from the idr interface here.

You seem to be storing the object ids in it from the user, and failing
if you get a duplicate, then later dequeuing the idr.

I'm not sure this an intended use of idr :-), my brain is saying a
bitmap would work just as well, or even why we want/need
deduplication here. If userspace does something stupid does it matter?

Because otherwise you could just memdup_user the array of ids, and
pass that in without the idr stuff.

Dave.