| Message ID | 1547829823-9877-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Fix flush_work() without INIT_WORK(). | expand |
> 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>
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
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
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;
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(+)