netdev - Re: [PATCH 1/7] cpumask: fix checking valid cpu range

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: <xhsmhbkqz4rqr.mognet@vschneid.remote.csb>
Date:   Wed, 28 Sep 2022 13:18:20 +0100
From:   Valentin Schneider <vschneid@...hat.com>
To:     Yury Norov <yury.norov@...il.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Yury Norov <yury.norov@...il.com>
Subject: Re: [PATCH 1/7] cpumask: fix checking valid cpu range

On 19/09/22 14:05, Yury Norov wrote:
> The range of valid CPUs is [0, nr_cpu_ids). Some cpumask functions are
> passed with a shifted CPU index, and for them, the valid range is
> [-1, nr_cpu_ids-1). Currently for those functions, we check the index
> against [-1, nr_cpu_ids), which is wrong.
>
> Signed-off-by: Yury Norov <yury.norov@...il.com>
> ---
>  include/linux/cpumask.h | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index e4f9136a4a63..a1cd4eb1a3d6 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -174,9 +174,8 @@ static inline unsigned int cpumask_last(const struct cpumask *srcp)
>  static inline
>  unsigned int cpumask_next(int n, const struct cpumask *srcp)
>  {
> -	/* -1 is a legal arg here. */
> -	if (n != -1)
> -		cpumask_check(n);
> +	/* n is a prior cpu */
> +	cpumask_check(n + 1);
>       return find_next_bit(cpumask_bits(srcp), nr_cpumask_bits, n + 1);

I'm confused, this makes passing nr_cpu_ids-1 to cpumask_next*() trigger a
warning. The documentation does states:

* @n: the cpu prior to the place to search (ie. return will be > @n)

So n is a valid CPU number (with -1 being the exception for scan
initialization), this shouldn't exclude nr_cpu_ids-1.

IMO passing nr_cpu_ids-1 should be treated the same as passing the
last set bit in a bitmap: no warning, and returns the bitmap
size. Otherwise reaching nr_cpu_ids-1 has to be special-cased by the
calling code which seems like unnecessary boiler plate

For instance, I trigger the cpumask_check() warning there:

3d2dcab932d0:block/blk-mq.c @l2047
        if (--hctx->next_cpu_batch <= 0) {
select_cpu:
                next_cpu = cpumask_next_and(next_cpu, hctx->cpumask, <-----
                                cpu_online_mask);
                if (next_cpu >= nr_cpu_ids)
                        next_cpu = blk_mq_first_mapped_cpu(hctx);
                hctx->next_cpu_batch = BLK_MQ_CPU_WORK_BATCH;
        }

next_cpu is a valid CPU number, shifting it doesn't seem to make sense, and
we do want it to reach nr_cpu_ids-1.