lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 31 Mar 2022 21:28:36 +0300 From: Dmitry Osipenko <dmitry.osipenko@...labora.com> To: Rob Clark <robdclark@...il.com>, dri-devel@...ts.freedesktop.org Cc: freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, Rob Clark <robdclark@...omium.org>, Sean Paul <sean@...rly.run>, Abhinav Kumar <quic_abhinavk@...cinc.com>, David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>, open list <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin On 3/31/22 21:27, Dmitry Osipenko wrote: > On 3/30/22 23:47, Rob Clark wrote: >> From: Rob Clark <robdclark@...omium.org> >> >> Combines duplicate vma lookup in the get_and_pin path. >> >> Signed-off-by: Rob Clark <robdclark@...omium.org> >> --- >> drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- >> 1 file changed, 26 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c >> index deafae6feaa8..218744a490a4 100644 >> --- a/drivers/gpu/drm/msm/msm_gem.c >> +++ b/drivers/gpu/drm/msm/msm_gem.c >> @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) >> } >> } >> >> -static int get_iova_locked(struct drm_gem_object *obj, >> - struct msm_gem_address_space *aspace, uint64_t *iova, >> +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj, >> + struct msm_gem_address_space *aspace, >> u64 range_start, u64 range_end) >> { >> struct msm_gem_vma *vma; >> - int ret = 0; >> >> GEM_WARN_ON(!msm_gem_is_locked(obj)); >> >> vma = lookup_vma(obj, aspace); >> >> if (!vma) { >> + int ret; >> + >> vma = add_vma(obj, aspace); >> if (IS_ERR(vma)) >> - return PTR_ERR(vma); >> + return vma; >> >> ret = msm_gem_init_vma(aspace, vma, obj->size, >> range_start, range_end); >> if (ret) { > You're allocation range_start -> range_end *allocating > >> del_vma(vma); >> - return ret; >> + return ERR_PTR(ret); >> } >> + } else { >> + GEM_WARN_ON(vma->iova < range_start); >> + GEM_WARN_ON((vma->iova + obj->size) > range_end); > > and then comparing range_start -> range_start + obj->size, hence you're > assuming that range_end always equals to obj->size during the allocation. > > I'm not sure what is the idea here.. this looks inconsistent. I think > you wanted to write: > > GEM_WARN_ON(vma->iova < range_start); > GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) > range_end); > > But is it really useful to check whether the new range is inside of the > old range? Shouldn't it be always a error to change the IOVA range > without reallocating vma? > > I'd expect to see: > > GEM_WARN_ON(vma->iova != range_start); > GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) != range_end); > > and then error out if range mismatches.
Powered by blists - more mailing lists