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

From: Daniel Vetter <daniel@ffwll.ch>
To: Sean Paul <sean@poorly.run>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>,
	syzbot <syzbot+12f1b031b6da017e34f8@syzkaller.appspotmail.com>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	dri-devel@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
	Sean Paul <seanpaul@chromium.org>
Subject: Re: [PATCH] drm/vkms: Fix flush_work() without INIT_WORK().
Date: Fri, 25 Jan 2019 17:07:39 +0100	[thread overview]
Message-ID: <20190125160739.GG3271@phenom.ffwll.local> (raw)
In-Reply-To: <20190125144615.GS114153@art_vandelay>

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