| Message ID | 20241211202538.168311-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] vmalloc: Fix accounting with i915 | expand |
On Wed, Dec 11, 2024 at 08:25:37PM +0000, Matthew Wilcox (Oracle) wrote: > If the caller of vmap() specifies VM_MAP_PUT_PAGES (currently only the > i915 driver), we will decrement nr_vmalloc_pages and MEMCG_VMALLOC in > vfree(). These counters are incremented by vmalloc() but not by vmap() > so this will cause an underflow. Check the VM_MAP_PUT_PAGES flag before > decrementing either counter. > > Fixes: b944afc9d64d (mm: add a VM_MAP_PUT_PAGES flag for vmap) > Cc: stable@vger.kernel.org > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> Oh you merged them into one. Good. Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
On 12/12/24 07:25, Matthew Wilcox (Oracle) wrote: > If the caller of vmap() specifies VM_MAP_PUT_PAGES (currently only the > i915 driver), we will decrement nr_vmalloc_pages and MEMCG_VMALLOC in > vfree(). These counters are incremented by vmalloc() but not by vmap() > so this will cause an underflow. Check the VM_MAP_PUT_PAGES flag before > decrementing either counter. > > Fixes: b944afc9d64d (mm: add a VM_MAP_PUT_PAGES flag for vmap) > Cc: stable@vger.kernel.org > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > --- > mm/vmalloc.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index f009b21705c1..5c88d0e90c20 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -3374,7 +3374,8 @@ void vfree(const void *addr) > struct page *page = vm->pages[i]; > > BUG_ON(!page); > - mod_memcg_page_state(page, MEMCG_VMALLOC, -1); > + if (!(vm->flags & VM_MAP_PUT_PAGES)) > + mod_memcg_page_state(page, MEMCG_VMALLOC, -1); > /* > * High-order allocs for huge vmallocs are split, so > * can be freed as an array of order-0 allocations > @@ -3382,7 +3383,8 @@ void vfree(const void *addr) > __free_page(page); > cond_resched(); > } > - atomic_long_sub(vm->nr_pages, &nr_vmalloc_pages); > + if (!(vm->flags & VM_MAP_PUT_PAGES)) > + atomic_long_sub(vm->nr_pages, &nr_vmalloc_pages); > kvfree(vm->pages); > kfree(vm); > } I found another user kunit_iov_vector.c, but even it though it uses VM_MAP_PUT_PAGES, it does not call into vfree() (which I need to check for further bugs) Reviewed-by: Balbir Singh <balbirs@nvidia.com>
On Wed, Dec 11, 2024 at 08:25:37PM +0000, Matthew Wilcox (Oracle) wrote: > If the caller of vmap() specifies VM_MAP_PUT_PAGES (currently only the > i915 driver), we will decrement nr_vmalloc_pages and MEMCG_VMALLOC in > vfree(). These counters are incremented by vmalloc() but not by vmap() > so this will cause an underflow. Check the VM_MAP_PUT_PAGES flag before > decrementing either counter. > > Fixes: b944afc9d64d (mm: add a VM_MAP_PUT_PAGES flag for vmap) > Cc: stable@vger.kernel.org > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> Nice, thanks, looks good to me.
On Wed 11-12-24 20:25:37, Matthew Wilcox wrote: > If the caller of vmap() specifies VM_MAP_PUT_PAGES (currently only the > i915 driver), we will decrement nr_vmalloc_pages and MEMCG_VMALLOC in > vfree(). These counters are incremented by vmalloc() but not by vmap() > so this will cause an underflow. Check the VM_MAP_PUT_PAGES flag before > decrementing either counter. > > Fixes: b944afc9d64d (mm: add a VM_MAP_PUT_PAGES flag for vmap) > Cc: stable@vger.kernel.org > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > mm/vmalloc.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index f009b21705c1..5c88d0e90c20 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -3374,7 +3374,8 @@ void vfree(const void *addr) > struct page *page = vm->pages[i]; > > BUG_ON(!page); > - mod_memcg_page_state(page, MEMCG_VMALLOC, -1); > + if (!(vm->flags & VM_MAP_PUT_PAGES)) > + mod_memcg_page_state(page, MEMCG_VMALLOC, -1); > /* > * High-order allocs for huge vmallocs are split, so > * can be freed as an array of order-0 allocations > @@ -3382,7 +3383,8 @@ void vfree(const void *addr) > __free_page(page); > cond_resched(); > } > - atomic_long_sub(vm->nr_pages, &nr_vmalloc_pages); > + if (!(vm->flags & VM_MAP_PUT_PAGES)) > + atomic_long_sub(vm->nr_pages, &nr_vmalloc_pages); > kvfree(vm->pages); > kfree(vm); > } > -- > 2.45.2
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index f009b21705c1..5c88d0e90c20 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -3374,7 +3374,8 @@ void vfree(const void *addr) struct page *page = vm->pages[i]; BUG_ON(!page); - mod_memcg_page_state(page, MEMCG_VMALLOC, -1); + if (!(vm->flags & VM_MAP_PUT_PAGES)) + mod_memcg_page_state(page, MEMCG_VMALLOC, -1); /* * High-order allocs for huge vmallocs are split, so * can be freed as an array of order-0 allocations @@ -3382,7 +3383,8 @@ void vfree(const void *addr) __free_page(page); cond_resched(); } - atomic_long_sub(vm->nr_pages, &nr_vmalloc_pages); + if (!(vm->flags & VM_MAP_PUT_PAGES)) + atomic_long_sub(vm->nr_pages, &nr_vmalloc_pages); kvfree(vm->pages); kfree(vm); }