Re: [Intel-gfx] [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find
On Thu, Aug 26, 2021 at 10:01:20AM +0800, Desmond Cheong Zhi Xi wrote:
> __drm_mode_object_find checks if the given drm file holds the required
> lease on a object by calling _drm_lease_held. _drm_lease_held in turn
> uses drm_file_get_master to access drm_file.master.
>
> However, in a future patch, the drm_file.master_lookup_lock in
> drm_file_get_master will be replaced by drm_device.master_rwsem. This
> is an issue for two reasons:
>
> 1. master_rwsem is sometimes already held when __drm_mode_object_find
> is called, which leads to recursive locks on master_rwsem
>
> 2. drm_mode_object_find is sometimes called with the modeset_mutex
> held, which leads to an inversion of the master_rwsem -->
> modeset_mutex lock hierarchy
>
> To fix this, we make __drm_mode_object_find the locked version of
> drm_mode_object_find, and wrap calls to __drm_mode_object_find with
> locks on master_rwsem. This allows us to safely access drm_file.master
> in _drm_lease_held (__drm_mode_object_find is its only caller) without
> the use of drm_file_get_master.
>
> Functions that already lock master_rwsem are modified to call
> __drm_mode_object_find, whereas functions that haven't locked
> master_rwsem should call drm_mode_object_find. These two options
> allow us to grab master_rwsem before modeset_mutex (such as in
> drm_mode_get_obj_get_properties_ioctl).
>
> This new rule requires more extensive changes to three functions:
> drn_connector_lookup, drm_crtc_find, and drm_plane_find. These
> functions are only sometimes called with master_rwsem held. Hence, we
> have to further split them into locked and unlocked versions that call
> __drm_mode_object_find and drm_mode_object_find respectively.
I think approach looks good, but the naming isn't so great. Usually __
prefix means "do not call directly, this is only exported for static
inline and other helpers". For these the usual rule is to add a _locked or
_unlocked suffix. I'd leave the normal _find functions as-is (since those
take the lock) themselves, and annotate the _locked ones.
Also same for the other lookup helpers.
>
> Signed-off-by: Desmond Cheong Zhi Xi
> ---
> drivers/gpu/drm/drm_atomic_uapi.c| 7 ++---
> drivers/gpu/drm/drm_color_mgmt.c | 2 +-
> drivers/gpu/drm/drm_crtc.c | 5 ++--
> drivers/gpu/drm/drm_framebuffer.c| 2 +-
> drivers/gpu/drm/drm_lease.c | 21 +--
> drivers/gpu/drm/drm_mode_object.c| 28 +---
> drivers/gpu/drm/drm_plane.c | 8 +++---
> drivers/gpu/drm/drm_property.c | 6 ++---
> drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
> include/drm/drm_connector.h | 23
> include/drm/drm_crtc.h | 22 +++
> include/drm/drm_mode_object.h| 3 +++
> include/drm/drm_plane.h | 20 ++
> 15 files changed, 118 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 909f31833181..cda9a501cf74 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -557,7 +557,7 @@ static int drm_atomic_plane_set_property(struct drm_plane
> *plane,
> return -EINVAL;
>
> } else if (property == config->prop_crtc_id) {
> - struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
> + struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
>
> if (val && !crtc)
> return -EACCES;
> @@ -709,7 +709,7 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
> int ret;
>
> if (property == config->prop_crtc_id) {
> - struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
> + struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
>
> if (val && !crtc)
> return -EACCES;
> @@ -1385,7 +1385,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> goto out;
> }
>
> - obj = drm_mode_object_find(dev, file_priv, obj_id,
> DRM_MODE_OBJECT_ANY);
> + obj = __drm_mode_object_find(dev, file_priv, obj_id,
> + DRM_MODE_OBJECT_ANY);
> if (!obj) {
> ret = -ENOENT;
> goto out;
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> b/drivers/gpu/drm/drm_color_mgmt.c
> index bb14f488c8f6..9dcb2ccca3ab 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -365,7 +365,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return -EO
[Intel-gfx] [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find
__drm_mode_object_find checks if the given drm file holds the required
lease on a object by calling _drm_lease_held. _drm_lease_held in turn
uses drm_file_get_master to access drm_file.master.
However, in a future patch, the drm_file.master_lookup_lock in
drm_file_get_master will be replaced by drm_device.master_rwsem. This
is an issue for two reasons:
1. master_rwsem is sometimes already held when __drm_mode_object_find
is called, which leads to recursive locks on master_rwsem
2. drm_mode_object_find is sometimes called with the modeset_mutex
held, which leads to an inversion of the master_rwsem -->
modeset_mutex lock hierarchy
To fix this, we make __drm_mode_object_find the locked version of
drm_mode_object_find, and wrap calls to __drm_mode_object_find with
locks on master_rwsem. This allows us to safely access drm_file.master
in _drm_lease_held (__drm_mode_object_find is its only caller) without
the use of drm_file_get_master.
Functions that already lock master_rwsem are modified to call
__drm_mode_object_find, whereas functions that haven't locked
master_rwsem should call drm_mode_object_find. These two options
allow us to grab master_rwsem before modeset_mutex (such as in
drm_mode_get_obj_get_properties_ioctl).
This new rule requires more extensive changes to three functions:
drn_connector_lookup, drm_crtc_find, and drm_plane_find. These
functions are only sometimes called with master_rwsem held. Hence, we
have to further split them into locked and unlocked versions that call
__drm_mode_object_find and drm_mode_object_find respectively.
Signed-off-by: Desmond Cheong Zhi Xi
---
drivers/gpu/drm/drm_atomic_uapi.c| 7 ++---
drivers/gpu/drm/drm_color_mgmt.c | 2 +-
drivers/gpu/drm/drm_crtc.c | 5 ++--
drivers/gpu/drm/drm_framebuffer.c| 2 +-
drivers/gpu/drm/drm_lease.c | 21 +--
drivers/gpu/drm/drm_mode_object.c| 28 +---
drivers/gpu/drm/drm_plane.c | 8 +++---
drivers/gpu/drm/drm_property.c | 6 ++---
drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
include/drm/drm_connector.h | 23
include/drm/drm_crtc.h | 22 +++
include/drm/drm_mode_object.h| 3 +++
include/drm/drm_plane.h | 20 ++
15 files changed, 118 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 909f31833181..cda9a501cf74 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -557,7 +557,7 @@ static int drm_atomic_plane_set_property(struct drm_plane
*plane,
return -EINVAL;
} else if (property == config->prop_crtc_id) {
- struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
+ struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
if (val && !crtc)
return -EACCES;
@@ -709,7 +709,7 @@ static int drm_atomic_connector_set_property(struct
drm_connector *connector,
int ret;
if (property == config->prop_crtc_id) {
- struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
+ struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
if (val && !crtc)
return -EACCES;
@@ -1385,7 +1385,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
goto out;
}
- obj = drm_mode_object_find(dev, file_priv, obj_id,
DRM_MODE_OBJECT_ANY);
+ obj = __drm_mode_object_find(dev, file_priv, obj_id,
+DRM_MODE_OBJECT_ANY);
if (!obj) {
ret = -ENOENT;
goto out;
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index bb14f488c8f6..9dcb2ccca3ab 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -365,7 +365,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
if (!drm_core_check_feature(dev, DRIVER_MODESET))
return -EOPNOTSUPP;
- crtc = drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
+ crtc = __drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
if (!crtc)
return -ENOENT;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 26a77a735905..b1279bb3fa61 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -656,7 +656,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
if (crtc_req->x & 0x || crtc_req->y & 0x)
return -ERANGE;
- crtc = drm_crtc_find(dev, file_priv, crtc_req->crtc_i
