Re: [Intel-gfx] [PATCH] drm/shmem-helper: Fix obj->filp derefence

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jun 16, 2020 at 02:10:10PM +0200, Thomas Zimmermann wrote:
> Hi,
> 
> as discussed on IRC, we still need this patch.
> 
> Am 15.06.20 um 17:10 schrieb Daniel Vetter:
> > I broke that in my refactoring:
> > 
> > commit 7d2cd72a9aa3df3604cafd169a2d4a525afb68ca
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Fri May 29 16:05:42 2020 +0200
> > 
> >     drm/shmem-helpers: Simplify dma-buf importing
> > 
> > Reported-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Fixes: 7d2cd72a9aa3 ("drm/shmem-helpers: Simplify dma-buf importing")
> > Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_gem_shmem_helper.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 0a7e3b664bc2..3e7ee407a17c 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -70,15 +70,17 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private)
> >  	mutex_init(&shmem->vmap_lock);
> >  	INIT_LIST_HEAD(&shmem->madv_list);
> >  
> > -	/*
> > -	 * Our buffers are kept pinned, so allocating them
> > -	 * from the MOVABLE zone is a really bad idea, and
> > -	 * conflicts with CMA. See comments above new_inode()
> > -	 * why this is required _and_ expected if you're
> > -	 * going to pin these pages.
> > -	 */
> > -	mapping_set_gfp_mask(obj->filp->f_mapping, GFP_HIGHUSER |
> > -			     __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> > +	if (!private) {
> 
> I would test for (obj->filp) here, because it's what the branch depends
> on. Your choice. In any case

I was pondering this too, on one hand it's the thing we're using, otoh
it's a direct consequence of the private flag, and the private flag has a
bit the clearer control flow I think - the obj->filp is clear that it's a
NULL check, but it's a lot less clear _why_ it is ok to have obj->filp ==
NULL. Checking for private makes this a bit clearer imo.

But yeah I considered both options. Maybe we should improve the comment in
a follow-up patch? I want to land the bugfix meanwhile, to close the
regression.

> Tested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for testing and reviewing!
-Daniel

> 
> 
> > +		/*
> > +		 * Our buffers are kept pinned, so allocating them
> > +		 * from the MOVABLE zone is a really bad idea, and
> > +		 * conflicts with CMA. See comments above new_inode()
> > +		 * why this is required _and_ expected if you're
> > +		 * going to pin these pages.
> > +		 */
> > +		mapping_set_gfp_mask(obj->filp->f_mapping, GFP_HIGHUSER |
> > +				     __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> > +	}
> >  
> >  	return shmem;
> >  
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx