Re: [Intel-gfx] [PATCH] drm/shmem-helper: Fix obj->filp derefence
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
> > Date: Fri May 29 16:05:42 2020 +0200
> >
> > drm/shmem-helpers: Simplify dma-buf importing
> >
> > Reported-by: Thomas Zimmermann
> > Fixes: 7d2cd72a9aa3 ("drm/shmem-helpers: Simplify dma-buf importing")
> > Cc: Boris Brezillon
> > Cc: Thomas Zimmermann
> > Cc: Gerd Hoffmann
> > Cc: Rob Herring
> > Cc: Noralf Trønnes
> > Signed-off-by: Daniel Vetter
> > ---
> > 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(>vmap_lock);
> > INIT_LIST_HEAD(>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
> Reviewed-by: Thomas Zimmermann
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
Re: [Intel-gfx] [PATCH] drm/shmem-helper: Fix obj->filp derefence
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
> Date: Fri May 29 16:05:42 2020 +0200
>
> drm/shmem-helpers: Simplify dma-buf importing
>
> Reported-by: Thomas Zimmermann
> Fixes: 7d2cd72a9aa3 ("drm/shmem-helpers: Simplify dma-buf importing")
> Cc: Boris Brezillon
> Cc: Thomas Zimmermann
> Cc: Gerd Hoffmann
> Cc: Rob Herring
> Cc: Noralf Trønnes
> Signed-off-by: Daniel Vetter
> ---
> 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(>vmap_lock);
> INIT_LIST_HEAD(>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
Tested-by: Thomas Zimmermann
Reviewed-by: Thomas Zimmermann
> + /*
> + * 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
signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/shmem-helper: Fix obj->filp derefence
Hi Daniel
Am 15.06.20 um 17:10 schrieb Daniel Vetter:
> I broke that in my refactoring:
>
> commit 7d2cd72a9aa3df3604cafd169a2d4a525afb68ca
> Author: Daniel Vetter
> Date: Fri May 29 16:05:42 2020 +0200
>
> drm/shmem-helpers: Simplify dma-buf importing
>
> Reported-by: Thomas Zimmermann
> Fixes: 7d2cd72a9aa3 ("drm/shmem-helpers: Simplify dma-buf importing")
> Cc: Boris Brezillon
> Cc: Thomas Zimmermann
> Cc: Gerd Hoffmann
> Cc: Rob Herring
> Cc: Noralf Trønnes
> Signed-off-by: Daniel Vetter
> ---
> 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(>vmap_lock);
> INIT_LIST_HEAD(>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) {
> + /*
> + * 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);
> + }
This bug is gone, but now I see
[5.577857] [ cut here ]
[5.577881] WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem.c:564
drm_gem_get_pages+0x190/0x1b0
[5.577883] Modules linked in:
[5.577891] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0-1-pae+ #40
[5.577893] Hardware name: MSI MS-6380
/MS-6380 , BIOS 07.00T
[5.577897] EIP: drm_gem_get_pages+0x190/0x1b0
[5.577904] Code: b7 ff 8d 45 b0 e8 30 63 b7 ff e8 6b d8 38 00 eb 9d
8d b4 26 00 00 00 00 66 90 89 fb eb 97 8d 74 26 00 bb f4 ff ff ff eb 8c
90 <0f> 0b bb ea ff ff ff eb 82 8d b4 26 00 00 00 00 0f 0b e9 95 fe ff
[5.577907] EAX: f24c0c00 EBX: f24c0c00 ECX: f3ae1900 EDX:
[5.577909] ESI: EDI: EBP: f3941b50 ESP: f3941afc
[5.577912] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010246
[5.577915] CR0: 80050033 CR2: b7f5c784 CR3: 1b6b8000 CR4: 06f0
[5.577918] Call Trace:
[5.577938] ? _cond_resched+0x18/0x50
[5.577950] drm_gem_shmem_get_pages+0x52/0xa0
[5.577955] drm_gem_shmem_vmap+0xa1/0x160
[5.577963] skms_simple_display_pipe_update+0x68/0xb0
[5.577973] drm_simple_kms_plane_atomic_update+0x23/0x30
[5.577976] drm_atomic_helper_commit_planes+0xba/0x220
[5.577981] drm_atomic_helper_commit_tail+0x33/0x70
[5.577984] commit_tail+0xe7/0x120
[5.577988] drm_atomic_helper_commit+0x107/0x130
[5.577991] ? drm_atomic_helper_setup_commit+0x5a0/0x5a0
[5.577995] drm_atomic_commit+0x3a/0x50
[5.577999] drm_client_modeset_commit_atomic+0x1ae/0x1e0
[5.578004] drm_client_modeset_commit_locked+0x48/0x80
[5.578008] drm_client_modeset_commit+0x20/0x40
[5.578012] drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0x90
[5.578015] drm_fb_helper_set_par+0x2e/0x40
[5.578025] fbcon_init+0x285/0x590
[5.578035] visual_init+0xb9/0x120
[5.578040] do_bind_con_driver.isra.0+0x18a/0x280
[5.578045] do_take_over_console+0x2c/0x40
[5.578049] do_fbcon_takeover+0x5f/0xd0
[5.578053] fbcon_fb_registered+0xb7/0xe0
[5.578057] do_register_framebuffer+0x1ae/0x2e0
[5.578062] register_framebuffer+0x1c/0x30
[5.578065] __drm_fb_helper_initial_config_and_unlock+0x96/0xd0
[5.578069] drm_fbdev_client_hotplug+0x136/0x220
[5.578072] drm_fbdev_generic_setup+0x9f/0x14a
[5.578076] ? skms_device_create.constprop.0+0x9f/0xb0
[5.578079] skms_probe+0x1b/0x20
[5.578083] platform_drv_probe+0x47/0x90
[5.578092] really_probe+0x2a9/0x3f0
[5.578096] driver_probe_device+0xa9/0xf0
[5.578100] ? _cond_resched+0x18/0x50
[5.578103] device_driver_attach+0x99/0xa0
[5.578107] __driver_attach+0x79/0x130
[5.578111] ? device_driver_attach+0xa0/0xa0
[5.578114] bus_for_each_dev+0x5b/0xa0
[5.578118] driver_attach+0x19/0x20
[5.578122] ? device_driver_attach+0xa0/0xa0
[5.578125]
