* [PATCH] drm: vc4: Fix race during binding
@ 2017-10-06 18:43 Stefan Wahren
2017-10-06 19:42 ` Eric Anholt
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Wahren @ 2017-10-06 18:43 UTC (permalink / raw)
To: Eric Anholt, David Airlie; +Cc: Stefan Wahren, dri-devel
This fixes the race between vc4_overflow_mem_work and the init of the
job lock. Otherwise we could trigger a NULL pointer dereference
during VC4 binding.
Link: https://github.com/anholt/linux/issues/114
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
---
drivers/gpu/drm/vc4/vc4_gem.c | 1 -
drivers/gpu/drm/vc4/vc4_irq.c | 1 +
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index d0c6bfb..47d964f 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -1088,7 +1088,6 @@ vc4_gem_init(struct drm_device *dev)
INIT_LIST_HEAD(&vc4->render_job_list);
INIT_LIST_HEAD(&vc4->job_done_list);
INIT_LIST_HEAD(&vc4->seqno_cb_list);
- spin_lock_init(&vc4->job_lock);
INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
setup_timer(&vc4->hangcheck.timer,
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 7d7af3a..d508d13 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -195,6 +195,7 @@ vc4_irq_preinstall(struct drm_device *dev)
struct vc4_dev *vc4 = to_vc4_dev(dev);
init_waitqueue_head(&vc4->job_wait_queue);
+ spin_lock_init(&vc4->job_lock);
INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work);
/* Clear any pending interrupts someone might have left around
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm: vc4: Fix race during binding
2017-10-06 18:43 [PATCH] drm: vc4: Fix race during binding Stefan Wahren
@ 2017-10-06 19:42 ` Eric Anholt
2017-10-07 8:13 ` Stefan Wahren
0 siblings, 1 reply; 5+ messages in thread
From: Eric Anholt @ 2017-10-06 19:42 UTC (permalink / raw)
To: David Airlie; +Cc: Stefan Wahren, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1728 bytes --]
Stefan Wahren <stefan.wahren@i2se.com> writes:
> This fixes the race between vc4_overflow_mem_work and the init of the
> job lock. Otherwise we could trigger a NULL pointer dereference
> during VC4 binding.
>
> Link: https://github.com/anholt/linux/issues/114
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
> ---
> drivers/gpu/drm/vc4/vc4_gem.c | 1 -
> drivers/gpu/drm/vc4/vc4_irq.c | 1 +
> 2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index d0c6bfb..47d964f 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -1088,7 +1088,6 @@ vc4_gem_init(struct drm_device *dev)
> INIT_LIST_HEAD(&vc4->render_job_list);
> INIT_LIST_HEAD(&vc4->job_done_list);
> INIT_LIST_HEAD(&vc4->seqno_cb_list);
> - spin_lock_init(&vc4->job_lock);
>
> INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
> setup_timer(&vc4->hangcheck.timer,
> diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> index 7d7af3a..d508d13 100644
> --- a/drivers/gpu/drm/vc4/vc4_irq.c
> +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> @@ -195,6 +195,7 @@ vc4_irq_preinstall(struct drm_device *dev)
> struct vc4_dev *vc4 = to_vc4_dev(dev);
>
> init_waitqueue_head(&vc4->job_wait_queue);
> + spin_lock_init(&vc4->job_lock);
> INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work);
>
> /* Clear any pending interrupts someone might have left around
Are you sure this is a fix? We don't attach the IRQ handler until V3D
bind, and vc4_gem_init happens before component_bind_all(), right?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm: vc4: Fix race during binding
2017-10-06 19:42 ` Eric Anholt
@ 2017-10-07 8:13 ` Stefan Wahren
2017-10-08 17:09 ` Eric Anholt
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Wahren @ 2017-10-07 8:13 UTC (permalink / raw)
To: Eric Anholt; +Cc: dri-devel
Hi Eric,
> Eric Anholt <eric@anholt.net> hat am 6. Oktober 2017 um 21:42 geschrieben:
>
>
> Stefan Wahren <stefan.wahren@i2se.com> writes:
>
> > This fixes the race between vc4_overflow_mem_work and the init of the
> > job lock. Otherwise we could trigger a NULL pointer dereference
> > during VC4 binding.
> >
> > Link: https://github.com/anholt/linux/issues/114
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
> > ---
> > drivers/gpu/drm/vc4/vc4_gem.c | 1 -
> > drivers/gpu/drm/vc4/vc4_irq.c | 1 +
> > 2 files changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> > index d0c6bfb..47d964f 100644
> > --- a/drivers/gpu/drm/vc4/vc4_gem.c
> > +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> > @@ -1088,7 +1088,6 @@ vc4_gem_init(struct drm_device *dev)
> > INIT_LIST_HEAD(&vc4->render_job_list);
> > INIT_LIST_HEAD(&vc4->job_done_list);
> > INIT_LIST_HEAD(&vc4->seqno_cb_list);
> > - spin_lock_init(&vc4->job_lock);
> >
> > INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
> > setup_timer(&vc4->hangcheck.timer,
> > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> > index 7d7af3a..d508d13 100644
> > --- a/drivers/gpu/drm/vc4/vc4_irq.c
> > +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> > @@ -195,6 +195,7 @@ vc4_irq_preinstall(struct drm_device *dev)
> > struct vc4_dev *vc4 = to_vc4_dev(dev);
> >
> > init_waitqueue_head(&vc4->job_wait_queue);
> > + spin_lock_init(&vc4->job_lock);
> > INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work);
> >
> > /* Clear any pending interrupts someone might have left around
>
> Are you sure this is a fix? We don't attach the IRQ handler until V3D
> bind, and vc4_gem_init happens before component_bind_all(), right?
i agree i should have mark it as a RFC. But is it really impossible that vc4_overflow_mem_work is triggered before vc4_gem_init?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm: vc4: Fix race during binding
2017-10-07 8:13 ` Stefan Wahren
@ 2017-10-08 17:09 ` Eric Anholt
2017-10-08 18:51 ` Stefan Wahren
0 siblings, 1 reply; 5+ messages in thread
From: Eric Anholt @ 2017-10-08 17:09 UTC (permalink / raw)
To: Stefan Wahren; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2615 bytes --]
Stefan Wahren <stefan.wahren@i2se.com> writes:
> Hi Eric,
>
>> Eric Anholt <eric@anholt.net> hat am 6. Oktober 2017 um 21:42 geschrieben:
>>
>>
>> Stefan Wahren <stefan.wahren@i2se.com> writes:
>>
>> > This fixes the race between vc4_overflow_mem_work and the init of the
>> > job lock. Otherwise we could trigger a NULL pointer dereference
>> > during VC4 binding.
>> >
>> > Link: https://github.com/anholt/linux/issues/114
>> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> > Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
>> > ---
>> > drivers/gpu/drm/vc4/vc4_gem.c | 1 -
>> > drivers/gpu/drm/vc4/vc4_irq.c | 1 +
>> > 2 files changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
>> > index d0c6bfb..47d964f 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_gem.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_gem.c
>> > @@ -1088,7 +1088,6 @@ vc4_gem_init(struct drm_device *dev)
>> > INIT_LIST_HEAD(&vc4->render_job_list);
>> > INIT_LIST_HEAD(&vc4->job_done_list);
>> > INIT_LIST_HEAD(&vc4->seqno_cb_list);
>> > - spin_lock_init(&vc4->job_lock);
>> >
>> > INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
>> > setup_timer(&vc4->hangcheck.timer,
>> > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
>> > index 7d7af3a..d508d13 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_irq.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_irq.c
>> > @@ -195,6 +195,7 @@ vc4_irq_preinstall(struct drm_device *dev)
>> > struct vc4_dev *vc4 = to_vc4_dev(dev);
>> >
>> > init_waitqueue_head(&vc4->job_wait_queue);
>> > + spin_lock_init(&vc4->job_lock);
>> > INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work);
>> >
>> > /* Clear any pending interrupts someone might have left around
>>
>> Are you sure this is a fix? We don't attach the IRQ handler until V3D
>> bind, and vc4_gem_init happens before component_bind_all(), right?
>
> i agree i should have mark it as a RFC. But is it really impossible
> that vc4_overflow_mem_work is triggered before vc4_gem_init?
As far as I can see, it gets queued from the IRQ handler, the IRQ
handler is added during V3D bind, and binding happens after GEM init.
Hmm. If we fail out of component binding and try again, we'll end up
doing the job_wait_queue and overflow_mem_work initialization on the
same pointer twice. Will that cause any trouble? We cancel any pending
work during uninstall (V3D unbind path), but does drm_irq_uninstall()
make sure that the irq handler has finished?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm: vc4: Fix race during binding
2017-10-08 17:09 ` Eric Anholt
@ 2017-10-08 18:51 ` Stefan Wahren
0 siblings, 0 replies; 5+ messages in thread
From: Stefan Wahren @ 2017-10-08 18:51 UTC (permalink / raw)
To: Eric Anholt; +Cc: dri-devel
> Eric Anholt <eric@anholt.net> hat am 8. Oktober 2017 um 19:09 geschrieben:
>
>
> Stefan Wahren <stefan.wahren@i2se.com> writes:
>
> > Hi Eric,
> >
> >> Eric Anholt <eric@anholt.net> hat am 6. Oktober 2017 um 21:42 geschrieben:
> >>
> >>
> >> Stefan Wahren <stefan.wahren@i2se.com> writes:
> >>
> >> > This fixes the race between vc4_overflow_mem_work and the init of the
> >> > job lock. Otherwise we could trigger a NULL pointer dereference
> >> > during VC4 binding.
> >> >
> >> > Link: https://github.com/anholt/linux/issues/114
> >> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >> > Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
> >> > ---
> >> > drivers/gpu/drm/vc4/vc4_gem.c | 1 -
> >> > drivers/gpu/drm/vc4/vc4_irq.c | 1 +
> >> > 2 files changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> >> > index d0c6bfb..47d964f 100644
> >> > --- a/drivers/gpu/drm/vc4/vc4_gem.c
> >> > +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> >> > @@ -1088,7 +1088,6 @@ vc4_gem_init(struct drm_device *dev)
> >> > INIT_LIST_HEAD(&vc4->render_job_list);
> >> > INIT_LIST_HEAD(&vc4->job_done_list);
> >> > INIT_LIST_HEAD(&vc4->seqno_cb_list);
> >> > - spin_lock_init(&vc4->job_lock);
> >> >
> >> > INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
> >> > setup_timer(&vc4->hangcheck.timer,
> >> > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> >> > index 7d7af3a..d508d13 100644
> >> > --- a/drivers/gpu/drm/vc4/vc4_irq.c
> >> > +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> >> > @@ -195,6 +195,7 @@ vc4_irq_preinstall(struct drm_device *dev)
> >> > struct vc4_dev *vc4 = to_vc4_dev(dev);
> >> >
> >> > init_waitqueue_head(&vc4->job_wait_queue);
> >> > + spin_lock_init(&vc4->job_lock);
> >> > INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work);
> >> >
> >> > /* Clear any pending interrupts someone might have left around
> >>
> >> Are you sure this is a fix? We don't attach the IRQ handler until V3D
> >> bind, and vc4_gem_init happens before component_bind_all(), right?
> >
> > i agree i should have mark it as a RFC. But is it really impossible
> > that vc4_overflow_mem_work is triggered before vc4_gem_init?
>
> As far as I can see, it gets queued from the IRQ handler, the IRQ
> handler is added during V3D bind, and binding happens after GEM init.
>
> Hmm. If we fail out of component binding and try again, we'll end up
> doing the job_wait_queue and overflow_mem_work initialization on the
> same pointer twice. Will that cause any trouble? We cancel any pending
> work during uninstall (V3D unbind path), but does drm_irq_uninstall()
> make sure that the irq handler has finished?
I cannot see an issue.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-08 18:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-06 18:43 [PATCH] drm: vc4: Fix race during binding Stefan Wahren
2017-10-06 19:42 ` Eric Anholt
2017-10-07 8:13 ` Stefan Wahren
2017-10-08 17:09 ` Eric Anholt
2017-10-08 18:51 ` Stefan Wahren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.