Re: [PATCH 1/5] drm/prime: fix potential race in drm_gem_map_detach
On Tue, Mar 06, 2018 at 10:30:56AM +0100, Christian König wrote: > Am 06.03.2018 um 10:15 schrieb Daniel Vetter: > > On Wed, Feb 28, 2018 at 11:25:59AM +0100, Christian König wrote: > > > Am 28.02.2018 um 10:48 schrieb Lucas Stach: > > > > Hi Christian, > > > > > > > > Am Dienstag, den 27.02.2018, 12:49 +0100 schrieb Christian König: > > > > > Unpin the GEM object only after freeing the sg table. > > > > What is the race that is being fixed here? The SG table is private to > > > > the importer and the importer should hopefully only call map_detach if > > > > it is done with all operations using the SG table. Thus it shouldn't > > > > matter that the SG table might point to moved pages during execution of > > > > this function. > > > Exactly, it shouldn't matter. This is just a precaution. > > > > > > When the device driver is buggy I want proper error messages from IOMMU > > > and > > > not accessing pages which might already be reused for something else. > > Please add this to the commit message, rather crucial to understand the > > motivation. With that fixed you can have my > > > > Reviewed-by: Daniel Vetter > > > > And pls push to drm-misc. > > Can I use standard git for that now? I really don't want to mess with dim in > my environment. Ping Alex to run it for you please. In an ideal world we'd run all that stuff server-side, but that's not happening anytime soon. Also if you have any specific issues about dim stomping over your setup, we'll be happy to fix it. If you set the (relative) paths correctly you can hide the various additional checkouts it needs rather well. -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
Re: [PATCH 1/5] drm/prime: fix potential race in drm_gem_map_detach
Am 06.03.2018 um 10:15 schrieb Daniel Vetter: On Wed, Feb 28, 2018 at 11:25:59AM +0100, Christian König wrote: Am 28.02.2018 um 10:48 schrieb Lucas Stach: Hi Christian, Am Dienstag, den 27.02.2018, 12:49 +0100 schrieb Christian König: Unpin the GEM object only after freeing the sg table. What is the race that is being fixed here? The SG table is private to the importer and the importer should hopefully only call map_detach if it is done with all operations using the SG table. Thus it shouldn't matter that the SG table might point to moved pages during execution of this function. Exactly, it shouldn't matter. This is just a precaution. When the device driver is buggy I want proper error messages from IOMMU and not accessing pages which might already be reused for something else. Please add this to the commit message, rather crucial to understand the motivation. With that fixed you can have my Reviewed-by: Daniel Vetter And pls push to drm-misc. Can I use standard git for that now? I really don't want to mess with dim in my environment. Christian. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] drm/prime: fix potential race in drm_gem_map_detach
On Wed, Feb 28, 2018 at 11:25:59AM +0100, Christian König wrote:
> Am 28.02.2018 um 10:48 schrieb Lucas Stach:
> > Hi Christian,
> >
> > Am Dienstag, den 27.02.2018, 12:49 +0100 schrieb Christian König:
> > > Unpin the GEM object only after freeing the sg table.
> > What is the race that is being fixed here? The SG table is private to
> > the importer and the importer should hopefully only call map_detach if
> > it is done with all operations using the SG table. Thus it shouldn't
> > matter that the SG table might point to moved pages during execution of
> > this function.
>
> Exactly, it shouldn't matter. This is just a precaution.
>
> When the device driver is buggy I want proper error messages from IOMMU and
> not accessing pages which might already be reused for something else.
Please add this to the commit message, rather crucial to understand the
motivation. With that fixed you can have my
Reviewed-by: Daniel Vetter
And pls push to drm-misc.
-Daniel
>
> Regards,
> Christian.
>
> >
> > Regards,
> > Lucas
> >
> > > Signed-off-by: Christian König
> > > ---
> > > drivers/gpu/drm/drm_prime.c | 32
> > > 1 file changed, 16 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_prime.c
> > > b/drivers/gpu/drm/drm_prime.c
> > > index e82a976f0fba..c38dacda6119 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -230,26 +230,26 @@ void drm_gem_map_detach(struct dma_buf
> > > *dma_buf,
> > > struct drm_prime_attachment *prime_attach = attach->priv;
> > > struct drm_gem_object *obj = dma_buf->priv;
> > > struct drm_device *dev = obj->dev;
> > > - struct sg_table *sgt;
> > > - if (dev->driver->gem_prime_unpin)
> > > - dev->driver->gem_prime_unpin(obj);
> > > + if (prime_attach) {
> > > + struct sg_table *sgt = prime_attach->sgt;
> > > - if (!prime_attach)
> > > - return;
> > > -
> > > - sgt = prime_attach->sgt;
> > > - if (sgt) {
> > > - if (prime_attach->dir != DMA_NONE)
> > > - dma_unmap_sg_attrs(attach->dev, sgt->sgl,
> > > sgt->nents,
> > > -prime_attach->dir,
> > > -DMA_ATTR_SKIP_CPU_SYNC);
> > > - sg_free_table(sgt);
> > > + if (sgt) {
> > > + if (prime_attach->dir != DMA_NONE)
> > > + dma_unmap_sg_attrs(attach->dev, sgt-
> > > > sgl,
> > > +sgt->nents,
> > > +prime_attach-
> > > > dir,
> > > +DMA_ATTR_SKIP_CPU
> > > _SYNC);
> > > + sg_free_table(sgt);
> > > + }
> > > +
> > > + kfree(sgt);
> > > + kfree(prime_attach);
> > > + attach->priv = NULL;
> > > }
> > > - kfree(sgt);
> > > - kfree(prime_attach);
> > > - attach->priv = NULL;
> > > + if (dev->driver->gem_prime_unpin)
> > > + dev->driver->gem_prime_unpin(obj);
> > > }
> > > EXPORT_SYMBOL(drm_gem_map_detach);
>
> ___
> 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
Re: [PATCH 1/5] drm/prime: fix potential race in drm_gem_map_detach
Ping? Anybody who could give me an review on those changes?
The first one is just nice to have, the rest is a nice memory usage
reduction in some cases.
Christian.
Am 27.02.2018 um 12:49 schrieb Christian König:
Unpin the GEM object only after freeing the sg table.
Signed-off-by: Christian König
---
drivers/gpu/drm/drm_prime.c | 32
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index e82a976f0fba..c38dacda6119 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -230,26 +230,26 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
struct drm_prime_attachment *prime_attach = attach->priv;
struct drm_gem_object *obj = dma_buf->priv;
struct drm_device *dev = obj->dev;
- struct sg_table *sgt;
- if (dev->driver->gem_prime_unpin)
- dev->driver->gem_prime_unpin(obj);
+ if (prime_attach) {
+ struct sg_table *sgt = prime_attach->sgt;
- if (!prime_attach)
- return;
-
- sgt = prime_attach->sgt;
- if (sgt) {
- if (prime_attach->dir != DMA_NONE)
- dma_unmap_sg_attrs(attach->dev, sgt->sgl, sgt->nents,
- prime_attach->dir,
- DMA_ATTR_SKIP_CPU_SYNC);
- sg_free_table(sgt);
+ if (sgt) {
+ if (prime_attach->dir != DMA_NONE)
+ dma_unmap_sg_attrs(attach->dev, sgt->sgl,
+ sgt->nents,
+ prime_attach->dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
+ sg_free_table(sgt);
+ }
+
+ kfree(sgt);
+ kfree(prime_attach);
+ attach->priv = NULL;
}
- kfree(sgt);
- kfree(prime_attach);
- attach->priv = NULL;
+ if (dev->driver->gem_prime_unpin)
+ dev->driver->gem_prime_unpin(obj);
}
EXPORT_SYMBOL(drm_gem_map_detach);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] drm/prime: fix potential race in drm_gem_map_detach
Am 28.02.2018 um 10:48 schrieb Lucas Stach:
Hi Christian,
Am Dienstag, den 27.02.2018, 12:49 +0100 schrieb Christian König:
Unpin the GEM object only after freeing the sg table.
What is the race that is being fixed here? The SG table is private to
the importer and the importer should hopefully only call map_detach if
it is done with all operations using the SG table. Thus it shouldn't
matter that the SG table might point to moved pages during execution of
this function.
Exactly, it shouldn't matter. This is just a precaution.
When the device driver is buggy I want proper error messages from IOMMU
and not accessing pages which might already be reused for something else.
Regards,
Christian.
Regards,
Lucas
Signed-off-by: Christian König
---
drivers/gpu/drm/drm_prime.c | 32
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c
b/drivers/gpu/drm/drm_prime.c
index e82a976f0fba..c38dacda6119 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -230,26 +230,26 @@ void drm_gem_map_detach(struct dma_buf
*dma_buf,
struct drm_prime_attachment *prime_attach = attach->priv;
struct drm_gem_object *obj = dma_buf->priv;
struct drm_device *dev = obj->dev;
- struct sg_table *sgt;
- if (dev->driver->gem_prime_unpin)
- dev->driver->gem_prime_unpin(obj);
+ if (prime_attach) {
+ struct sg_table *sgt = prime_attach->sgt;
- if (!prime_attach)
- return;
-
- sgt = prime_attach->sgt;
- if (sgt) {
- if (prime_attach->dir != DMA_NONE)
- dma_unmap_sg_attrs(attach->dev, sgt->sgl,
sgt->nents,
- prime_attach->dir,
- DMA_ATTR_SKIP_CPU_SYNC);
- sg_free_table(sgt);
+ if (sgt) {
+ if (prime_attach->dir != DMA_NONE)
+ dma_unmap_sg_attrs(attach->dev, sgt-
sgl,
+ sgt->nents,
+ prime_attach-
dir,
+ DMA_ATTR_SKIP_CPU
_SYNC);
+ sg_free_table(sgt);
+ }
+
+ kfree(sgt);
+ kfree(prime_attach);
+ attach->priv = NULL;
}
- kfree(sgt);
- kfree(prime_attach);
- attach->priv = NULL;
+ if (dev->driver->gem_prime_unpin)
+ dev->driver->gem_prime_unpin(obj);
}
EXPORT_SYMBOL(drm_gem_map_detach);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] drm/prime: fix potential race in drm_gem_map_detach
Hi Christian,
Am Dienstag, den 27.02.2018, 12:49 +0100 schrieb Christian König:
> Unpin the GEM object only after freeing the sg table.
What is the race that is being fixed here? The SG table is private to
the importer and the importer should hopefully only call map_detach if
it is done with all operations using the SG table. Thus it shouldn't
matter that the SG table might point to moved pages during execution of
this function.
Regards,
Lucas
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/drm_prime.c | 32
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c
> b/drivers/gpu/drm/drm_prime.c
> index e82a976f0fba..c38dacda6119 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -230,26 +230,26 @@ void drm_gem_map_detach(struct dma_buf
> *dma_buf,
> struct drm_prime_attachment *prime_attach = attach->priv;
> struct drm_gem_object *obj = dma_buf->priv;
> struct drm_device *dev = obj->dev;
> - struct sg_table *sgt;
>
> - if (dev->driver->gem_prime_unpin)
> - dev->driver->gem_prime_unpin(obj);
> + if (prime_attach) {
> + struct sg_table *sgt = prime_attach->sgt;
>
> - if (!prime_attach)
> - return;
> -
> - sgt = prime_attach->sgt;
> - if (sgt) {
> - if (prime_attach->dir != DMA_NONE)
> - dma_unmap_sg_attrs(attach->dev, sgt->sgl,
> sgt->nents,
> -prime_attach->dir,
> -DMA_ATTR_SKIP_CPU_SYNC);
> - sg_free_table(sgt);
> + if (sgt) {
> + if (prime_attach->dir != DMA_NONE)
> + dma_unmap_sg_attrs(attach->dev, sgt-
> >sgl,
> +sgt->nents,
> +prime_attach-
> >dir,
> +DMA_ATTR_SKIP_CPU
> _SYNC);
> + sg_free_table(sgt);
> + }
> +
> + kfree(sgt);
> + kfree(prime_attach);
> + attach->priv = NULL;
> }
>
> - kfree(sgt);
> - kfree(prime_attach);
> - attach->priv = NULL;
> + if (dev->driver->gem_prime_unpin)
> + dev->driver->gem_prime_unpin(obj);
> }
> EXPORT_SYMBOL(drm_gem_map_detach);
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
