linux-kernel - Re: [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <931c87d8-5856-e393-7108-66275ee4099a@suse.cz>
Date:   Fri, 14 Oct 2022 09:35:16 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Alexander Aring <aahringo@...hat.com>, cl@...ux.com
Cc:     penberg@...nel.org, rientjes@...gle.com, iamjoonsoo.kim@....com,
        akpm@...ux-foundation.org, roman.gushchin@...ux.dev,
        42.hyeyoo@...il.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, cluster-devel@...hat.com
Subject: Re: [PATCHv2] mm: slab: comment __GFP_ZERO case for kmem_cache_alloc

On 10/11/22 16:54, Alexander Aring wrote:
> This patch will add a comment for the __GFP_ZERO flag case for
> kmem_cache_alloc(). As the current comment mentioned that the flags only
> matters if the cache has no available objects it's different for the
> __GFP_ZERO flag which will ensure that the returned object is always
> zeroed in any case.
> 
> I have the feeling I run into this question already two times if the
> user need to zero the object or not, but the user does not need to zero
> the object afterwards. However another use of __GFP_ZERO and only zero
> the object if the cache has no available objects would also make no
> sense.

Hmm, but even with the update, the comment is still rather misleading, no?
- can the caller know if the cache has available objects and thus the flags
are irrelevant, in order to pass flags that are potentially wrong (if there
were no objects)? Not really.
- even if cache has available objects, we'll always end up in
slab_pre_alloc_hook doing might_alloc(flags) which will trigger warnings
(given CONFIG_DEBUG_ATOMIC_SLEEP etc.) if the flags are inappropriate for
given context. So they are still "relevant"

So maybe just delete the whole comment? slub.c doesn't have it, and if any
such comment should exist for kmem_cache_alloc() and contain anything useful
and not misleading, it should be probably in include/linux/slab.h anyway?

> Acked-by: Hyeonggon Yoo <42.hyeyoo@...il.com>
> Acked-by: David Rientjes <rientjes@...gle.com>
> Signed-off-by: Alexander Aring <aahringo@...hat.com>
> ---
> changes since v2:
>  - don't make a separate sentence for except
> 
>  mm/slab.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 10e96137b44f..a86879f42f2e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3482,7 +3482,8 @@ void *__kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru,
>   * @flags: See kmalloc().
>   *
>   * Allocate an object from this cache.  The flags are only relevant
> - * if the cache has no available objects.
> + * if the cache has no available objects, except flag __GFP_ZERO which
> + * will zero the returned object.
>   *
>   * Return: pointer to the new object or %NULL in case of error
>   */