| Message ID | 20230121071051.1143058-9-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] mm: reject vmap with VM_FLUSH_RESET_PERMS | expand |
On 21.01.23 08:10, Christoph Hellwig wrote: > All these checks apply to the free_vm_area interface as well, so move > them to the common routine. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > mm/vmalloc.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 97156eab6fe581..5b432508319a4f 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2588,11 +2588,20 @@ struct vm_struct *remove_vm_area(const void *addr) > > might_sleep(); > > + if (WARN(!PAGE_ALIGNED(addr), "Trying to vfree() bad address (%p)\n", > + addr)) > + return NULL; While at it, might want to use WARN_ONCE() instead. Reviewed-by: David Hildenbrand <david@redhat.com>
On Mon, Jan 23, 2023 at 11:43:31AM +0100, David Hildenbrand wrote: >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >> index 97156eab6fe581..5b432508319a4f 100644 >> --- a/mm/vmalloc.c >> +++ b/mm/vmalloc.c >> @@ -2588,11 +2588,20 @@ struct vm_struct *remove_vm_area(const void *addr) >> might_sleep(); >> + if (WARN(!PAGE_ALIGNED(addr), "Trying to vfree() bad address (%p)\n", >> + addr)) >> + return NULL; > > While at it, might want to use WARN_ONCE() instead. One thing at a time. But yes, this makes sense and could be an incremental patch.
On 23.01.23 15:50, Christoph Hellwig wrote: > On Mon, Jan 23, 2023 at 11:43:31AM +0100, David Hildenbrand wrote: >>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >>> index 97156eab6fe581..5b432508319a4f 100644 >>> --- a/mm/vmalloc.c >>> +++ b/mm/vmalloc.c >>> @@ -2588,11 +2588,20 @@ struct vm_struct *remove_vm_area(const void *addr) >>> might_sleep(); >>> + if (WARN(!PAGE_ALIGNED(addr), "Trying to vfree() bad address (%p)\n", >>> + addr)) >>> + return NULL; >> >> While at it, might want to use WARN_ONCE() instead. > > One thing at a time. But yes, this makes sense and could be an > incremental patch. Sure, there are some more !ONCE WARN calls hiding in that file.
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 97156eab6fe581..5b432508319a4f 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2588,11 +2588,20 @@ struct vm_struct *remove_vm_area(const void *addr) might_sleep(); + if (WARN(!PAGE_ALIGNED(addr), "Trying to vfree() bad address (%p)\n", + addr)) + return NULL; + va = find_unlink_vmap_area((unsigned long)addr); if (!va || !va->vm) return NULL; vm = va->vm; + + debug_check_no_locks_freed(vm->addr, get_vm_area_size(vm)); + debug_check_no_obj_freed(vm->addr, get_vm_area_size(vm)); kasan_free_module_shadow(vm); + kasan_poison_vmalloc(vm->addr, get_vm_area_size(vm)); + free_unmap_vmap_area(va); return vm; } @@ -2664,10 +2673,6 @@ static void __vunmap(const void *addr, int deallocate_pages) if (!addr) return; - if (WARN(!PAGE_ALIGNED(addr), "Trying to vfree() bad address (%p)\n", - addr)) - return; - area = remove_vm_area(addr); if (unlikely(!area)) { WARN(1, KERN_ERR "Trying to vfree() nonexistent vm area (%p)\n", @@ -2675,11 +2680,6 @@ static void __vunmap(const void *addr, int deallocate_pages) return; } - debug_check_no_locks_freed(area->addr, get_vm_area_size(area)); - debug_check_no_obj_freed(area->addr, get_vm_area_size(area)); - - kasan_poison_vmalloc(area->addr, get_vm_area_size(area)); - vm_remove_mappings(area, deallocate_pages); if (deallocate_pages) {