Re: [Nouveau] [PATCH v8 5/8] mm: Device exclusive memory access
Hi, Alistair, The overall patch looks good to me, however I have a few comments or questions inlined below. On Wed, Apr 07, 2021 at 06:42:35PM +1000, Alistair Popple wrote: > Some devices require exclusive write access to shared virtual > memory (SVM) ranges to perform atomic operations on that memory. This > requires CPU page tables to be updated to deny access whilst atomic > operations are occurring. > > In order to do this introduce a new swap entry > type (SWP_DEVICE_EXCLUSIVE). When a SVM range needs to be marked for > exclusive access by a device all page table mappings for the particular > range are replaced with device exclusive swap entries. This causes any > CPU access to the page to result in a fault. > > Faults are resovled by replacing the faulting entry with the original > mapping. This results in MMU notifiers being called which a driver uses > to update access permissions such as revoking atomic access. After > notifiers have been called the device will no longer have exclusive > access to the region. > > Signed-off-by: Alistair Popple > Reviewed-by: Christoph Hellwig > > --- > > v8: > * Remove device exclusive entries on fork rather than copy them. > > v7: > * Added Christoph's Reviewed-by. > * Minor cosmetic cleanups suggested by Christoph. > * Replace mmu_notifier_range_init_migrate/exclusive with > mmu_notifier_range_init_owner as suggested by Christoph. > * Replaced lock_page() with lock_page_retry() when handling faults. > * Restrict to anonymous pages for now. > > v6: > * Fixed a bisectablity issue due to incorrectly applying the rename of > migrate_pgmap_owner to the wrong patches for Nouveau and hmm_test. > > v5: > * Renamed range->migrate_pgmap_owner to range->owner. May be nicer to mention this rename in commit message (or make it a separate patch)? > * Added MMU_NOTIFY_EXCLUSIVE to allow passing of a driver cookie which > allows notifiers called as a result of make_device_exclusive_range() to > be ignored. > * Added a check to try_to_protect_one() to detect if the pages originally > returned from get_user_pages() have been unmapped or not. > * Removed check_device_exclusive_range() as it is no longer required with > the other changes. > * Documentation update. > > v4: > * Add function to check that mappings are still valid and exclusive. > * s/long/unsigned long/ in make_device_exclusive_entry(). > --- > Documentation/vm/hmm.rst | 19 ++- > drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +- > include/linux/mmu_notifier.h | 26 ++-- > include/linux/rmap.h | 4 + > include/linux/swap.h | 4 +- > include/linux/swapops.h | 44 +- > lib/test_hmm.c| 2 +- > mm/hmm.c | 5 + > mm/memory.c | 176 +++-- > mm/migrate.c | 10 +- > mm/mprotect.c | 8 + > mm/page_vma_mapped.c | 9 +- > mm/rmap.c | 210 ++ > 13 files changed, 487 insertions(+), 32 deletions(-) > > diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst > index 09e28507f5b2..a14c2938e7af 100644 > --- a/Documentation/vm/hmm.rst > +++ b/Documentation/vm/hmm.rst > @@ -332,7 +332,7 @@ between device driver specific code and shared common > code: > walks to fill in the ``args->src`` array with PFNs to be migrated. > The ``invalidate_range_start()`` callback is passed a > ``struct mmu_notifier_range`` with the ``event`` field set to > - ``MMU_NOTIFY_MIGRATE`` and the ``migrate_pgmap_owner`` field set to > + ``MMU_NOTIFY_MIGRATE`` and the ``owner`` field set to > the ``args->pgmap_owner`` field passed to migrate_vma_setup(). This is > allows the device driver to skip the invalidation callback and only > invalidate device private MMU mappings that are actually migrating. > @@ -405,6 +405,23 @@ between device driver specific code and shared common > code: > > The lock can now be released. > > +Exclusive access memory > +=== > + > +Some devices have features such as atomic PTE bits that can be used to > implement > +atomic access to system memory. To support atomic operations to a shared > virtual > +memory page such a device needs access to that page which is exclusive of any > +userspace access from the CPU. The ``make_device_exclusive_range()`` function > +can be used to make a memory range inaccessible from userspace. > + > +This replaces all mappings for pages in the given range with special swap > +entries. Any attempt to access the swap entry results in a fault which is > +resovled by replacing the entry with the original mapping. A driver gets > +notified that the mapping has been changed by MMU notifiers, after which > point > +it will no longer have exclusive access to the page. Exclusive access is > +guranteed to last until the
Re: [Nouveau] [PATCH v8 1/8] mm: Remove special swap entry functions
On Wed, Apr 07, 2021 at 06:42:31PM +1000, Alistair Popple wrote:
> +static inline struct page *pfn_swap_entry_to_page(swp_entry_t entry)
> +{
> + struct page *p = pfn_to_page(swp_offset(entry));
> +
> + /*
> + * Any use of migration entries may only occur while the
> + * corresponding page is locked
> + */
> + BUG_ON(is_migration_entry(entry) && !PageLocked(p));
> +
> + return p;
> +}
Would swap_pfn_entry_to_page() be slightly better?
The thing is it's very easy to read pfn_*() as a function to take a pfn as
parameter...
Since I'm also recently working on some swap-related new ptes [1], I'm thinking
whether we could name these swap entries as "swap XXX entries". Say, "swap
hwpoison entry", "swap pfn entry" (which is a superset of "swap migration
entry", "swap device exclusive entry", ...). That's where I came with the
above swap_pfn_entry_to_page(), then below will be naturally
is_swap_pfn_entry().
No strong opinion as this is already a v8 series (and sorry to chim in this
late), just to raise this up.
[1] https://lore.kernel.org/lkml/20210427161317.50682-1-pet...@redhat.com/
Thanks,
> +
> +/*
> + * A pfn swap entry is a special type of swap entry that always has a pfn
> stored
> + * in the swap offset. They are used to represent unaddressable device memory
> + * and to restrict access to a page undergoing migration.
> + */
> +static inline bool is_pfn_swap_entry(swp_entry_t entry)
> +{
> + return is_migration_entry(entry) || is_device_private_entry(entry);
> +}
--
Peter Xu
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 0/7] Per client engine busyness
[Public] Cycling some of the Nvidia/nouveau guys here too. I think there is a benefit on trying to estandarize how fdinfo can be used to expose per engine and device memory utilization. Another of the advantages of going the /proc/ way instead of the sysfs debugfs approach is that you inherit the access lists directly from the distribution and you don't need to start messing with ownership and group access. By default an user can monitor its own processes as long as /proc is mounted. I am not saying that fdinfo or the way we implemented is 100% the way to go, but I'd rather have a solution within the confines of proc first. David From: Nieto, David M Sent: Monday, May 17, 2021 11:02 AM To: Tvrtko Ursulin ; Daniel Vetter ; Koenig, Christian Cc: Alex Deucher ; Intel Graphics Development ; Maling list - DRI developers Subject: Re: [PATCH 0/7] Per client engine busyness The format is simple: : % we also have entries for the memory mapped: mem : KiB On my submission https://lists.freedesktop.org/archives/amd-gfx/2021-May/063149.html I added a python script to print out the info. It has a CPU usage lower that top, for example. To be absolutely honest, I agree that there is an overhead, but It might not be as much as you fear. From: Tvrtko Ursulin Sent: Monday, May 17, 2021 9:00 AM To: Nieto, David M ; Daniel Vetter ; Koenig, Christian Cc: Alex Deucher ; Intel Graphics Development ; Maling list - DRI developers Subject: Re: [PATCH 0/7] Per client engine busyness On 17/05/2021 15:39, Nieto, David M wrote: > [AMD Official Use Only] > > > Maybe we could try to standardize how the different submission ring > usage gets exposed in the fdinfo? We went the simple way of just > adding name and index, but if someone has a suggestion on how else we > could format them so there is commonality across vendors we could just > amend those. Could you paste an example of your format? Standardized fdinfo sounds good to me in principle. But I would also like people to look at the procfs proposal from Chris, - link to which I have pasted elsewhere in the thread. Only potential issue with fdinfo I see at the moment is a bit of an extra cost in DRM client discovery (compared to my sysfs series and also procfs RFC from Chris). It would require reading all processes (well threads, then maybe aggregating threads into parent processes), all fd symlinks, and doing a stat on them to figure out which ones are DRM devices. Btw is DRM_MAJOR 226 consider uapi? I don't see it in uapi headers. > I’d really like to have the process managers tools display GPU usage > regardless of what vendor is installed. Definitely. Regards, Tvrtko ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 0/7] Per client engine busyness
On Monday, May 17th, 2021 at 8:16 PM, Nieto, David M wrote: > Btw is DRM_MAJOR 226 consider uapi? I don't see it in uapi headers. It's not in the headers, but it's de facto uAPI, as seen in libdrm: > git grep 226 xf86drm.c 99:#define DRM_MAJOR 226 /* Linux */ ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
On Mon, May 17, 2021 at 09:32:44AM -0400, Jeremy Cline wrote:
> On Mon, May 17, 2021 at 11:19:02AM +0200, Thierry Reding wrote:
> > On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> > > On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > > > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have
> > > > to
> > > > do it through nouveau_bo_ref.
> > > >
> > > > ==
> > > > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> > > >
> > > > Use-after-free write at 0x4dc4663c (in kfence-#44):
> > > > ttm_bo_put+0x11/0x40 [ttm]
> > > > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > > drm_ioctl+0x215/0x390 [drm]
> > > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > > __x64_sys_ioctl+0x83/0xb0
> > > > do_syscall_64+0x33/0x40
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > >
> > > > kfence-#44 [0xc0593b31-0x2e74122b, size=792,
> > > > cache=kmalloc-1k] allocated by task 2657:
> > > > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > > > nouveau_gem_new+0x38/0xf0 [nouveau]
> > > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > > drm_ioctl+0x215/0x390 [drm]
> > > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > > __x64_sys_ioctl+0x83/0xb0
> > > > do_syscall_64+0x33/0x40
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > >
> > > > freed by task 2657:
> > > > ttm_bo_release+0x1cc/0x300 [ttm]
> > > > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > > > ttm_bo_init+0x5e/0xd0 [ttm]
> > > > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > > > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > > drm_ioctl+0x215/0x390 [drm]
> > > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > > __x64_sys_ioctl+0x83/0xb0
> > > > do_syscall_64+0x33/0x40
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > >
> > > > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM
> > > > object"
> > > > Cc: Thierry Reding
> > > > Signed-off-by: Karol Herbst
> > > > ---
> > > > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > > > 1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > > b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > > index c88cbb85f101..1165ff990fb5 100644
> > > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size,
> > > > int align, uint32_t domain,
> > > >
> > > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > > if (ret) {
> > > > - nouveau_bo_ref(NULL, );
> > > > return ret;
> > > > }
> > >
> > > Looking at the surrounding code, I wonder if I just managed to jumble
> > > the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> > > drm_gem_object_init() fails, I don't think it's necessary (though it
> > > also doesn't look harmful) to call drm_gem_object_release().
> > >
> > > However, if nouveau_bo_init() fails, then I think we'd still need to
> > > call drm_gem_object_release(), to make sure to undo the effects of
> > > drm_gem_object_init().
> > >
> > > So I wonder if we need something like this instead:
> > >
> > > --- >8 ---
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > index c88cbb85f101..9b6055116f30 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size,
> > > int align, uint32_t domain,
> > >* to the caller, instead of a normal nouveau_bo ttm reference. */
> > > ret = drm_gem_object_init(drm->dev, >bo.base, size);
> > > if (ret) {
> > > - drm_gem_object_release(>bo.base);
> > > kfree(nvbo);
> > > return ret;
> > > }
> > >
> > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > if (ret) {
> > > - nouveau_bo_ref(NULL, );
> > > + drm_gem_object_release(>bo.base);
> > > return ret;
> > > }
> > >
> > > --- >8 ---
> > >
> > > Thierry
> >
> > Adding Jeremy for visibility.
> >
>
> Admittedly I only skimmed the code so I'm not extremely confident in my
> analysis, but isn't that handled by the nouveau_bo_del_ttm() callback
> which should get called after the last reference is dropped with
> nouveau_bo_ref?
Yes, it should. But the point here is that we need to get rid of that
nouveau_bo_ref() call to avoid the use-after-free (which is actually
more like a use-before-init in this case, because at this point the
buffer object hasn't been fully initialized yet), so we won't actually
be dropping the
Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
On Mon, May 17, 2021 at 11:19:02AM +0200, Thierry Reding wrote:
> On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> > On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > > do it through nouveau_bo_ref.
> > >
> > > ==
> > > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> > >
> > > Use-after-free write at 0x4dc4663c (in kfence-#44):
> > > ttm_bo_put+0x11/0x40 [ttm]
> > > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > kfence-#44 [0xc0593b31-0x2e74122b, size=792,
> > > cache=kmalloc-1k] allocated by task 2657:
> > > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > > nouveau_gem_new+0x38/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > freed by task 2657:
> > > ttm_bo_release+0x1cc/0x300 [ttm]
> > > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > > ttm_bo_init+0x5e/0xd0 [ttm]
> > > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM
> > > object"
> > > Cc: Thierry Reding
> > > Signed-off-by: Karol Herbst
> > > ---
> > > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > index c88cbb85f101..1165ff990fb5 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size,
> > > int align, uint32_t domain,
> > >
> > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > if (ret) {
> > > - nouveau_bo_ref(NULL, );
> > > return ret;
> > > }
> >
> > Looking at the surrounding code, I wonder if I just managed to jumble
> > the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> > drm_gem_object_init() fails, I don't think it's necessary (though it
> > also doesn't look harmful) to call drm_gem_object_release().
> >
> > However, if nouveau_bo_init() fails, then I think we'd still need to
> > call drm_gem_object_release(), to make sure to undo the effects of
> > drm_gem_object_init().
> >
> > So I wonder if we need something like this instead:
> >
> > --- >8 ---
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > index c88cbb85f101..9b6055116f30 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size,
> > int align, uint32_t domain,
> > * to the caller, instead of a normal nouveau_bo ttm reference. */
> > ret = drm_gem_object_init(drm->dev, >bo.base, size);
> > if (ret) {
> > - drm_gem_object_release(>bo.base);
> > kfree(nvbo);
> > return ret;
> > }
> >
> > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > if (ret) {
> > - nouveau_bo_ref(NULL, );
> > + drm_gem_object_release(>bo.base);
> > return ret;
> > }
> >
> > --- >8 ---
> >
> > Thierry
>
> Adding Jeremy for visibility.
>
Admittedly I only skimmed the code so I'm not extremely confident in my
analysis, but isn't that handled by the nouveau_bo_del_ttm() callback
which should get called after the last reference is dropped with
nouveau_bo_ref?
- Jeremy
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
On Mon, May 17, 2021 at 11:17 AM Thierry Reding wrote:
>
> On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> > On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > > do it through nouveau_bo_ref.
> > >
> > > ==
> > > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> > >
> > > Use-after-free write at 0x4dc4663c (in kfence-#44):
> > > ttm_bo_put+0x11/0x40 [ttm]
> > > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > kfence-#44 [0xc0593b31-0x2e74122b, size=792,
> > > cache=kmalloc-1k] allocated by task 2657:
> > > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > > nouveau_gem_new+0x38/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > freed by task 2657:
> > > ttm_bo_release+0x1cc/0x300 [ttm]
> > > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > > ttm_bo_init+0x5e/0xd0 [ttm]
> > > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > > drm_ioctl_kernel+0xb2/0x100 [drm]
> > > drm_ioctl+0x215/0x390 [drm]
> > > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > > __x64_sys_ioctl+0x83/0xb0
> > > do_syscall_64+0x33/0x40
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM
> > > object"
> > > Cc: Thierry Reding
> > > Signed-off-by: Karol Herbst
> > > ---
> > > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > index c88cbb85f101..1165ff990fb5 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size,
> > > int align, uint32_t domain,
> > >
> > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > > if (ret) {
> > > - nouveau_bo_ref(NULL, );
> > > return ret;
> > > }
> >
> > Looking at the surrounding code, I wonder if I just managed to jumble
> > the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> > drm_gem_object_init() fails, I don't think it's necessary (though it
> > also doesn't look harmful) to call drm_gem_object_release().
> >
> > However, if nouveau_bo_init() fails, then I think we'd still need to
> > call drm_gem_object_release(), to make sure to undo the effects of
> > drm_gem_object_init().
> >
> > So I wonder if we need something like this instead:
> >
> > --- >8 ---
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > index c88cbb85f101..9b6055116f30 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size,
> > int align, uint32_t domain,
> >* to the caller, instead of a normal nouveau_bo ttm reference. */
> > ret = drm_gem_object_init(drm->dev, >bo.base, size);
> > if (ret) {
> > - drm_gem_object_release(>bo.base);
> > kfree(nvbo);
> > return ret;
> > }
> >
> > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > if (ret) {
> > - nouveau_bo_ref(NULL, );
> > + drm_gem_object_release(>bo.base);
> > return ret;
> > }
> >
I was looking at this already and fixed the above part in
925681454d7b557d404b5d28ef4469fac1b2e105, but yeah.. maybe calling
drm_gem_object_release up there is indeed not needed. Would have to
take a deeper look as well.
> > --- >8 ---
> >
> > Thierry
>
> Adding Jeremy for visibility.
>
> Thierry
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 0/3] drm: Remove some includes of drm_legacy.h
On Sun, 16 May 2021, Thomas Zimmermann wrote: > Remove include statements for DRM legacy headers. None of these > dependencies is required. Built-tested w/o CONFIG_DRM_LEGACY set. > > These patches should probably go through drm-misc, like the rest > of the legacy cleanups. Acked-by: Jani Nikula > > Thomas Zimmermann (3): > drm/i915: Don't include drm_legacy.h > drm/nouveau: Don't include drm_legacy.h > drm: Don't include drm_legacy.h in drm_lease.c > > drivers/gpu/drm/drm_lease.c | 1 - > drivers/gpu/drm/i915/gem/i915_gem_phys.c | 1 - > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/nouveau/nouveau_ttm.c| 2 -- > 4 files changed, 5 deletions(-) > > > base-commit: 77fc6f68ed347b0a4c6969f6adac70026d5b1449 > prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d > prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24 > prerequisite-patch-id: c59ca2ddb182af06006fa360ad3e90fe16b93d3a > prerequisite-patch-id: 8c45deec68d6ab65d66f551b51b12acf2e9ae0b4 > prerequisite-patch-id: 742f08083f0d5776068a761b1e2432e8edc2bdf8 > prerequisite-patch-id: 39cfaf5f337ec53d3237bf2a700e77c84f789039 > -- > 2.31.1 > -- Jani Nikula, Intel Open Source Graphics Center ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
On Mon, May 17, 2021 at 10:56:29AM +0200, Thierry Reding wrote:
> On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> > If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> > do it through nouveau_bo_ref.
> >
> > ==
> > BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
> >
> > Use-after-free write at 0x4dc4663c (in kfence-#44):
> > ttm_bo_put+0x11/0x40 [ttm]
> > nouveau_gem_new+0xc1/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > kfence-#44 [0xc0593b31-0x2e74122b, size=792,
> > cache=kmalloc-1k] allocated by task 2657:
> > nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> > nouveau_gem_new+0x38/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > freed by task 2657:
> > ttm_bo_release+0x1cc/0x300 [ttm]
> > ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> > ttm_bo_init+0x5e/0xd0 [ttm]
> > nouveau_bo_init+0xaf/0xc0 [nouveau]
> > nouveau_gem_new+0x7f/0xf0 [nouveau]
> > nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> > drm_ioctl_kernel+0xb2/0x100 [drm]
> > drm_ioctl+0x215/0x390 [drm]
> > nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> > __x64_sys_ioctl+0x83/0xb0
> > do_syscall_64+0x33/0x40
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> > Cc: Thierry Reding
> > Signed-off-by: Karol Herbst
> > ---
> > drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > index c88cbb85f101..1165ff990fb5 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> > @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int
> > align, uint32_t domain,
> >
> > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> > if (ret) {
> > - nouveau_bo_ref(NULL, );
> > return ret;
> > }
>
> Looking at the surrounding code, I wonder if I just managed to jumble
> the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
> drm_gem_object_init() fails, I don't think it's necessary (though it
> also doesn't look harmful) to call drm_gem_object_release().
>
> However, if nouveau_bo_init() fails, then I think we'd still need to
> call drm_gem_object_release(), to make sure to undo the effects of
> drm_gem_object_init().
>
> So I wonder if we need something like this instead:
>
> --- >8 ---
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index c88cbb85f101..9b6055116f30 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int
> align, uint32_t domain,
>* to the caller, instead of a normal nouveau_bo ttm reference. */
> ret = drm_gem_object_init(drm->dev, >bo.base, size);
> if (ret) {
> - drm_gem_object_release(>bo.base);
> kfree(nvbo);
> return ret;
> }
>
> ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> if (ret) {
> - nouveau_bo_ref(NULL, );
> + drm_gem_object_release(>bo.base);
> return ret;
> }
>
> --- >8 ---
>
> Thierry
Adding Jeremy for visibility.
Thierry
signature.asc
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] nouveau/gem: fix user-after-free in nouveau_gem_new
On Tue, May 11, 2021 at 06:35:53PM +0200, Karol Herbst wrote:
> If ttm_bo_init fails it will already call ttm_bo_put, so we don't have to
> do it through nouveau_bo_ref.
>
> ==
> BUG: KFENCE: use-after-free write in ttm_bo_put+0x11/0x40 [ttm]
>
> Use-after-free write at 0x4dc4663c (in kfence-#44):
> ttm_bo_put+0x11/0x40 [ttm]
> nouveau_gem_new+0xc1/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> kfence-#44 [0xc0593b31-0x2e74122b, size=792,
> cache=kmalloc-1k] allocated by task 2657:
> nouveau_bo_alloc+0x63/0x4c0 [nouveau]
> nouveau_gem_new+0x38/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> freed by task 2657:
> ttm_bo_release+0x1cc/0x300 [ttm]
> ttm_bo_init_reserved+0x2ec/0x300 [ttm]
> ttm_bo_init+0x5e/0xd0 [ttm]
> nouveau_bo_init+0xaf/0xc0 [nouveau]
> nouveau_gem_new+0x7f/0xf0 [nouveau]
> nouveau_gem_ioctl_new+0x53/0xf0 [nouveau]
> drm_ioctl_kernel+0xb2/0x100 [drm]
> drm_ioctl+0x215/0x390 [drm]
> nouveau_drm_ioctl+0x55/0xa0 [nouveau]
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Fixes: 019cbd4a4feb3 "drm/nouveau: Initialize GEM object before TTM object"
> Cc: Thierry Reding
> Signed-off-by: Karol Herbst
> ---
> drivers/gpu/drm/nouveau/nouveau_gem.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
> b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index c88cbb85f101..1165ff990fb5 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -212,7 +212,6 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int
> align, uint32_t domain,
>
> ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
> if (ret) {
> - nouveau_bo_ref(NULL, );
> return ret;
> }
Looking at the surrounding code, I wonder if I just managed to jumble
the cleanup paths for drm_gem_object_init() and nouveau_bo_init(). If
drm_gem_object_init() fails, I don't think it's necessary (though it
also doesn't look harmful) to call drm_gem_object_release().
However, if nouveau_bo_init() fails, then I think we'd still need to
call drm_gem_object_release(), to make sure to undo the effects of
drm_gem_object_init().
So I wonder if we need something like this instead:
--- >8 ---
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index c88cbb85f101..9b6055116f30 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -205,14 +205,13 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int
align, uint32_t domain,
* to the caller, instead of a normal nouveau_bo ttm reference. */
ret = drm_gem_object_init(drm->dev, >bo.base, size);
if (ret) {
- drm_gem_object_release(>bo.base);
kfree(nvbo);
return ret;
}
ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL);
if (ret) {
- nouveau_bo_ref(NULL, );
+ drm_gem_object_release(>bo.base);
return ret;
}
--- >8 ---
Thierry
signature.asc
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
