linux-kernel - Re: [PATCH] cpumask and md/raid5: Fix implicit type conversion

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: <625AA6E4-9661-4280-8991-BB931661EF2C@vmware.com>
Date:   Tue, 26 Oct 2021 16:36:17 +0000
From:   Nadav Amit <namit@...are.com>
To:     Jiasheng Jiang <jiasheng@...as.ac.cn>
CC:     "song@...nel.org" <song@...nel.org>,
        "valentin.schneider@....com" <valentin.schneider@....com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "bristot@...hat.com" <bristot@...hat.com>,
        "linux-raid@...r.kernel.org" <linux-raid@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cpumask and md/raid5: Fix implicit type conversion



> On Oct 26, 2021, at 2:26 AM, Jiasheng Jiang <jiasheng@...as.ac.cn> wrote:
> 
> The description of the macro in `include/linux/cpumask.h` says the
> variable 'cpu' can be unsigned int.
> However in the for_each_cpu(), for_each_cpu_wrap() and
> for_each_cpu_and(), its value is assigned to -1.
> That doesn't make sense. Moreover in the cpumask_next(),
> cpumask_next_zero(), cpumask_next_wrap() and cpumask_next_and(),
> 'cpu' will be implicitly type conversed to int if the type is
> unsigned int.
> It is universally accepted that the implicit type conversion is
> terrible.
> Also, having the good programming custom will set an example for
> others.
> Thus, it might be better to fix the macro description of 'cpu' that
> remove the '(optionally unsigned)' and change the definition of 'cpu'
> in `drivers/md/raid5.c` from unsigned long to long.

Implicit casts are dangerous in certain cases. I am not sure the
case you addressed is such.

Sometimes the generated code is more efficient when casting is
avoided, especially when both the size and sign are changed.

However, in practice, the performance impact is negligent.

If you want to address this issue, it would be best, I think,
to add some assertion and actually deal with all the existing
issues (e.g., see below). Anyhow, unless you find a real
functional bug, I would drop the “fixes” tag.

To find additional issues, you can try to use something like:

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 5d4d07a9e1ed..cda89e6e601e 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -230,6 +230,12 @@ int cpumask_any_and_distribute(const struct cpumask *src1p,
                               const struct cpumask *src2p);
 int cpumask_any_distribute(const struct cpumask *srcp);
 
+#include <linux/build_bug.h>
+
+static __always_inline void build_bug_on(bool c) {
+       BUILD_BUG_ON(c);
+}
+
 /**
  * for_each_cpu - iterate over every cpu in a mask
  * @cpu: the (optionally unsigned) integer iterator
@@ -237,10 +243,10 @@ int cpumask_any_distribute(const struct cpumask *srcp);
  *
  * After the loop, cpu is >= nr_cpu_ids.
  */
-#define for_each_cpu(cpu, mask)                                \
-       for ((cpu) = -1;                                \
-               (cpu) = cpumask_next((cpu), (mask)),    \
-               (cpu) < nr_cpu_ids;)
+#define for_each_cpu(cpu, mask)                                        \
+       for ((cpu) = -1, build_bug_on(!__same_type((cpu), int) && !__same_type((cpu), unsigned int);    \
+               (cpu) = cpumask_next((cpu), (mask)),            \
+               (cpu) < nr_cpu_ids; )
 
 /**
  * for_each_cpu_not - iterate over every cpu in a complemented mask