On Mon, May 30, 2022 at 7:16 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > > Hi > > Am 30.05.22 um 15:47 schrieb Rob Clark: > > On Mon, May 30, 2022 at 12:26 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > >> > >> Hi > >> > >> Am 29.05.22 um 18:29 schrieb Rob Clark: > >>> From: Rob Clark <robdclark@xxxxxxxxxxxx> > >>> > >>> If a GEM object is allocated, and then exported as a dma-buf fd which is > >>> mmap'd before or without the GEM buffer being directly mmap'd, the > >>> vma_node could be unitialized. This leads to a situation where the CPU > >>> mapping is not correctly torn down in drm_vma_node_unmap(). > >> > >> Which drivers are affected by this problem? > >> > >> I checked several drivers and most appear to be initializing the offset > >> during object construction, such as GEM SHMEM. [1] TTM-based drivers > >> also seem unaffected. [2] > >> > >> From a quick grep, only etnaviv, msm and omapdrm appear to be affected? > >> They only seem to run drm_gem_create_mmap_offset() from their > >> ioctl-handling code. > >> > >> If so, I'd say it's preferable to fix these drivers and put a > >> drm_WARN_ONCE() into drm_gem_prime_mmap(). > > > > That is good if fewer drivers are affected, however I disagree with > > your proposal. At least for freedreno userspace, a lot of bo's never > > get mmap'd (either directly of via dmabuf), so we should not be > > allocating a mmap offset unnecessarily. > > I see. > > I the reason I'm arguing against the current patch is that the fix > appears like a workaround and 6 months from now, few will remember why > it's there. Especially since most drivers initialize the offset > correctly. (Not too long ago, I refactored the handling of these mmap > calls throughout DRM drivers and it was confusing at times.) I dispute the "correctly" part.. and that this is a workaround ;-) But I can send a v2 with the addition of a comment explaining the reason, so git-blame archeology isn't required to understand the reasoning BR, -R > So here's another suggestion: I further looked at the 3 drivers that I > mentioned. etnaviv and msm can easily wrap the call to > drm_gem_prime_mmap() and init the offset first. [1][2] omapdrm doesn't > actually use drm_gem_prime_mmap(). The offset can instead be initialized > at the top of the driver's dmabuf mmap function. [3] > > Best regards > Thomas > > [1] > https://elixir.bootlin.com/linux/v5.18/source/drivers/gpu/drm/etnaviv/etnaviv_drv.c#L480 > [2] > https://elixir.bootlin.com/linux/v5.18/source/drivers/gpu/drm/msm/msm_drv.c#L961 > [3] > https://elixir.bootlin.com/linux/v5.18/source/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c#L66 > > > > > BR, > > -R > > > >> Best regards > >> Thomas > >> > >> [1] > >> https://elixir.bootlin.com/linux/v5.18/source/drivers/gpu/drm/drm_gem_shmem_helper.c#L85 > >> [2] > >> https://elixir.bootlin.com/linux/v5.18/source/drivers/gpu/drm/ttm/ttm_bo.c#L1002 > >> > >>> > >>> Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset") > >>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx> > >>> --- > >>> Note, it's possible the issue existed in some related form prior to the > >>> commit tagged with Fixes. > >>> > >>> drivers/gpu/drm/drm_prime.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > >>> index e3f09f18110c..849eea154dfc 100644 > >>> --- a/drivers/gpu/drm/drm_prime.c > >>> +++ b/drivers/gpu/drm/drm_prime.c > >>> @@ -716,6 +716,11 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > >>> struct file *fil; > >>> int ret; > >>> > >>> + /* Ensure that the vma_node is initialized: */ > >>> + ret = drm_gem_create_mmap_offset(obj); > >>> + if (ret) > >>> + return ret; > >>> + > >>> /* Add the fake offset */ > >>> vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > >>> > >> > >> -- > >> Thomas Zimmermann > >> Graphics Driver Developer > >> SUSE Software Solutions Germany GmbH > >> Maxfeldstr. 5, 90409 Nürnberg, Germany > >> (HRB 36809, AG Nürnberg) > >> Geschäftsführer: Ivo Totev > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev
