linux-kernel - Re: [PATCH v7 1/2] mm/vmalloc: Moved macros with no functional change happened

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: <ZekW/nGXfTqOlvPZ@MiWiFi-R3L-srv>
Date: Thu, 7 Mar 2024 09:23:10 +0800
From: Baoquan He <bhe@...hat.com>
To: Uladzislau Rezki <urezki@...il.com>
Cc: rulinhuang <rulin.huang@...el.com>, akpm@...ux-foundation.org,
	colin.king@...el.com, hch@...radead.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	lstoakes@...il.com, tianyou.li@...el.com, tim.c.chen@...el.com,
	wangyang.guo@...el.com, zhiguo.zhou@...el.com
Subject: Re: [PATCH v7 1/2] mm/vmalloc: Moved macros with no functional
 change happened

On 03/06/24 at 08:01pm, Uladzislau Rezki wrote:
> On Fri, Mar 01, 2024 at 10:54:16AM -0500, rulinhuang wrote:
.....
> 
> Sorry for the late answer, i also just noticed this email. It was not in
> my inbox...
> 
> OK, now you move part of the per-cpu allocator on the top and leave
> another part down making it split. This is just for the:
> 
> BUG_ON(va_flags & VMAP_RAM);
> 
> VMAP_RAM macro. Do we really need this BUG_ON()?

Sorry, I suggested that when reviewing v5:
https://lore.kernel.org/all/ZdiltpK5fUvwVWtD@MiWiFi-R3L-srv/T/#u

About part of per-cpu kva allocator moving and the split making, I would
argue that we will have vmap_nodes defintion and basic helper functions
like addr_to_node_id() etc at top, and leave other part like
size_to_va_pool(), node_pool_add_va() etc down. These are similar.

While about whether we should add 'BUG_ON(va_flags & VMAP_RAM);', I am
not sure about it. When I suggested that, I am also hesitant. From the
current code, alloc_vmap_area() is called in below three functions, only
__get_vm_area_node() will pass the non-NULL vm. 
 new_vmap_block()     -|
 vm_map_ram()         ----> alloc_vmap_area()
 __get_vm_area_node() -|

It could be wrongly passed in the future? Only checking if vm is
non-NULL makes me feel a little unsafe. While I am fine if removing the
BUG_ON, because there's no worry in the current code. We can wait and
see in the future.

       if (vm) {
               BUG_ON(va_flags & VMAP_RAM);
               setup_vmalloc_vm(vm, va, flags, caller);
       }

Thanks
Baoquan