Re: [PATCH] bitmap: Fix return values to be unsigned

From: Kees Cook <keescook@chromium.org>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Yury Norov <yury.norov@gmail.com>,
	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
Subject: Re: [PATCH] bitmap: Fix return values to be unsigned
Date: Tue, 17 May 2022 14:15:41 -0700	[thread overview]
Message-ID: <202205171414.D21437E7@keescook> (raw)
In-Reply-To: <b443e5d7-0803-cfa4-89ab-dec1637953bf@rasmusvillemoes.dk>

On Tue, May 17, 2022 at 08:49:38AM +0200, Rasmus Villemoes wrote:
> 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.

Cool. I think I should split this into two patches.

> 
> >  
> >  #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.

Yeah, all I find as a hint is:

/* FIXME: better would be to fix all architectures to never return
          > MAX_NUMNODES, then the silly min_ts could be dropped. */

which also predates git history.

> 
> >  
> >  #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.

Agreed; I'll drop this change.

Thanks!

-- 
Kees Cook