We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it.
Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping.
References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/ Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org --- drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx);
+ might_fault(); + ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK)
Am 10.07.24 um 11:31 schrieb Daniel Vetter:
We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it.
Mhm, lockdep should be able to deduce that when there might be faults under the dma_resv lock there might also be faults under the drm_modeset_lock.
Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping.
References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/ Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
On the other hand pointing it out explicitly doesn't hurts us at all, so Reviewed-by: Christian König christian.koenig@amd.com.
Regards, Christian.
drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx);
might_fault();- ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK)
On Wed, 10 Jul 2024 at 13:39, Christian König christian.koenig@amd.com wrote:
Am 10.07.24 um 11:31 schrieb Daniel Vetter:
We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it.
Mhm, lockdep should be able to deduce that when there might be faults under the dma_resv lock there might also be faults under the drm_modeset_lock.
You're not allowed to take a fault under dma_resv, because drivers might need to take that lock to handle faults. So unfortunately in our combined lockdep priming, there really seems to be no chain yet that teaches about faults possibly happening while holding drm_modeset_lock. -Sima
Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping.
References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/ Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
On the other hand pointing it out explicitly doesn't hurts us at all, so Reviewed-by: Christian König christian.koenig@amd.com.
Regards, Christian.
drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx);
might_fault();ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK)
Am 10.07.24 um 13:58 schrieb Daniel Vetter:
On Wed, 10 Jul 2024 at 13:39, Christian König christian.koenig@amd.com wrote:
Am 10.07.24 um 11:31 schrieb Daniel Vetter:
We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it.
Mhm, lockdep should be able to deduce that when there might be faults under the dma_resv lock there might also be faults under the drm_modeset_lock.
You're not allowed to take a fault under dma_resv, because drivers might need to take that lock to handle faults. So unfortunately in our combined lockdep priming, there really seems to be no chain yet that teaches about faults possibly happening while holding drm_modeset_lock.
Ah, of course! You are right, it was just the other way around.
Thanks, Christian.
-Sima
Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping.
References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/ Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
On the other hand pointing it out explicitly doesn't hurts us at all, so Reviewed-by: Christian König christian.koenig@amd.com.
Regards, Christian.
drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx);
might_fault();ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK)
On Wed, Jul 10, 2024 at 02:40:04PM +0200, Christian König wrote:
Am 10.07.24 um 13:58 schrieb Daniel Vetter:
On Wed, 10 Jul 2024 at 13:39, Christian König christian.koenig@amd.com wrote:
Am 10.07.24 um 11:31 schrieb Daniel Vetter:
We already teach lockdep that dma_resv nests within drm_modeset_lock, but there's a lot more: All drm kms ioctl rely on being able to put/get_user while holding modeset locks, so we really need a might_fault in there too to complete the picture. Add it.
Mhm, lockdep should be able to deduce that when there might be faults under the dma_resv lock there might also be faults under the drm_modeset_lock.
You're not allowed to take a fault under dma_resv, because drivers might need to take that lock to handle faults. So unfortunately in our combined lockdep priming, there really seems to be no chain yet that teaches about faults possibly happening while holding drm_modeset_lock.
Ah, of course! You are right, it was just the other way around.
Applied to drm-misc-next, thanks for your review. -Sima
Thanks, Christian.
-Sima
Motivated by a syzbot report that blew up on bcachefs doing an unconditional console_lock way deep in the locking hierarchy, and lockdep only noticing the depency loop in a drm ioctl instead of much earlier. This annotation will make sure such issues have a much harder time escaping.
References: https://lore.kernel.org/dri-devel/00000000000073db8b061cd43496@google.com/ Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
On the other hand pointing it out explicitly doesn't hurts us at all, so Reviewed-by: Christian König christian.koenig@amd.com.
Regards, Christian.
drivers/gpu/drm/drm_mode_config.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 568972258222..37d2e0a4ef4b 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -456,6 +456,8 @@ int drmm_mode_config_init(struct drm_device *dev) if (ret == -EDEADLK) ret = drm_modeset_backoff(&modeset_ctx);
might_fault();ww_acquire_init(&resv_ctx, &reservation_ww_class); ret = dma_resv_lock(&resv, &resv_ctx); if (ret == -EDEADLK)
linaro-mm-sig@lists.linaro.org
-
Christian König -
Daniel Vetter