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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Dec 2021 19:40:17 -0800
From:   Davidlohr Bueso <dbueso@...e.de>
To:     Manfred Spraul <manfred@...orfullife.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vasily Averin <vvs@...tuozzo.com>, cgel.zte@...il.com,
        shakeelb@...gle.com, rdunlap@...radead.org, unixbhaskar@...il.com,
        chi.minghao@....com.cn, arnd@...db.de,
        Zeal Robot <zealci@....com.cn>, linux-mm@...ck.org,
        1vier1@....de, stable@...r.kernel.org, mhocko@...nel.org,
        willy@...radead.org, vbabka@...e.cz
Subject: Re: [PATCH] mm/util.c: Make kvfree() safe for calling while holding
 spinlocks

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@...e.de>

> 
> Reported-by: Zeal Robot <zealci@....com.cn>
> Reported-by: Minghao Chi <chi.minghao@....com.cn>
> Link:
> https://lore.kernel.org/all/20211222081026.484058-1-chi.minghao@zte.com.cn/
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo 
> allocation")
> Cc: stable@...r.kernel.org
> Signed-off-by: Manfred Spraul <manfred@...orfullife.com>
> ---
>  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);
>  }