| Message ID | 20190601233627.5092-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [libdrm] etnaviv: Fix double-free in etna_bo_cache_free() | expand |
Am So., 2. Juni 2019 um 01:37 Uhr schrieb Marek Vasut <marex@denx.de>: > > The following situation can happen in a multithreaded OpenGL application. > A BO is submitted from etna_cmd_stream #1 with flags set for read. > A BO is submitted from etna_cmd_stream #2 with flags set for write. > This triggers a flush on stream #1 and clears the BO's current_stream > pointer. If at this point, stream #2 attempts to queue BO again, which > does happen, the BO will be added to the submit list twice. The Linux > kernel driver correctly detects this and warns about it with "BO at > index %u already on submit list" kernel message. > > However, when cleaning the BO cache in etna_bo_cache_free(), the BO > which was submitted twice will also be free()d twice, this triggering > a glibc double free detector. > > The fix is easy, even if the BO does not have current_stream set, > iterate over current streams' list of BOs before adding the BO to it > and verify that the BO is not yet there. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com> Will land this patch even libdrm parts got moved to mesa as it fixes a serious issue and somebody might use newest libdrm with mesa < 19.1. > --- > etnaviv/etnaviv_cmd_stream.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/etnaviv/etnaviv_cmd_stream.c b/etnaviv/etnaviv_cmd_stream.c > index 7139c324..261777b0 100644 > --- a/etnaviv/etnaviv_cmd_stream.c > +++ b/etnaviv/etnaviv_cmd_stream.c > @@ -150,11 +150,7 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, > > pthread_mutex_lock(&idx_lock); > > - if (!bo->current_stream) { > - idx = append_bo(stream, bo); > - bo->current_stream = stream; > - bo->idx = idx; > - } else if (bo->current_stream == stream) { > + if (bo->current_stream == stream) { > idx = bo->idx; > } else { > /* slow-path: */ > @@ -165,6 +161,8 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, > /* not found */ > idx = append_bo(stream, bo); > } > + bo->current_stream = stream; > + bo->idx = idx; > } > pthread_mutex_unlock(&idx_lock); > > -- > 2.20.1 >
On Sun, Jun 02, 2019 at 01:36:27AM +0200, Marek Vasut wrote: > The following situation can happen in a multithreaded OpenGL application. > A BO is submitted from etna_cmd_stream #1 with flags set for read. > A BO is submitted from etna_cmd_stream #2 with flags set for write. > This triggers a flush on stream #1 and clears the BO's current_stream > pointer. If at this point, stream #2 attempts to queue BO again, which > does happen, the BO will be added to the submit list twice. The Linux > kernel driver correctly detects this and warns about it with "BO at > index %u already on submit list" kernel message. > > However, when cleaning the BO cache in etna_bo_cache_free(), the BO > which was submitted twice will also be free()d twice, this triggering > a glibc double free detector. > > The fix is easy, even if the BO does not have current_stream set, > iterate over current streams' list of BOs before adding the BO to it > and verify that the BO is not yet there. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > --- > etnaviv/etnaviv_cmd_stream.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) Thanks for catching this. Reviewed-by: Wladimir J. van der Laan <laanwj@gmail.com> > diff --git a/etnaviv/etnaviv_cmd_stream.c b/etnaviv/etnaviv_cmd_stream.c > index 7139c324..261777b0 100644 > --- a/etnaviv/etnaviv_cmd_stream.c > +++ b/etnaviv/etnaviv_cmd_stream.c > @@ -150,11 +150,7 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, > > pthread_mutex_lock(&idx_lock); > > - if (!bo->current_stream) { > - idx = append_bo(stream, bo); > - bo->current_stream = stream; > - bo->idx = idx; > - } else if (bo->current_stream == stream) { > + if (bo->current_stream == stream) { > idx = bo->idx; > } else { > /* slow-path: */ > @@ -165,6 +161,8 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, > /* not found */ > idx = append_bo(stream, bo); > } > + bo->current_stream = stream; > + bo->idx = idx; > } > pthread_mutex_unlock(&idx_lock); > > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/etnaviv/etnaviv_cmd_stream.c b/etnaviv/etnaviv_cmd_stream.c index 7139c324..261777b0 100644 --- a/etnaviv/etnaviv_cmd_stream.c +++ b/etnaviv/etnaviv_cmd_stream.c @@ -150,11 +150,7 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, pthread_mutex_lock(&idx_lock); - if (!bo->current_stream) { - idx = append_bo(stream, bo); - bo->current_stream = stream; - bo->idx = idx; - } else if (bo->current_stream == stream) { + if (bo->current_stream == stream) { idx = bo->idx; } else { /* slow-path: */ @@ -165,6 +161,8 @@ static uint32_t bo2idx(struct etna_cmd_stream *stream, struct etna_bo *bo, /* not found */ idx = append_bo(stream, bo); } + bo->current_stream = stream; + bo->idx = idx; } pthread_mutex_unlock(&idx_lock);
The following situation can happen in a multithreaded OpenGL application. A BO is submitted from etna_cmd_stream #1 with flags set for read. A BO is submitted from etna_cmd_stream #2 with flags set for write. This triggers a flush on stream #1 and clears the BO's current_stream pointer. If at this point, stream #2 attempts to queue BO again, which does happen, the BO will be added to the submit list twice. The Linux kernel driver correctly detects this and warns about it with "BO at index %u already on submit list" kernel message. However, when cleaning the BO cache in etna_bo_cache_free(), the BO which was submitted twice will also be free()d twice, this triggering a glibc double free detector. The fix is easy, even if the BO does not have current_stream set, iterate over current streams' list of BOs before adding the BO to it and verify that the BO is not yet there. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> Cc: Lucas Stach <l.stach@pengutronix.de> --- etnaviv/etnaviv_cmd_stream.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)