| Message ID | 20210102202437.1630365-1-iskren.chernev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: Fix MSM_INFO_GET_IOVA with carveout | expand |
On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@gmail.com> wrote: > > The msm_gem_get_iova should be guarded with gpu != NULL and not aspace > != NULL, because aspace is NULL when using vram carveout. > > Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > --- > drivers/gpu/drm/msm/msm_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index c5e61cb3356df..c1953fb079133 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, > struct drm_file *file, struct drm_gem_object *obj, > uint64_t *iova) > { > + struct msm_drm_private *priv = dev->dev_private; > struct msm_file_private *ctx = file->driver_priv; > > - if (!ctx->aspace) > + if (!priv->gpu) > return -EINVAL; Does this actually work? It seems like you would hit a null ptr deref in msm_gem_init_vma().. and in general I think a lot of code paths would be surprised by a null address space, so this seems like a risky idea. Maybe instead we should be creating an address space for the vram carveout? BR, -R > /* > -- > 2.29.2 >
On Thu, Jan 7, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote: > > On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@gmail.com> wrote: > > > > The msm_gem_get_iova should be guarded with gpu != NULL and not aspace > > != NULL, because aspace is NULL when using vram carveout. > > > > Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") > > > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > > --- > > drivers/gpu/drm/msm/msm_drv.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index c5e61cb3356df..c1953fb079133 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, > > struct drm_file *file, struct drm_gem_object *obj, > > uint64_t *iova) > > { > > + struct msm_drm_private *priv = dev->dev_private; > > struct msm_file_private *ctx = file->driver_priv; > > > > - if (!ctx->aspace) > > + if (!priv->gpu) > > return -EINVAL; > > Does this actually work? It seems like you would hit a null ptr deref > in msm_gem_init_vma().. and in general I think a lot of code paths > would be surprised by a null address space, so this seems like a risky > idea. oh, actually, I suppose it is ok, since in the vram carveout case we create the vma up front when the gem obj is created.. (still, it does seem a bit fragile.. and easy for folks testing on devices not using vram carvout to break.. hmm..) BR, -R > Maybe instead we should be creating an address space for the vram carveout? > > BR, > -R > > > > /* > > -- > > 2.29.2 > >
On 1/8/21 12:36 AM, Rob Clark wrote: > On Thu, Jan 7, 2021 at 9:20 AM Rob Clark <robdclark@gmail.com> wrote: >> >> On Sat, Jan 2, 2021 at 12:26 PM Iskren Chernev <iskren.chernev@gmail.com> wrote: >>> >>> The msm_gem_get_iova should be guarded with gpu != NULL and not aspace >>> != NULL, because aspace is NULL when using vram carveout. >>> >>> Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") >>> >>> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> >>> --- >>> drivers/gpu/drm/msm/msm_drv.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c >>> index c5e61cb3356df..c1953fb079133 100644 >>> --- a/drivers/gpu/drm/msm/msm_drv.c >>> +++ b/drivers/gpu/drm/msm/msm_drv.c >>> @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, >>> struct drm_file *file, struct drm_gem_object *obj, >>> uint64_t *iova) >>> { >>> + struct msm_drm_private *priv = dev->dev_private; >>> struct msm_file_private *ctx = file->driver_priv; >>> >>> - if (!ctx->aspace) >>> + if (!priv->gpu) >>> return -EINVAL; >> >> Does this actually work? It seems like you would hit a null ptr deref >> in msm_gem_init_vma().. and in general I think a lot of code paths >> would be surprised by a null address space, so this seems like a risky >> idea. > > oh, actually, I suppose it is ok, since in the vram carveout case we > create the vma up front when the gem obj is created.. > > (still, it does seem a bit fragile.. and easy for folks testing on > devices not using vram carvout to break.. hmm..) In _msm_gem_new add_vma is called with NULL, so consequently lookup_vma finds it when aspace is NULL. Also, this is how the code was before the "breaking" change, so it should not be worse. I'll be happy to work on refactoring this a bit, but some some documentation about the different gpu/mdp pieces and how they fit together won't hurt. Regards, Iskren > BR, > -R > >> Maybe instead we should be creating an address space for the vram carveout? >> >> BR, >> -R >> >> >>> /* >>> -- >>> 2.29.2 >>>
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index c5e61cb3356df..c1953fb079133 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -775,9 +775,10 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, struct drm_file *file, struct drm_gem_object *obj, uint64_t *iova) { + struct msm_drm_private *priv = dev->dev_private; struct msm_file_private *ctx = file->driver_priv; - if (!ctx->aspace) + if (!priv->gpu) return -EINVAL; /*
The msm_gem_get_iova should be guarded with gpu != NULL and not aspace != NULL, because aspace is NULL when using vram carveout. Fixes: 933415e24bd0d ("drm/msm: Add support for private address space instances") Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> --- drivers/gpu/drm/msm/msm_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)