Re: [PATCH] mm/util.c: Make kvfree() safe for calling while holding spinlocks

[Davidlohr Bueso]

Cc'ing more mm folks.

On 2021-12-22 11:48, Manfred Spraul wrote:
> One codepath in find_alloc_undo() calls kvfree() while holding a 
> spinlock.
> Since vfree() can sleep this is a bug.

afaict the only other offender is devx_async_cmd_event_destroy_uobj(), 
in drivers/infiniband/hw/mlx5/devx.c. I was expecting to find more, 
actually.

> Previously, the code path used kfree(), and kfree() is safe to be 
> called
> while holding a spinlock.
>
> Minghao proposed to fix this by updating find_alloc_undo().
>
> Alternate proposal to fix this: Instead of changing find_alloc_undo(),
> change kvfree() so that the same rules as for kfree() apply:
> Having different rules for kfree() and kvfree() just asks for bugs.

I agree that it is best to have the same atomic semantics across all 
family of calls.

> Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.

I would not expect the added latency to be a big deal unless under 
serious memory pressure, for which case things are already fragile to 
begin with. Furthermore users of kvfree() are already warned that this 
is the slower choice. Feel free to add my:

Acked-by: Davidlohr Bueso <dbueso@xxxxxxx>

> Reported-by: Zeal Robot <zealci@xxxxxxxxxx>
> Reported-by: Minghao Chi <chi.minghao@xxxxxxxxxx>
> Link:
> https://lore.kernel.org/all/20211222081026.484058-1-chi.minghao@xxxxxxxxxx/
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo 
> allocation")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
> ---
>  mm/util.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/util.c b/mm/util.c
> index 741ba32a43ac..7f9181998835 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -610,12 +610,12 @@ EXPORT_SYMBOL(kvmalloc_node);
>   * It is slightly more efficient to use kfree() or vfree() if you are 
> certain
>   * that you know which one to use.
>   *
> - * Context: Either preemptible task context or not-NMI interrupt.
> + * Context: Any context except NMI interrupt.
>   */
>  void kvfree(const void *addr)
>  {
>  	if (is_vmalloc_addr(addr))
> -		vfree(addr);
> +		vfree_atomic(addr);
>  	else
>  		kfree(addr);
>  }