linux-kernel - Re: [PATCH 3/3] mm/vmalloc: correct a few logic error in __insert_vmap

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: <20160920165441.76e5a01b@roar.ozlabs.ibm.com>
Date:   Tue, 20 Sep 2016 16:54:41 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     zijun_hu <zijun_hu@...o.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, zijun_hu@....com,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org, tj@...nel.org,
        mingo@...nel.org, rientjes@...gle.com, iamjoonsoo.kim@....com,
        mgorman@...hsingularity.net
Subject: Re: [PATCH 3/3] mm/vmalloc: correct a few logic error in
 __insert_vmap_area()

On Tue, 20 Sep 2016 14:02:26 +0800
zijun_hu <zijun_hu@...o.com> wrote:

> From: zijun_hu <zijun_hu@....com>
> 
> correct a few logic error in __insert_vmap_area() since the else if
> condition is always true and meaningless
> 
> avoid endless loop under [un]mapping improper ranges whose boundary
> are not aligned to page
> 
> correct lazy_max_pages() return value if the number of online cpus
> is power of 2
> 
> improve performance for pcpu_get_vm_areas() via optimizing vmap_areas
> overlay checking algorithm and finding near vmap_areas by list_head
> other than rbtree
> 
> simplify /proc/vmallocinfo implementation via seq_file helpers
> for list_head
> 
> Signed-off-by: zijun_hu <zijun_hu@....com>
> Signed-off-by: zijun_hu <zijun_hu@...o.com>

Could you submit each of these changes as a separate patch? Would you
consider using capitalisation and punctuation in the changelog?

Did you measure any performance improvements, or do you have a workload
where vmalloc shows up in profiles?

> @@ -108,6 +108,9 @@ static void vunmap_page_range(unsigned long addr, unsigned long end)
>  	unsigned long next;
>  
>  	BUG_ON(addr >= end);
> +	WARN_ON(!PAGE_ALIGNED(addr | end));

I prefer to avoid mixing bitwise and arithmetic operations unless it's
necessary. Gcc should be able to optimise

WARN_ON(!PAGE_ALIGNED(addr) || !PAGE_ALIGNED(end))

> +	addr = round_down(addr, PAGE_SIZE);

I don't know if it's really necessary to relax the API like this for
internal vmalloc.c functions. If garbage is detected here, it's likely
due to a bug, and I'm not sure that rounding it would solve the problem.

For API functions perhaps it's reasonable -- in such cases you should
consider using WARN_ON_ONCE() or similar.

Thanks,
Nick