Re: [PATCH v10 0/4] drm: update locking for modesetting

[Desmond Cheong Zhi Xi]

On 31/8/21 3:24 am, Desmond Cheong Zhi Xi wrote:
> Sorry for the noise, rebasing on top of drm-misc-next. Please ignore the
> v9 series.
>
> Hi,
>
> I updated the patch set with some suggestions by Daniel Vetter, and
> dropped the patches after patch 4 so that we can stick the landing for
> avoiding races with modesetting rights before dealing with the tricky
> spinlock.
>
> Overall, this series fixes races with modesetting rights, and converts
> drm_device.master_mutex into master_rwsem.
>
> - Patch 1: Fix a potential null ptr dereference in drm_master_release
>
> - Patch 2: Convert master_mutex into rwsem (avoids creating a new lock)
>
> - Patch 3: Update global mutex locking in the ioctl handler (avoids
> deadlock when grabbing read lock on master_rwsem in drm_ioctl_kernel)
>
> - Patch 4: Plug races with drm modesetting rights
>
> v9 -> v10:
> - Rebase on top of drm-misc-next, caught by Intel-gfx CI
>
> v8 -> v9 (suggested by Daniel Vetter):
> - Drop patches 5-7 to handle it in another series
> - Add the appropriate Fixes: tag for the null ptr dereference fix
> (patch 1)
> - Create a locked_ioctl bool to clarify locking/unlocking patterns in
> the ioctl handler (patch 3)
> - Clarify the kernel doc for master_rwsem (patch 4)
>
> v7 -> v8:
> - Avoid calling drm_lease_held in drm_mode_setcrtc and
> drm_wait_vblank_ioctl, caught by Intel-gfx CI
>
> v6 -> v7:
> - Export __drm_mode_object_find for loadable modules, caught by the
> Intel-gfx CI
>
> v5 -> v6:
> - Fix recursive locking on master_rwsem, caught by the Intel-gfx CI
>
> v4 -> v5:
> - Avoid calling drm_file_get_master while holding on to the modeset
> mutex, caught by the Intel-gfx CI
>
> v3 -> v4 (suggested by Daniel Vetter):
> - Drop a patch that added an unnecessary master_lookup_lock in
> drm_master_release
> - Drop a patch that addressed a non-existent race in
> drm_is_current_master_locked
> - Remove fixes for non-existent null ptr dereferences
> - Protect drm_master.magic_map,unique{_len} with master_rwsem instead of
> master_lookup_lock
> - Drop the patch that moved master_lookup_lock into struct drm_device
> - Drop a patch to export task_work_add
> - Revert the check for the global mutex in the ioctl handler to use
> drm_core_check_feature instead of drm_dev_needs_global_mutex
> - Push down master_rwsem locking for selected ioctls to avoid lock
> hierarchy inversions, and to allow us to hold write locks on
> master_rwsem instead of flushing readers
> - Remove master_lookup_lock by replacing it with master_rwsem
>
> v2 -> v3:
> - Unexport drm_master_flush, as suggested by Daniel Vetter.
> - Merge master_mutex and master_rwsem, as suggested by Daniel Vetter.
> - Export task_work_add, reported by kernel test robot.
> - Make master_flush static, reported by kernel test robot.
> - Move master_lookup_lock into struct drm_device.
> - Add a missing lock on master_lookup_lock in drm_master_release.
> - Fix a potential race in drm_is_current_master_locked.
> - Fix potential null ptr dereferences in drm_{auth, ioctl}.
> - Protect magic_map,unique{_len} with  master_lookup_lock.
> - Convert master_mutex into a rwsem.
> - Update global mutex locking in the ioctl handler.
>
> v1 -> v2 (suggested by Daniel Vetter):
> - Address an additional race when drm_open runs.
> - Switch from SRCU to rwsem to synchronise readers and writers.
> - Implement drm_master_flush with task_work so that flushes can be
> queued to run before returning to userspace without creating a new
> DRM_MASTER_FLUSH ioctl flag.
>
> Best wishes,
> Desmond
>
> Desmond Cheong Zhi Xi (4):
>    drm: fix null ptr dereference in drm_master_release
>    drm: convert drm_device.master_mutex into a rwsem
>    drm: lock drm_global_mutex earlier in the ioctl handler
>    drm: avoid races with modesetting rights
>
>   drivers/gpu/drm/drm_auth.c    | 39 ++++++++++++++++------------
>   drivers/gpu/drm/drm_debugfs.c |  4 +--
>   drivers/gpu/drm/drm_drv.c     |  3 +--
>   drivers/gpu/drm/drm_file.c    |  6 ++---
>   drivers/gpu/drm/drm_ioctl.c   | 49 ++++++++++++++++++++++-------------
>   drivers/gpu/drm/drm_lease.c   | 35 +++++++++++++++++--------
>   include/drm/drm_auth.h        |  6 ++---
>   include/drm/drm_device.h      | 16 +++++++++---
>   include/drm/drm_file.h        | 12 ++++-----
>   9 files changed, 104 insertions(+), 66 deletions(-)

Hi Daniel,

Just pinging so this doesn't get buried, though I guess it's also a busy 
merge window. Any thoughts on the series as it is? Tests seemed to have 
passed with the Intel-gfx CI [1].

Not sure if I can set up the CI to do otherwise, but I think this series 
has to go in before I can test new patches to remove the 
drm_file.master_lookup_lock spinlock.

As always, thank you for your time.

Link: https://patchwork.freedesktop.org/series/93864/ [1]

Best wishes,
Desmond