[PATCH] drm: Reorder set_property_atomic to avoid returning with an active ww_ctx

[PATCH] drm: Reorder set_property_atomic to avoid returning with an active ww_ctx

[Chris Wilson]

Delay the drm_modeset_acquire_init() until after we check for an
allocation failure so that we can return immediately upon error without
having to unwind.

WARNING: lock held when returning to user space!
4.20.0+ #174 Not tainted
------------------------------------------------
syz-executor556/8153 is leaving the kernel with locks still held!
1 lock held by syz-executor556/8153:
  #0: 000000005100c85c (crtc_ww_class_acquire){+.+.}, at:
set_property_atomic+0xb3/0x330 drivers/gpu/drm/drm_mode_object.c:462

Reported-by: syzbot+6ea337c427f5083ebdf2@syzkaller.appspotmail.com
Fixes: 144a7999d633 ("drm: Handle properties in the core for atomic drivers")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: <stable@vger.kernel.org> # v4.14+
---
 drivers/gpu/drm/drm_mode_object.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index bb1dd46496cd..a9005c1c2384 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -459,12 +459,13 @@ static int set_property_atomic(struct drm_mode_object *obj,
 	struct drm_modeset_acquire_ctx ctx;
 	int ret;
 
-	drm_modeset_acquire_init(&ctx, 0);
-
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
 		return -ENOMEM;
+
+	drm_modeset_acquire_init(&ctx, 0);
 	state->acquire_ctx = &ctx;
+
 retry:
 	if (prop == state->dev->mode_config.dpms_property) {
 		if (obj->type != DRM_MODE_OBJECT_CONNECTOR) {
-- 
2.20.1

Re: [PATCH] drm: Reorder set_property_atomic to avoid returning with an active ww_ctx

[Maarten Lankhorst]

Op 30-12-2018 om 13:28 schreef Chris Wilson:
> Delay the drm_modeset_acquire_init() until after we check for an
> allocation failure so that we can return immediately upon error without
> having to unwind.
>
> WARNING: lock held when returning to user space!
> 4.20.0+ #174 Not tainted
> ------------------------------------------------
> syz-executor556/8153 is leaving the kernel with locks still held!
> 1 lock held by syz-executor556/8153:
>   #0: 000000005100c85c (crtc_ww_class_acquire){+.+.}, at:
> set_property_atomic+0xb3/0x330 drivers/gpu/drm/drm_mode_object.c:462
>
> Reported-by: syzbot+6ea337c427f5083ebdf2@syzkaller.appspotmail.com
> Fixes: 144a7999d633 ("drm: Handle properties in the core for atomic drivers")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: <stable@vger.kernel.org> # v4.14+
> ---
>  drivers/gpu/drm/drm_mode_object.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index bb1dd46496cd..a9005c1c2384 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -459,12 +459,13 @@ static int set_property_atomic(struct drm_mode_object *obj,
>  	struct drm_modeset_acquire_ctx ctx;
>  	int ret;
>  
> -	drm_modeset_acquire_init(&ctx, 0);
> -
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state)
>  		return -ENOMEM;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
>  	state->acquire_ctx = &ctx;
> +
>  retry:
>  	if (prop == state->dev->mode_config.dpms_property) {
>  		if (obj->type != DRM_MODE_OBJECT_CONNECTOR) {

Woops only now see you did the same.. :)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Re: [PATCH] drm: Reorder set_property_atomic to avoid returning with an active ww_ctx

[Chris Wilson]

Quoting Maarten Lankhorst (2019-01-03 09:03:27)
> Op 30-12-2018 om 13:28 schreef Chris Wilson:
> > Delay the drm_modeset_acquire_init() until after we check for an
> > allocation failure so that we can return immediately upon error without
> > having to unwind.
> >
> > WARNING: lock held when returning to user space!
> > 4.20.0+ #174 Not tainted
> > ------------------------------------------------
> > syz-executor556/8153 is leaving the kernel with locks still held!
> > 1 lock held by syz-executor556/8153:
> >   #0: 000000005100c85c (crtc_ww_class_acquire){+.+.}, at:
> > set_property_atomic+0xb3/0x330 drivers/gpu/drm/drm_mode_object.c:462
> >
> > Reported-by: syzbot+6ea337c427f5083ebdf2@syzkaller.appspotmail.com
> > Fixes: 144a7999d633 ("drm: Handle properties in the core for atomic drivers")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: <stable@vger.kernel.org> # v4.14+
> > ---
> >  drivers/gpu/drm/drm_mode_object.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > index bb1dd46496cd..a9005c1c2384 100644
> > --- a/drivers/gpu/drm/drm_mode_object.c
> > +++ b/drivers/gpu/drm/drm_mode_object.c
> > @@ -459,12 +459,13 @@ static int set_property_atomic(struct drm_mode_object *obj,
> >       struct drm_modeset_acquire_ctx ctx;
> >       int ret;
> >  
> > -     drm_modeset_acquire_init(&ctx, 0);
> > -
> >       state = drm_atomic_state_alloc(dev);
> >       if (!state)
> >               return -ENOMEM;
> > +
> > +     drm_modeset_acquire_init(&ctx, 0);
> >       state->acquire_ctx = &ctx;
> > +
> >  retry:
> >       if (prop == state->dev->mode_config.dpms_property) {
> >               if (obj->type != DRM_MODE_OBJECT_CONNECTOR) {
> 
> Woops only now see you did the same.. :)

I'm impressed that syszbot managed to hit it! Afaict, it is only a
debugging faux pas with no real user impact, so perhaps the stable is
overkill.
 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Ta, pushed to drm-misc-next
-Chris

Re: [PATCH] drm: Reorder set_property_atomic to avoid returning with an active ww_ctx

[Daniel Vetter]

On Thu, Jan 03, 2019 at 10:16:54AM +0000, Chris Wilson wrote:
> Quoting Maarten Lankhorst (2019-01-03 09:03:27)
> > Op 30-12-2018 om 13:28 schreef Chris Wilson:
> > > Delay the drm_modeset_acquire_init() until after we check for an
> > > allocation failure so that we can return immediately upon error without
> > > having to unwind.
> > >
> > > WARNING: lock held when returning to user space!
> > > 4.20.0+ #174 Not tainted
> > > ------------------------------------------------
> > > syz-executor556/8153 is leaving the kernel with locks still held!
> > > 1 lock held by syz-executor556/8153:
> > >   #0: 000000005100c85c (crtc_ww_class_acquire){+.+.}, at:
> > > set_property_atomic+0xb3/0x330 drivers/gpu/drm/drm_mode_object.c:462
> > >
> > > Reported-by: syzbot+6ea337c427f5083ebdf2@syzkaller.appspotmail.com
> > > Fixes: 144a7999d633 ("drm: Handle properties in the core for atomic drivers")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Sean Paul <sean@poorly.run>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: <stable@vger.kernel.org> # v4.14+
> > > ---
> > >  drivers/gpu/drm/drm_mode_object.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > > index bb1dd46496cd..a9005c1c2384 100644
> > > --- a/drivers/gpu/drm/drm_mode_object.c
> > > +++ b/drivers/gpu/drm/drm_mode_object.c
> > > @@ -459,12 +459,13 @@ static int set_property_atomic(struct drm_mode_object *obj,
> > >       struct drm_modeset_acquire_ctx ctx;
> > >       int ret;
> > >  
> > > -     drm_modeset_acquire_init(&ctx, 0);
> > > -
> > >       state = drm_atomic_state_alloc(dev);
> > >       if (!state)
> > >               return -ENOMEM;
> > > +
> > > +     drm_modeset_acquire_init(&ctx, 0);
> > >       state->acquire_ctx = &ctx;
> > > +
> > >  retry:
> > >       if (prop == state->dev->mode_config.dpms_property) {
> > >               if (obj->type != DRM_MODE_OBJECT_CONNECTOR) {
> > 
> > Woops only now see you did the same.. :)
> 
> I'm impressed that syszbot managed to hit it! Afaict, it is only a
> debugging faux pas with no real user impact, so perhaps the stable is
> overkill.

Yeah, "small allocs can't fail" will make sure this isn't a real world
bug. syzbot uses fault injection stuff to hit these (at least that's what
it did in one of the destilled minimal reproduction cases in some other
very similar report).

> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Ta, pushed to drm-misc-next

So agreed -next makes sense, no fixes (but I'm sure the autoselect will
pick it up anyway, but that one can't be helped).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch