Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b443e5d7-0803-cfa4-89ab-dec1637953bf@rasmusvillemoes.dk>
Date:   Tue, 17 May 2022 08:49:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] bitmap: Fix return values to be unsigned
Content-Language: en-US
To:     Kees Cook <keescook@chromium.org>,
        Yury Norov <yury.norov@gmail.com>
Cc:     Christophe de Dinechin <dinechin@redhat.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Zhen Lei <thunder.leizhen@huawei.com>,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
References: <20220517035411.31144-1-keescook@chromium.org>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
In-Reply-To: <20220517035411.31144-1-keescook@chromium.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/05/2022 05.54, Kees Cook wrote:
> Both nodemask and bitmap routines had mixed return values that provided
> potentially signed results that could never happen. This was leading to
> the compiler getting confusing about the range of possible return values
> (it was thinking things could be negative where they could not be). Fix
> all the nodemask and bitmap routines that should be returning unsigned
> (or bool) values. Silences GCC 12 warnings:

So, for the bitmap functions themselves, makes sense, and then also for
the nodemask functions which are merely wrappers around the bitmap
functions (or wrappers around wrappers ...). But see below.

>  
>  #define first_node(src) __first_node(&(src))
> -static inline int __first_node(const nodemask_t *srcp)
> +static inline unsigned int __first_node(const nodemask_t *srcp)
>  {
> -	return min_t(int, MAX_NUMNODES, find_first_bit(srcp->bits, MAX_NUMNODES));
> +	return min_t(unsigned int, MAX_NUMNODES, find_first_bit(srcp->bits, MAX_NUMNODES));
>  }

Unrelated to the type change, but what's that min() doing there in the
first place? Doesn't find_first_bit() already return the nbits argument
if no "first bit" exists (i.e., the bitmap is empty)?

>  #define next_node(n, src) __next_node((n), &(src))
> -static inline int __next_node(int n, const nodemask_t *srcp)
> +static inline unsigned int __next_node(int n, const nodemask_t *srcp)
>  {
> -	return min_t(int,MAX_NUMNODES,find_next_bit(srcp->bits, MAX_NUMNODES, n+1));
> +	return min_t(unsigned int, MAX_NUMNODES, find_next_bit(srcp->bits, MAX_NUMNODES, n+1));
>  }

Same here and a few more places.

It seems to go all the way back to pre-git. Hm. Could be cleaned up
separately I guess.

>  
>  #if defined(CONFIG_NUMA) && (MAX_NUMNODES > 1)
> -extern int node_random(const nodemask_t *maskp);
> +extern unsigned int node_random(const nodemask_t *maskp);

So this one I'm not convinced about. It has a documented return value of
NUMA_NO_NODE aka -1 if the mask is empty. And since it's not a wrapper
around a corresponding bitmap_random() (which would presumably, did it
exist, use the "return nbits if empty" convention), there's no
compelling reason to make its return type unsigned.

>  
> @@ -18,9 +18,9 @@ EXPORT_SYMBOL(__next_node_in);
>   * Return the bit number of a random bit set in the nodemask.
>   * (returns NUMA_NO_NODE if nodemask is empty)
>   */
> -int node_random(const nodemask_t *maskp)
> +unsigned int node_random(const nodemask_t *maskp)
>  {
> -	int w, bit = NUMA_NO_NODE;
> +	unsigned int w, bit = NUMA_NO_NODE;
>  
>  	w = nodes_weight(*maskp);
>  	if (w)

Rasmus