cleanup vfree and vunmap

[Christoph Hellwig]

Hi all,

this little series untangles the vfree and vunmap code path a bit.

For the KASAN maintainers: the interesting patch re KASAN is patch 8.

Note that it depends on 'Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata
region before/after use"' in linux-next.

Changes since v1:
- drop an extra WARN_ON

Diffstat:
vmalloc.c | 304 +++++++++++++++++++++++++++-----------------------------------
1 file changed, 134 insertions(+), 169 deletions(-)

[Christoph Hellwig]

VM_FLUSH_RESET_PERMS is just for use with vmalloc as it is tied to freeing
the underlying pages.

Signed-off-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---
mm/vmalloc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 0781c5a8e0e73d..6957d15d526e46 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2883,6 +2883,9 @@ void *vmap(struct page **pages, unsigned int count,

might_sleep();

+ if (WARN_ON_ONCE(flags & VM_FLUSH_RESET_PERMS))
+ return NULL;
+
/*
* Your top guard is someone else's bottom guard. Not having a top
* guard compromises someone else's mappings too.
--
2.39.0

[Christoph Hellwig]

__vfree is a subset of vfree that just skips a few checks, and which is
only used by vfree and an error cleanup path. Fold __vfree into vfree
and switch the only other caller to call vfree() instead.

Signed-off-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---

mm/vmalloc.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 6957d15d526e46..b989828b45109a 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2801,14 +2801,6 @@ void vfree_atomic(const void *addr)
__vfree_deferred(addr);
}

-static void __vfree(const void *addr)
-{
- if (unlikely(in_interrupt()))
- __vfree_deferred(addr);
- else
- __vunmap(addr, 1);
-}
-
/**
* vfree - Release memory allocated by vmalloc()
* @addr: Memory base address
@@ -2836,8 +2828,10 @@ void vfree(const void *addr)

if (!addr)
return;
-
- __vfree(addr);
+ if (unlikely(in_interrupt()))
+ __vfree_deferred(addr);
+ else
+ __vunmap(addr, 1);
}
EXPORT_SYMBOL(vfree);

@@ -3104,7 +3098,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,

/*
* If not enough pages were obtained to accomplish an
- * allocation request, free them via __vfree() if any.
+ * allocation request, free them via vfree() if any.
*/
if (area->nr_pages != nr_small_pages) {
warn_alloc(gfp_mask, NULL,
@@ -3144,7 +3138,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
return area->addr;

fail:
- __vfree(area->addr);
+ vfree(area->addr);
return NULL;
}

--
2.39.0

[Christoph Hellwig]

Fold __vfree_deferred into vfree_atomic, and call vfree_atomic early on
from vfree if called from interrupt context so that the extra low-level
helper can be avoided.

Signed-off-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---

mm/vmalloc.c | 43 +++++++++++++++++--------------------------
1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index b989828b45109a..fafb6227f4428f 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2769,20 +2769,6 @@ static void __vunmap(const void *addr, int deallocate_pages)
kfree(area);
}

-static inline void __vfree_deferred(const void *addr)
-{
- /*
- * Use raw_cpu_ptr() because this can be called from preemptible
- * context. Preemption is absolutely fine here, because the llist_add()
- * implementation is lockless, so it works even if we are adding to
- * another cpu's list. schedule_work() should be fine with this too.
- */
- struct vfree_deferred *p = raw_cpu_ptr(&vfree_deferred);
-
- if (llist_add((struct llist_node *)addr, &p->list))
- schedule_work(&p->wq);
-}
-
/**
* vfree_atomic - release memory allocated by vmalloc()
* @addr: memory base address
@@ -2792,13 +2778,19 @@ static inline void __vfree_deferred(const void *addr)
*/
void vfree_atomic(const void *addr)
{
- BUG_ON(in_nmi());
+ struct vfree_deferred *p = raw_cpu_ptr(&vfree_deferred);

+ BUG_ON(in_nmi());
kmemleak_free(addr);

- if (!addr)
- return;
- __vfree_deferred(addr);
+ /*
+ * Use raw_cpu_ptr() because this can be called from preemptible
+ * context. Preemption is absolutely fine here, because the llist_add()
+ * implementation is lockless, so it works even if we are adding to
+ * another cpu's list. schedule_work() should be fine with this too.
+ */
+ if (addr && llist_add((struct llist_node *)addr, &p->list))
+ schedule_work(&p->wq);
}

/**
@@ -2820,17 +2812,16 @@ void vfree_atomic(const void *addr)
*/
void vfree(const void *addr)
{
- BUG_ON(in_nmi());
+ if (unlikely(in_interrupt())) {
+ vfree_atomic(addr);
+ return;
+ }

+ BUG_ON(in_nmi());
kmemleak_free(addr);
+ might_sleep();

- might_sleep_if(!in_interrupt());
-
- if (!addr)
- return;

- if (unlikely(in_interrupt()))
- __vfree_deferred(addr);
- else

+ if (addr)
__vunmap(addr, 1);
}
EXPORT_SYMBOL(vfree);
--
2.39.0

[Christoph Hellwig]

Move these two functions around a bit to avoid forward declarations.

Signed-off-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---

mm/vmalloc.c | 105 +++++++++++++++++++++++++--------------------------
1 file changed, 52 insertions(+), 53 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index fafb6227f4428f..daeb28b54663d5 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -89,17 +89,6 @@ struct vfree_deferred {
};
static DEFINE_PER_CPU(struct vfree_deferred, vfree_deferred);

-static void __vunmap(const void *, int);
-
-static void free_work(struct work_struct *w)
-{
- struct vfree_deferred *p = container_of(w, struct vfree_deferred, wq);
- struct llist_node *t, *llnode;
-
- llist_for_each_safe(llnode, t, llist_del_all(&p->list))
- __vunmap((void *)llnode, 1);
-}
-
/*** Page table manipulation functions ***/
static int vmap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
phys_addr_t phys_addr, pgprot_t prot,
@@ -2449,48 +2438,6 @@ static void vmap_init_free_space(void)
}
}

-void __init vmalloc_init(void)
-{
- struct vmap_area *va;
- struct vm_struct *tmp;
- int i;
-
- /*
- * Create the cache for vmap_area objects.
- */
- vmap_area_cachep = KMEM_CACHE(vmap_area, SLAB_PANIC);
-
- for_each_possible_cpu(i) {
- struct vmap_block_queue *vbq;
- struct vfree_deferred *p;
-
- vbq = &per_cpu(vmap_block_queue, i);
- spin_lock_init(&vbq->lock);
- INIT_LIST_HEAD(&vbq->free);
- p = &per_cpu(vfree_deferred, i);
- init_llist_head(&p->list);
- INIT_WORK(&p->wq, free_work);
- }
-
- /* Import existing vmlist entries. */
- for (tmp = vmlist; tmp; tmp = tmp->next) {
- va = kmem_cache_zalloc(vmap_area_cachep, GFP_NOWAIT);
- if (WARN_ON_ONCE(!va))
- continue;
-
- va->va_start = (unsigned long)tmp->addr;
- va->va_end = va->va_start + tmp->size;
- va->vm = tmp;
- insert_vmap_area(va, &vmap_area_root, &vmap_area_list);
- }
-
- /*
- * Now we can initialize a free vmap space.
- */
- vmap_init_free_space();
- vmap_initialized = true;
-}
-
static inline void setup_vmalloc_vm_locked(struct vm_struct *vm,
struct vmap_area *va, unsigned long flags, const void *caller)
{
@@ -2769,6 +2716,15 @@ static void __vunmap(const void *addr, int deallocate_pages)
kfree(area);
}

+static void delayed_vfree_work(struct work_struct *w)
+{
+ struct vfree_deferred *p = container_of(w, struct vfree_deferred, wq);
+ struct llist_node *t, *llnode;
+
+ llist_for_each_safe(llnode, t, llist_del_all(&p->list))
+ __vunmap((void *)llnode, 1);
+}
+

/**
* vfree_atomic - release memory allocated by vmalloc()
* @addr: memory base address

@@ -4315,3 +4271,46 @@ static int __init proc_vmalloc_init(void)
module_init(proc_vmalloc_init);

#endif
+
+void __init vmalloc_init(void)
+{
+ struct vmap_area *va;
+ struct vm_struct *tmp;
+ int i;
+
+ /*
+ * Create the cache for vmap_area objects.
+ */
+ vmap_area_cachep = KMEM_CACHE(vmap_area, SLAB_PANIC);
+
+ for_each_possible_cpu(i) {
+ struct vmap_block_queue *vbq;
+ struct vfree_deferred *p;
+
+ vbq = &per_cpu(vmap_block_queue, i);
+ spin_lock_init(&vbq->lock);
+ INIT_LIST_HEAD(&vbq->free);
+ p = &per_cpu(vfree_deferred, i);
+ init_llist_head(&p->list);
+ INIT_WORK(&p->wq, delayed_vfree_work);
+ }
+
+ /* Import existing vmlist entries. */
+ for (tmp = vmlist; tmp; tmp = tmp->next) {
+ va = kmem_cache_zalloc(vmap_area_cachep, GFP_NOWAIT);
+ if (WARN_ON_ONCE(!va))
+ continue;
+
+ va->va_start = (unsigned long)tmp->addr;
+ va->va_end = va->va_start + tmp->size;
+ va->vm = tmp;
+ insert_vmap_area(va, &vmap_area_root, &vmap_area_list);
+ }
+
+ /*
+ * Now we can initialize a free vmap space.
+ */
+ vmap_init_free_space();
+ vmap_initialized = true;
+}
+
--
2.39.0

[Christoph Hellwig]

__remove_vm_area is the only part of va_remove_mappings that requires
a vmap_area. Move the call out to the caller and only pass the vm_struct
to va_remove_mappings.

Signed-off-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---

mm/vmalloc.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 3c07520b8b821b..ee0d641019c30b 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2614,18 +2614,15 @@ static inline void set_area_direct_map(const struct vm_struct *area,
set_direct_map(area->pages[i]);
}

-/* Handle removing and resetting vm mappings related to the VA's vm_struct. */
-static void va_remove_mappings(struct vmap_area *va, int deallocate_pages)
+/* Handle removing and resetting vm mappings related to the vm_struct. */
+static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
{
- struct vm_struct *area = va->vm;
unsigned long start = ULONG_MAX, end = 0;
unsigned int page_order = vm_area_page_order(area);
int flush_reset = area->flags & VM_FLUSH_RESET_PERMS;
int flush_dmap = 0;
int i;

- __remove_vm_area(va);
-
/* If this is not VM_FLUSH_RESET_PERMS memory, no need for the below. */
if (!flush_reset)
return;
@@ -2691,7 +2688,8 @@ static void __vunmap(const void *addr, int deallocate_pages)

kasan_poison_vmalloc(area->addr, get_vm_area_size(area));

- va_remove_mappings(va, deallocate_pages);
+ __remove_vm_area(va);
+ vm_remove_mappings(area, deallocate_pages);

if (deallocate_pages) {
int i;
--
2.39.0

[Christoph Hellwig]

This adds an extra, never taken, in_interrupt() branch, but will allow
to cut down the maze of vfree helpers.

Reviewed-by: Christoph Hellwig <h...@lst.de>

Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---

mm/vmalloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index daeb28b54663d5..3c07520b8b821b 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2722,7 +2722,7 @@ static void delayed_vfree_work(struct work_struct *w)
struct llist_node *t, *llnode;

llist_for_each_safe(llnode, t, llist_del_all(&p->list))
- __vunmap((void *)llnode, 1);

+ vfree(llnode);
}

/**
--
2.39.0

[Christoph Hellwig]

Use the common helper to find and remove a vmap_area instead of open
coding it.

Signed-off-by: Christoph Hellwig <h...@lst.de>

Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---

mm/vmalloc.c | 33 ++++++++++++---------------------
1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index ee0d641019c30b..97156eab6fe581 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2571,20 +2571,6 @@ struct vm_struct *find_vm_area(const void *addr)
return va->vm;
}

-static struct vm_struct *__remove_vm_area(struct vmap_area *va)
-{
- struct vm_struct *vm;
-
- if (!va || !va->vm)
- return NULL;
-
- vm = va->vm;
- kasan_free_module_shadow(vm);
- free_unmap_vmap_area(va);
-
- return vm;
-}
-
/**
* remove_vm_area - find and remove a continuous kernel virtual area
* @addr: base address
@@ -2597,10 +2583,18 @@ static struct vm_struct *__remove_vm_area(struct vmap_area *va)
*/
struct vm_struct *remove_vm_area(const void *addr)
{
+ struct vmap_area *va;
+ struct vm_struct *vm;
+
might_sleep();

- return __remove_vm_area(
- find_unlink_vmap_area((unsigned long) addr));
+ va = find_unlink_vmap_area((unsigned long)addr);
+ if (!va || !va->vm)
+ return NULL;
+ vm = va->vm;
+ kasan_free_module_shadow(vm);
+ free_unmap_vmap_area(va);
+ return vm;

}

static inline void set_area_direct_map(const struct vm_struct *area,

@@ -2666,7 +2660,6 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)

static void __vunmap(const void *addr, int deallocate_pages)

{
struct vm_struct *area;
- struct vmap_area *va;

if (!addr)
return;
@@ -2675,20 +2668,18 @@ static void __vunmap(const void *addr, int deallocate_pages)
addr))
return;

- va = find_unlink_vmap_area((unsigned long)addr);
- if (unlikely(!va)) {
+ area = remove_vm_area(addr);
+ if (unlikely(!area)) {
WARN(1, KERN_ERR "Trying to vfree() nonexistent vm area (%p)\n",
addr);
return;
}

- area = va->vm;
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));

- __remove_vm_area(va);
vm_remove_mappings(area, deallocate_pages);

if (deallocate_pages) {
--
2.39.0

[Christoph Hellwig]

All these checks apply to the free_vm_area interface as well, so move
them to the common routine.

Signed-off-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Uladzislau Rezki (Sony) <ure...@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;

+
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));
-

[Christoph Hellwig]

vunmap only needs to find and free the vmap_area and vm_strut, so open
code that there and merge the rest of the code into vfree.

Signed-off-by: Christoph Hellwig <h...@lst.de>
---
mm/vmalloc.c | 84 +++++++++++++++++++++++++---------------------------
1 file changed, 41 insertions(+), 43 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 5b432508319a4f..6bd811e4b7561d 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2666,45 +2666,6 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
set_area_direct_map(area, set_direct_map_default_noflush);
}

-static void __vunmap(const void *addr, int deallocate_pages)
-{
- struct vm_struct *area;

-
- if (!addr)
- return;
-

- area = remove_vm_area(addr);
- if (unlikely(!area)) {
- WARN(1, KERN_ERR "Trying to vfree() nonexistent vm area (%p)\n",
- addr);
- return;
- }
-
- vm_remove_mappings(area, deallocate_pages);
-
- if (deallocate_pages) {
- int i;
-
- for (i = 0; i < area->nr_pages; i++) {
- struct page *page = area->pages[i];
-
- BUG_ON(!page);
- 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
- */
- __free_pages(page, 0);
- cond_resched();
- }
- atomic_long_sub(area->nr_pages, &nr_vmalloc_pages);
-
- kvfree(area->pages);
- }
-
- kfree(area);
-}
-

static void delayed_vfree_work(struct work_struct *w)
{

struct vfree_deferred *p = container_of(w, struct vfree_deferred, wq);

@@ -2757,6 +2718,9 @@ void vfree_atomic(const void *addr)

*/
void vfree(const void *addr)
{

+ struct vm_struct *vm;
+ int i;
+
if (unlikely(in_interrupt())) {
vfree_atomic(addr);
return;
@@ -2766,8 +2730,32 @@ void vfree(const void *addr)
kmemleak_free(addr);
might_sleep();

- if (addr)
- __vunmap(addr, 1);
+ if (!addr)
+ return;
+
+ vm = remove_vm_area(addr);
+ if (unlikely(!vm)) {
+ WARN(1, KERN_ERR "Trying to vfree() nonexistent vm area (%p)\n",
+ addr);
+ return;
+ }
+
+ vm_remove_mappings(vm, true);
+ for (i = 0; i < vm->nr_pages; i++) {
+ struct page *page = vm->pages[i];
+
+ BUG_ON(!page);
+ 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
+ */
+ __free_pages(page, 0);
+ cond_resched();
+ }
+ atomic_long_sub(vm->nr_pages, &nr_vmalloc_pages);
+ kvfree(vm->pages);
+ kfree(vm);
}
EXPORT_SYMBOL(vfree);

@@ -2782,10 +2770,20 @@ EXPORT_SYMBOL(vfree);
*/
void vunmap(const void *addr)
{

+ struct vm_struct *vm;
+

BUG_ON(in_interrupt());
might_sleep();
- if (addr)
- __vunmap(addr, 0);
+
+ if (!addr)
+ return;
+ vm = remove_vm_area(addr);
+ if (unlikely(!vm)) {
+ WARN(1, KERN_ERR "Trying to vunmap() nonexistent vm area (%p)\n",
+ addr);
+ return;
+ }
+ kfree(vm);
}
EXPORT_SYMBOL(vunmap);

--
2.39.0

[Christoph Hellwig]

Move the VM_FLUSH_RESET_PERMS to the caller and rename the function
to better describe what it is doing.

Signed-off-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
---

mm/vmalloc.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 6bd811e4b7561d..dfde5324e4803d 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2617,35 +2617,23 @@ static inline void set_area_direct_map(const struct vm_struct *area,
set_direct_map(area->pages[i]);
}

-/* Handle removing and resetting vm mappings related to the vm_struct. */
-static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
+/*
+ * Flush the vm mapping and reset the direct map.
+ */
+static void vm_reset_perms(struct vm_struct *area)
{

unsigned long start = ULONG_MAX, end = 0;
unsigned int page_order = vm_area_page_order(area);

- int flush_reset = area->flags & VM_FLUSH_RESET_PERMS;

int flush_dmap = 0;
int i;

- /* If this is not VM_FLUSH_RESET_PERMS memory, no need for the below. */

- if (!flush_reset)
- return;
-
- /*
- * If not deallocating pages, just do the flush of the VM area and
- * return.
- */
- if (!deallocate_pages) {
- vm_unmap_aliases();
- return;
- }
-
/*
- * If execution gets here, flush the vm mapping and reset the direct
- * map. Find the start and end range of the direct mappings to make sure
+ * Find the start and end range of the direct mappings to make sure that
* the vm_unmap_aliases() flush includes the direct map.
*/
for (i = 0; i < area->nr_pages; i += 1U << page_order) {
unsigned long addr = (unsigned long)page_address(area->pages[i]);
+
if (addr) {
unsigned long page_size;

@@ -2740,7 +2728,8 @@ void vfree(const void *addr)
return;
}

- vm_remove_mappings(vm, true);
+ if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
+ vm_reset_perms(vm);

for (i = 0; i < vm->nr_pages; i++) {

struct page *page = vm->pages[i];

--
2.39.0

[Andrew Morton]

On Sat, 21 Jan 2023 08:10:41 +0100 Christoph Hellwig <h...@lst.de> wrote:

> Hi all,
>
> this little series untangles the vfree and vunmap code path a bit.
>
> For the KASAN maintainers: the interesting patch re KASAN is patch 8.
>
> Note that it depends on 'Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata
> region before/after use"' in linux-next.
>

In what way does it depend? Not textually.

I could merge the series as-is into mm-unstable, but presumably that
tree will now blow up if someone uses qcom_q6v5_mss.c. Which I suspect
is unlikely, but taking a copy of a899d542b687c9b ("Revert "remoteproc:
qcom_q6v5_mss: map/unmap metadata region before/after use"") is easy
enough, so I'll do that.

[Christoph Hellwig]

On Sat, Jan 21, 2023 at 05:20:57PM -0800, Andrew Morton wrote:
> > this little series untangles the vfree and vunmap code path a bit.
> >
> > For the KASAN maintainers: the interesting patch re KASAN is patch 8.
> >
> > Note that it depends on 'Revert "remoteproc: qcom_q6v5_mss: map/unmap metadata
> > region before/after use"' in linux-next.
> >
>
> In what way does it depend? Not textually.

It abuses VM_FLUSH_RESET_PERMS with vmap, which this series explicitly
forbids.

> I could merge the series as-is into mm-unstable, but presumably that
> tree will now blow up if someone uses qcom_q6v5_mss.c. Which I suspect
> is unlikely, but taking a copy of a899d542b687c9b ("Revert "remoteproc:
> qcom_q6v5_mss: map/unmap metadata region before/after use"") is easy
> enough, so I'll do that.

Ok.

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:
> VM_FLUSH_RESET_PERMS is just for use with vmalloc as it is tied to freeing
> the underlying pages.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

Reviewed-by: David Hildenbrand <da...@redhat.com>

--
Thanks,

David / dhildenb

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> Fold __vfree_deferred into vfree_atomic, and call vfree_atomic early on
> from vfree if called from interrupt context so that the extra low-level
> helper can be avoided.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> Move these two functions around a bit to avoid forward declarations.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> This adds an extra, never taken, in_interrupt() branch, but will allow
> to cut down the maze of vfree helpers.
>
> Reviewed-by: Christoph Hellwig <h...@lst.de>

Self-review? :) I assume that was supposed to be a Signed-off-by ...

> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> __vfree is a subset of vfree that just skips a few checks, and which is
> only used by vfree and an error cleanup path. Fold __vfree into vfree
> and switch the only other caller to call vfree() instead.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> __remove_vm_area is the only part of va_remove_mappings that requires
> a vmap_area. Move the call out to the caller and only pass the vm_struct
> to va_remove_mappings.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> Use the common helper to find and remove a vmap_area instead of open
> coding it.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

[David Hildenbrand]

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 <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@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.

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> vunmap only needs to find and free the vmap_area and vm_strut, so open

s/vm_strut/vm_struct/

> code that there and merge the rest of the code into vfree.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> ---

[David Hildenbrand]

On 21.01.23 08:10, Christoph Hellwig wrote:

> Move the VM_FLUSH_RESET_PERMS to the caller and rename the function
> to better describe what it is doing.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> Reviewed-by: Uladzislau Rezki (Sony) <ure...@gmail.com>
> ---

[Christoph Hellwig]

On Mon, Jan 23, 2023 at 11:38:46AM +0100, David Hildenbrand wrote:
> On 21.01.23 08:10, Christoph Hellwig wrote:
>> This adds an extra, never taken, in_interrupt() branch, but will allow
>> to cut down the maze of vfree helpers.
>>
>> Reviewed-by: Christoph Hellwig <h...@lst.de>
>
> Self-review? :) I assume that was supposed to be a Signed-off-by ...

Yes, this should be a singoff:

Signed-off-by: Christoph Hellwig <h...@lst.de>

[Christoph Hellwig]

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.

[David Hildenbrand]

Sure, there are some more !ONCE WARN calls hiding in that file.