[PATCH] drm/vkms: Fix flush_work() without INIT_WORK().

[PATCH] drm/vkms: Fix flush_work() without INIT_WORK().

[Tetsuo Handa]

syzbot is hitting a lockdep warning [1] because flush_work() is called
without INIT_WORK() after kzalloc() at vkms_atomic_crtc_reset().

Commit 6c234fe37c57627a ("drm/vkms: Implement CRC debugfs API") added
INIT_WORK() to only vkms_atomic_crtc_duplicate_state() side. Assuming
that lifecycle of crc_work is appropriately managed, fix this problem
by adding INIT_WORK() to vkms_atomic_crtc_reset() side.

[1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770

Reported-and-tested-by: syzbot <syzbot+12f1b031b6da017e34f8@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 177bbcb..3c37d8c 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -104,6 +104,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state)
 		return;
+	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
 
 	crtc->state = &vkms_state->base;
 	crtc->state->crtc = crtc;
-- 
1.8.3.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vkms: Fix flush_work() without INIT_WORK().

[Shayenne Moura]

> syzbot is hitting a lockdep warning [1] because flush_work() is called
> without INIT_WORK() after kzalloc() at vkms_atomic_crtc_reset().
>
> Commit 6c234fe37c57627a ("drm/vkms: Implement CRC debugfs API") added
> INIT_WORK() to only vkms_atomic_crtc_duplicate_state() side. Assuming
> that lifecycle of crc_work is appropriately managed, fix this problem
> by adding INIT_WORK() to vkms_atomic_crtc_reset() side.
>
> [1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770
>
> Reported-and-tested-by: syzbot <syzbot+12f1b031b6da017e34f8@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 177bbcb..3c37d8c 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -104,6 +104,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
>         vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>         if (!vkms_state)
>                 return;
> +       INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
>
>         crtc->state = &vkms_state->base;
>         crtc->state->crtc = crtc;
> --
> 1.8.3.1
>

Thank you for your patch!

Reviewed-by: Shayenne Moura <shayenneluzmoura@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vkms: Fix flush_work() without INIT_WORK().

[Sean Paul]

On Sat, Jan 19, 2019 at 01:43:43AM +0900, Tetsuo Handa wrote:
> syzbot is hitting a lockdep warning [1] because flush_work() is called
> without INIT_WORK() after kzalloc() at vkms_atomic_crtc_reset().
> 
> Commit 6c234fe37c57627a ("drm/vkms: Implement CRC debugfs API") added
> INIT_WORK() to only vkms_atomic_crtc_duplicate_state() side. Assuming
> that lifecycle of crc_work is appropriately managed, fix this problem
> by adding INIT_WORK() to vkms_atomic_crtc_reset() side.
> 
> [1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770
> 
> Reported-and-tested-by: syzbot <syzbot+12f1b031b6da017e34f8@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 177bbcb..3c37d8c 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -104,6 +104,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
>  	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>  	if (!vkms_state)
>  		return;
> +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);

Presumably we should migrate/duplicate the crc_work state from the old state
rather than just re-initializing? My context on the crc_work is foggy, but we
should be sure that either:

a) the existing crc work is flushed and inactive, or
b) we migrate the existing crc work to the duplicated state to keep track of it

Sean

>  
>  	crtc->state = &vkms_state->base;
>  	crtc->state->crtc = crtc;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vkms: Fix flush_work() without INIT_WORK().

[Daniel Vetter]

On Fri, Jan 25, 2019 at 09:46:15AM -0500, Sean Paul wrote:
> On Sat, Jan 19, 2019 at 01:43:43AM +0900, Tetsuo Handa wrote:
> > syzbot is hitting a lockdep warning [1] because flush_work() is called
> > without INIT_WORK() after kzalloc() at vkms_atomic_crtc_reset().
> > 
> > Commit 6c234fe37c57627a ("drm/vkms: Implement CRC debugfs API") added
> > INIT_WORK() to only vkms_atomic_crtc_duplicate_state() side. Assuming
> > that lifecycle of crc_work is appropriately managed, fix this problem
> > by adding INIT_WORK() to vkms_atomic_crtc_reset() side.
> > 
> > [1] https://syzkaller.appspot.com/bug?id=a5954455fcfa51c29ca2ab55b203076337e1c770
> > 
> > Reported-and-tested-by: syzbot <syzbot+12f1b031b6da017e34f8@syzkaller.appspotmail.com>
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > ---
> >  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 177bbcb..3c37d8c 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -104,6 +104,7 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
> >  	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
> >  	if (!vkms_state)
> >  		return;
> > +	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> 
> Presumably we should migrate/duplicate the crc_work state from the old state
> rather than just re-initializing? My context on the crc_work is foggy, but we
> should be sure that either:
> 
> a) the existing crc work is flushed and inactive, or
> b) we migrate the existing crc work to the duplicated state to keep track of it

The various reset callbacks are used at driver load time to initialize
atomic kms state. Once we do have atomic states we're only using the
various duplicate_state callbacks, which already have the INIT_WORK here.
So was missing once at driver load, and we've hit it once on the first
frame. There is not existing/preceeding crc work when this code is called.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel