* Re: [PATCH] dma-buf: fix stack corruption in dma_fence_chain_release
[not found] <20190805073657.1389-1-christian.koenig@amd.com>
@ 2019-08-05 8:23 ` Lionel Landwerlin
2019-08-05 12:03 ` Lionel Landwerlin
1 sibling, 0 replies; 5+ messages in thread
From: Lionel Landwerlin @ 2019-08-05 8:23 UTC (permalink / raw)
To: Christian König, dri-devel, david1.zhou
On 05/08/2019 10:36, Christian König wrote:
> We can't free up the chain using recursion or we run into a stack overflow.
>
> Manually free up the dangling chain nodes to avoid recursion.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
That definitely makes sense to me, but I'm not an expert in RCU foo :/
Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
I guess this deserves a
Fixes: 7bf60c52e0 ("dma-buf: add new dma_fence_chain container v7")
> ---
> drivers/dma-buf/dma-fence-chain.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index b5089f64be2a..44a741677d25 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -178,8 +178,30 @@ static bool dma_fence_chain_signaled(struct dma_fence *fence)
> static void dma_fence_chain_release(struct dma_fence *fence)
> {
> struct dma_fence_chain *chain = to_dma_fence_chain(fence);
> + struct dma_fence *prev;
> +
> + /* Manually unlink the chain as much as possible to avoid recursion
> + * and potential stack overflow.
> + */
> + while ((prev = rcu_dereference_protected(chain->prev, true))) {
> + struct dma_fence_chain *prev_chain;
> +
> + if (kref_read(&prev->refcount) > 1)
> + break;
> +
> + prev_chain = to_dma_fence_chain(prev);
> + if (!prev_chain)
> + break;
> +
> + /* No need for atomic operations since we hold the last
> + * reference to prev_chain.
> + */
> + chain->prev = prev_chain->prev;
> + RCU_INIT_POINTER(prev_chain->prev, NULL);
> + dma_fence_put(prev);
> + }
> + dma_fence_put(prev);
>
> - dma_fence_put(rcu_dereference_protected(chain->prev, true));
> dma_fence_put(chain->fence);
> dma_fence_free(fence);
> }
_______________________________________________
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] dma-buf: fix stack corruption in dma_fence_chain_release
[not found] <20190805073657.1389-1-christian.koenig@amd.com>
2019-08-05 8:23 ` [PATCH] dma-buf: fix stack corruption in dma_fence_chain_release Lionel Landwerlin
@ 2019-08-05 12:03 ` Lionel Landwerlin
2019-08-05 13:02 ` Christian König
1 sibling, 1 reply; 5+ messages in thread
From: Lionel Landwerlin @ 2019-08-05 12:03 UTC (permalink / raw)
To: Christian König, dri-devel, david1.zhou
By any change, did you run into this with a CTS test whose name ends
with ".chain" ? :)
-Lionel
On 05/08/2019 10:36, Christian König wrote:
> We can't free up the chain using recursion or we run into a stack overflow.
>
> Manually free up the dangling chain nodes to avoid recursion.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/dma-buf/dma-fence-chain.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index b5089f64be2a..44a741677d25 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -178,8 +178,30 @@ static bool dma_fence_chain_signaled(struct dma_fence *fence)
> static void dma_fence_chain_release(struct dma_fence *fence)
> {
> struct dma_fence_chain *chain = to_dma_fence_chain(fence);
> + struct dma_fence *prev;
> +
> + /* Manually unlink the chain as much as possible to avoid recursion
> + * and potential stack overflow.
> + */
> + while ((prev = rcu_dereference_protected(chain->prev, true))) {
> + struct dma_fence_chain *prev_chain;
> +
> + if (kref_read(&prev->refcount) > 1)
> + break;
> +
> + prev_chain = to_dma_fence_chain(prev);
> + if (!prev_chain)
> + break;
> +
> + /* No need for atomic operations since we hold the last
> + * reference to prev_chain.
> + */
> + chain->prev = prev_chain->prev;
> + RCU_INIT_POINTER(prev_chain->prev, NULL);
> + dma_fence_put(prev);
> + }
> + dma_fence_put(prev);
>
> - dma_fence_put(rcu_dereference_protected(chain->prev, true));
> dma_fence_put(chain->fence);
> dma_fence_free(fence);
> }
_______________________________________________
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] dma-buf: fix stack corruption in dma_fence_chain_release
2019-08-05 12:03 ` Lionel Landwerlin
@ 2019-08-05 13:02 ` Christian König
2019-08-05 13:32 ` Lionel Landwerlin
0 siblings, 1 reply; 5+ messages in thread
From: Christian König @ 2019-08-05 13:02 UTC (permalink / raw)
To: Lionel Landwerlin, dri-devel, david1.zhou
Not even remotely :)I tested this with my own crafted code inside the
kernel.
It's probably quite some hassle to actually trigger this problem from
userspace and I only found it because I created a very very long
sequence chain by accident.
Christian.
Am 05.08.19 um 14:03 schrieb Lionel Landwerlin:
> By any change, did you run into this with a CTS test whose name ends
> with ".chain" ? :)
>
> -Lionel
>
> On 05/08/2019 10:36, Christian König wrote:
>> We can't free up the chain using recursion or we run into a stack
>> overflow.
>>
>> Manually free up the dangling chain nodes to avoid recursion.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/dma-buf/dma-fence-chain.c | 24 +++++++++++++++++++++++-
>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence-chain.c
>> b/drivers/dma-buf/dma-fence-chain.c
>> index b5089f64be2a..44a741677d25 100644
>> --- a/drivers/dma-buf/dma-fence-chain.c
>> +++ b/drivers/dma-buf/dma-fence-chain.c
>> @@ -178,8 +178,30 @@ static bool dma_fence_chain_signaled(struct
>> dma_fence *fence)
>> static void dma_fence_chain_release(struct dma_fence *fence)
>> {
>> struct dma_fence_chain *chain = to_dma_fence_chain(fence);
>> + struct dma_fence *prev;
>> +
>> + /* Manually unlink the chain as much as possible to avoid recursion
>> + * and potential stack overflow.
>> + */
>> + while ((prev = rcu_dereference_protected(chain->prev, true))) {
>> + struct dma_fence_chain *prev_chain;
>> +
>> + if (kref_read(&prev->refcount) > 1)
>> + break;
>> +
>> + prev_chain = to_dma_fence_chain(prev);
>> + if (!prev_chain)
>> + break;
>> +
>> + /* No need for atomic operations since we hold the last
>> + * reference to prev_chain.
>> + */
>> + chain->prev = prev_chain->prev;
>> + RCU_INIT_POINTER(prev_chain->prev, NULL);
>> + dma_fence_put(prev);
>> + }
>> + dma_fence_put(prev);
>> - dma_fence_put(rcu_dereference_protected(chain->prev, true));
>> dma_fence_put(chain->fence);
>> dma_fence_free(fence);
>> }
>
>
_______________________________________________
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] dma-buf: fix stack corruption in dma_fence_chain_release
2019-08-05 13:02 ` Christian König
@ 2019-08-05 13:32 ` Lionel Landwerlin
2019-08-05 16:30 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Lionel Landwerlin @ 2019-08-05 13:32 UTC (permalink / raw)
To: christian.koenig, dri-devel, david1.zhou
That one test creates a 32k chain of fences I think.
Anyway my kernel crash was unrelated ;)
-Lionel
On 05/08/2019 16:02, Christian König wrote:
> Not even remotely :)I tested this with my own crafted code inside the
> kernel.
>
> It's probably quite some hassle to actually trigger this problem from
> userspace and I only found it because I created a very very long
> sequence chain by accident.
>
> Christian.
>
> Am 05.08.19 um 14:03 schrieb Lionel Landwerlin:
>> By any change, did you run into this with a CTS test whose name ends
>> with ".chain" ? :)
>>
>> -Lionel
>>
>> On 05/08/2019 10:36, Christian König wrote:
>>> We can't free up the chain using recursion or we run into a stack
>>> overflow.
>>>
>>> Manually free up the dangling chain nodes to avoid recursion.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>> drivers/dma-buf/dma-fence-chain.c | 24 +++++++++++++++++++++++-
>>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence-chain.c
>>> b/drivers/dma-buf/dma-fence-chain.c
>>> index b5089f64be2a..44a741677d25 100644
>>> --- a/drivers/dma-buf/dma-fence-chain.c
>>> +++ b/drivers/dma-buf/dma-fence-chain.c
>>> @@ -178,8 +178,30 @@ static bool dma_fence_chain_signaled(struct
>>> dma_fence *fence)
>>> static void dma_fence_chain_release(struct dma_fence *fence)
>>> {
>>> struct dma_fence_chain *chain = to_dma_fence_chain(fence);
>>> + struct dma_fence *prev;
>>> +
>>> + /* Manually unlink the chain as much as possible to avoid
>>> recursion
>>> + * and potential stack overflow.
>>> + */
>>> + while ((prev = rcu_dereference_protected(chain->prev, true))) {
>>> + struct dma_fence_chain *prev_chain;
>>> +
>>> + if (kref_read(&prev->refcount) > 1)
>>> + break;
>>> +
>>> + prev_chain = to_dma_fence_chain(prev);
>>> + if (!prev_chain)
>>> + break;
>>> +
>>> + /* No need for atomic operations since we hold the last
>>> + * reference to prev_chain.
>>> + */
>>> + chain->prev = prev_chain->prev;
>>> + RCU_INIT_POINTER(prev_chain->prev, NULL);
>>> + dma_fence_put(prev);
>>> + }
>>> + dma_fence_put(prev);
>>> - dma_fence_put(rcu_dereference_protected(chain->prev, true));
>>> dma_fence_put(chain->fence);
>>> dma_fence_free(fence);
>>> }
>>
>>
>
>
_______________________________________________
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] dma-buf: fix stack corruption in dma_fence_chain_release
2019-08-05 13:32 ` Lionel Landwerlin
@ 2019-08-05 16:30 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2019-08-05 16:30 UTC (permalink / raw)
To: Lionel Landwerlin; +Cc: christian.koenig, dri-devel
On Mon, Aug 05, 2019 at 04:32:20PM +0300, Lionel Landwerlin wrote:
> That one test creates a 32k chain of fences I think.
> Anyway my kernel crash was unrelated ;)
Hm I'd expect that with clever use of vgem fake fences we should be able
to repro this bug here with an igt. Would be real nice, any takers?
-Daniel
>
> -Lionel
>
> On 05/08/2019 16:02, Christian König wrote:
> > Not even remotely :)I tested this with my own crafted code inside the
> > kernel.
> >
> > It's probably quite some hassle to actually trigger this problem from
> > userspace and I only found it because I created a very very long
> > sequence chain by accident.
> >
> > Christian.
> >
> > Am 05.08.19 um 14:03 schrieb Lionel Landwerlin:
> > > By any change, did you run into this with a CTS test whose name ends
> > > with ".chain" ? :)
> > >
> > > -Lionel
> > >
> > > On 05/08/2019 10:36, Christian König wrote:
> > > > We can't free up the chain using recursion or we run into a
> > > > stack overflow.
> > > >
> > > > Manually free up the dangling chain nodes to avoid recursion.
> > > >
> > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > ---
> > > > drivers/dma-buf/dma-fence-chain.c | 24 +++++++++++++++++++++++-
> > > > 1 file changed, 23 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/dma-buf/dma-fence-chain.c
> > > > b/drivers/dma-buf/dma-fence-chain.c
> > > > index b5089f64be2a..44a741677d25 100644
> > > > --- a/drivers/dma-buf/dma-fence-chain.c
> > > > +++ b/drivers/dma-buf/dma-fence-chain.c
> > > > @@ -178,8 +178,30 @@ static bool dma_fence_chain_signaled(struct
> > > > dma_fence *fence)
> > > > static void dma_fence_chain_release(struct dma_fence *fence)
> > > > {
> > > > struct dma_fence_chain *chain = to_dma_fence_chain(fence);
> > > > + struct dma_fence *prev;
> > > > +
> > > > + /* Manually unlink the chain as much as possible to avoid
> > > > recursion
> > > > + * and potential stack overflow.
> > > > + */
> > > > + while ((prev = rcu_dereference_protected(chain->prev, true))) {
> > > > + struct dma_fence_chain *prev_chain;
> > > > +
> > > > + if (kref_read(&prev->refcount) > 1)
> > > > + break;
> > > > +
> > > > + prev_chain = to_dma_fence_chain(prev);
> > > > + if (!prev_chain)
> > > > + break;
> > > > +
> > > > + /* No need for atomic operations since we hold the last
> > > > + * reference to prev_chain.
> > > > + */
> > > > + chain->prev = prev_chain->prev;
> > > > + RCU_INIT_POINTER(prev_chain->prev, NULL);
> > > > + dma_fence_put(prev);
> > > > + }
> > > > + dma_fence_put(prev);
> > > > - dma_fence_put(rcu_dereference_protected(chain->prev, true));
> > > > dma_fence_put(chain->fence);
> > > > dma_fence_free(fence);
> > > > }
> > >
> > >
> >
> >
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-05 16:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190805073657.1389-1-christian.koenig@amd.com>
2019-08-05 8:23 ` [PATCH] dma-buf: fix stack corruption in dma_fence_chain_release Lionel Landwerlin
2019-08-05 12:03 ` Lionel Landwerlin
2019-08-05 13:02 ` Christian König
2019-08-05 13:32 ` Lionel Landwerlin
2019-08-05 16:30 ` Daniel Vetter
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.