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]
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ