linux-kernel - Re: [PATCH v2] mm: vmscan: Avoid signedness error for GCC 5.4

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] [day] [month] [year] [list]

Message-ID: <20250507214053.42c90b11@pumpkin>
Date: Wed, 7 May 2025 21:40:53 +0100
From: David Laight <david.laight.linux@...il.com>
To: WangYuli <wangyuli@...ontech.com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, yuzhao@...gle.com, stevensd@...omium.org,
 kaleshsingh@...gle.com, zhanjun@...ontech.com, niecheng1@...ontech.com,
 guanwentao@...ontech.com, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v2] mm: vmscan: Avoid signedness error for GCC 5.4

On Wed,  7 May 2025 12:08:27 +0800
WangYuli <wangyuli@...ontech.com> wrote:

> To the GCC 5.4 compiler, (MAX_NR_TIERS - 1) (i.e., (4U - 1)) is
> unsigned, whereas tier is a signed integer.

That is true for all version of the compiler.
The warning depends on whether the function is inlined or not.

> 
> Then, the __types_ok check within the __careful_cmp_once macro failed,
> triggered BUILD_BUG_ON.
> 
> Use min_t instead of min to circumvent this compiler error.

It isn't a compiler error, it is a coding error.

The correct fix is to change tier to be unsigned.
(and probably 'i' at the same time.)

> 
> Fix follow error with gcc 5.4:
>   mm/vmscan.c: In function ‘read_ctrl_pos’:
>   mm/vmscan.c:3166:728: error: call to ‘__compiletime_assert_887’ declared with attribute error: min(tier, 4U - 1) signedness error
> 
> Cc: Matthew Wilcox <willy@...radead.org>
> Fixes: 37a260870f2c ("mm/mglru: rework type selection")
> Signed-off-by: WangYuli <wangyuli@...ontech.com>
> ---
> Changelog:
>  *v1->v2:
>     1. Fix commit msg.
>     2. Use min_t instead of min.
> ---
>  mm/vmscan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 3783e45bfc92..8d9a82621c4f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3163,7 +3163,7 @@ static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
>  	pos->gain = gain;
>  	pos->refaulted = pos->total = 0;
>  
> -	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
> +	for (i = tier % MAX_NR_TIERS; i <= min_t(int, tier, MAX_NR_TIERS - 1); i++) {

That loop is atrocious.
There are two types of call to the function.
Either 'tier' is a constant 0, a variable 1..3 or a constant 4.
You really don't want it not inlined.
When inlined gcc tracks the domain of 'tier' and knows it is positive.
That stops the error from min().
If not inlined you get terrible code and the warning.

For the loop case (and that includes the 0, then 1..3 loop) you probably
want to ensure the [hist] and [type] array indexes are only done once.
I suspect gcc can't do that for you.

	David

>  		pos->refaulted += lrugen->avg_refaulted[type][i] +
>  				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
>  		pos->total += lrugen->avg_total[type][i] +