diff mbox series

nospec: Move array_index_nospec parameter checking into separate macro

Message ID 1517840166-15399-1-git-send-email-will.deacon@arm.com
State Accepted
Commit 8fa80c503b484ddc1abbd10c7cb2ab81f3824a50
Headers show
Series nospec: Move array_index_nospec parameter checking into separate macro | expand

Commit Message

Will Deacon Feb. 5, 2018, 2:16 p.m. UTC 
For architectures providing their own implementation of
array_index_mask_nospec in asm/barrier.h, attempting to use WARN_ONCE to
complain about out-of-range parameters using WARN_ON results in a mess
of mutually-dependent include files.

Rather than unpick the dependencies, simply have the core code in nospec.h
perform the checking for us.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>

---
 include/linux/nospec.h | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

-- 
2.1.4

Comments

Dan Williams Feb. 5, 2018, 6:54 p.m. UTC | #1 
On Mon, Feb 5, 2018 at 6:16 AM, Will Deacon <will.deacon@arm.com> wrote:
> For architectures providing their own implementation of

> array_index_mask_nospec in asm/barrier.h, attempting to use WARN_ONCE to

> complain about out-of-range parameters using WARN_ON results in a mess

> of mutually-dependent include files.

>

> Rather than unpick the dependencies, simply have the core code in nospec.h

> perform the checking for us.

>

> Cc: Dan Williams <dan.j.williams@intel.com>

> Cc: Ingo Molnar <mingo@redhat.com>

> Signed-off-by: Will Deacon <will.deacon@arm.com>


Looks ok to me and produces the same assembly.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Geert Uytterhoeven Feb. 19, 2018, 11:47 a.m. UTC | #2 
Hi Will,

On Mon, Feb 5, 2018 at 3:16 PM, Will Deacon <will.deacon@arm.com> wrote:
> For architectures providing their own implementation of

> array_index_mask_nospec in asm/barrier.h, attempting to use WARN_ONCE to

> complain about out-of-range parameters using WARN_ON results in a mess

> of mutually-dependent include files.

>

> Rather than unpick the dependencies, simply have the core code in nospec.h

> perform the checking for us.

>

> Cc: Dan Williams <dan.j.williams@intel.com>

> Cc: Ingo Molnar <mingo@redhat.com>

> Signed-off-by: Will Deacon <will.deacon@arm.com>

> ---

>  include/linux/nospec.h | 36 +++++++++++++++++++++---------------

>  1 file changed, 21 insertions(+), 15 deletions(-)

>

> diff --git a/include/linux/nospec.h b/include/linux/nospec.h

> index b99bced39ac2..fbc98e2c8228 100644

> --- a/include/linux/nospec.h

> +++ b/include/linux/nospec.h

> @@ -20,20 +20,6 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,

>                                                     unsigned long size)

>  {

>         /*

> -        * Warn developers about inappropriate array_index_nospec() usage.

> -        *

> -        * Even if the CPU speculates past the WARN_ONCE branch, the

> -        * sign bit of @index is taken into account when generating the

> -        * mask.

> -        *

> -        * This warning is compiled out when the compiler can infer that

> -        * @index and @size are less than LONG_MAX.

> -        */

> -       if (WARN_ONCE(index > LONG_MAX || size > LONG_MAX,

> -                       "array_index_nospec() limited to range of [0, LONG_MAX]\n"))

> -               return 0;

> -

> -       /*

>          * Always calculate and emit the mask even if the compiler

>          * thinks the mask is not needed. The compiler does not take

>          * into account the value of @index under speculation.

> @@ -44,6 +30,26 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,

>  #endif

>

>  /*

> + * Warn developers about inappropriate array_index_nospec() usage.

> + *

> + * Even if the CPU speculates past the WARN_ONCE branch, the

> + * sign bit of @index is taken into account when generating the

> + * mask.

> + *

> + * This warning is compiled out when the compiler can infer that

> + * @index and @size are less than LONG_MAX.

> + */

> +#define array_index_mask_nospec_check(index, size)                             \

> +({                                                                             \

> +       if (WARN_ONCE(index > LONG_MAX || size > LONG_MAX,                      \

> +           "array_index_nospec() limited to range of [0, LONG_MAX]\n"))        \

> +               _mask = 0;                                                      \

> +       else                                                                    \

> +               _mask = array_index_mask_nospec(index, size);                   \

> +       _mask;                                                                  \

> +})

> +

> +/*

>   * array_index_nospec - sanitize an array index after a bounds check

>   *

>   * For a code sequence like:

> @@ -61,7 +67,7 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,

>  ({                                                                     \

>         typeof(index) _i = (index);                                     \

>         typeof(size) _s = (size);                                       \

> -       unsigned long _mask = array_index_mask_nospec(_i, _s);          \

> +       unsigned long _mask = array_index_mask_nospec_check(_i, _s);    \

>                                                                         \

>         BUILD_BUG_ON(sizeof(_i) > sizeof(long));                        \

>         BUILD_BUG_ON(sizeof(_s) > sizeof(long));                        \


This change is commit 8fa80c503b484ddc ("nospec: Move array_index_nospec()
parameter checking into separate macro") in v4.16-rc2, and triggers the
following warning with gcc-4.1.2:

    net/wireless/nl80211.c: In function ‘parse_txq_params’:
    net/wireless/nl80211.c:2099: warning: comparison is always false
due to limited range of data type

Reverting the commit gets rid of the warning.

As kisskb hasn't completed the full build of  v4.16-rc2 yet, I don't know if
other compiler versions are affected.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Will Deacon Feb. 19, 2018, 11:54 a.m. UTC | #3 
Hi Geert,

On Mon, Feb 19, 2018 at 12:47:08PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 5, 2018 at 3:16 PM, Will Deacon <will.deacon@arm.com> wrote:

> > For architectures providing their own implementation of

> > array_index_mask_nospec in asm/barrier.h, attempting to use WARN_ONCE to

> > complain about out-of-range parameters using WARN_ON results in a mess

> > of mutually-dependent include files.

> >

> > Rather than unpick the dependencies, simply have the core code in nospec.h

> > perform the checking for us.

> >

> > Cc: Dan Williams <dan.j.williams@intel.com>

> > Cc: Ingo Molnar <mingo@redhat.com>

> > Signed-off-by: Will Deacon <will.deacon@arm.com>


[...]

> > @@ -61,7 +67,7 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,

> >  ({                                                                     \

> >         typeof(index) _i = (index);                                     \

> >         typeof(size) _s = (size);                                       \

> > -       unsigned long _mask = array_index_mask_nospec(_i, _s);          \

> > +       unsigned long _mask = array_index_mask_nospec_check(_i, _s);    \

> >                                                                         \

> >         BUILD_BUG_ON(sizeof(_i) > sizeof(long));                        \

> >         BUILD_BUG_ON(sizeof(_s) > sizeof(long));                        \

> 

> This change is commit 8fa80c503b484ddc ("nospec: Move array_index_nospec()

> parameter checking into separate macro") in v4.16-rc2, and triggers the

> following warning with gcc-4.1.2:

> 

>     net/wireless/nl80211.c: In function ‘parse_txq_params’:

>     net/wireless/nl80211.c:2099: warning: comparison is always false

> due to limited range of data type

> 

> Reverting the commit gets rid of the warning.


This is all getting ripped out, so stay tuned. The check is bogus, generates
crappy code and I did a poor job at macro-ising it. Apart from that, it's
great.

https://git.kernel.org/tip/1d91c1d2c80cb70e2e553845e278b87a960c04da

Will
diff mbox series

Patch

diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index b99bced39ac2..fbc98e2c8228 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -20,20 +20,6 @@  static inline unsigned long array_index_mask_nospec(unsigned long index,
 						    unsigned long size)
 {
 	/*
-	 * Warn developers about inappropriate array_index_nospec() usage.
-	 *
-	 * Even if the CPU speculates past the WARN_ONCE branch, the
-	 * sign bit of @index is taken into account when generating the
-	 * mask.
-	 *
-	 * This warning is compiled out when the compiler can infer that
-	 * @index and @size are less than LONG_MAX.
-	 */
-	if (WARN_ONCE(index > LONG_MAX || size > LONG_MAX,
-			"array_index_nospec() limited to range of [0, LONG_MAX]\n"))
-		return 0;
-
-	/*
 	 * Always calculate and emit the mask even if the compiler
 	 * thinks the mask is not needed. The compiler does not take
 	 * into account the value of @index under speculation.
@@ -44,6 +30,26 @@  static inline unsigned long array_index_mask_nospec(unsigned long index,
 #endif
 
 /*
+ * Warn developers about inappropriate array_index_nospec() usage.
+ *
+ * Even if the CPU speculates past the WARN_ONCE branch, the
+ * sign bit of @index is taken into account when generating the
+ * mask.
+ *
+ * This warning is compiled out when the compiler can infer that
+ * @index and @size are less than LONG_MAX.
+ */
+#define array_index_mask_nospec_check(index, size)				\
+({										\
+	if (WARN_ONCE(index > LONG_MAX || size > LONG_MAX,			\
+	    "array_index_nospec() limited to range of [0, LONG_MAX]\n"))	\
+		_mask = 0;							\
+	else									\
+		_mask = array_index_mask_nospec(index, size);			\
+	_mask;									\
+})
+
+/*
  * array_index_nospec - sanitize an array index after a bounds check
  *
  * For a code sequence like:
@@ -61,7 +67,7 @@  static inline unsigned long array_index_mask_nospec(unsigned long index,
 ({									\
 	typeof(index) _i = (index);					\
 	typeof(size) _s = (size);					\
-	unsigned long _mask = array_index_mask_nospec(_i, _s);		\
+	unsigned long _mask = array_index_mask_nospec_check(_i, _s);	\
 									\
 	BUILD_BUG_ON(sizeof(_i) > sizeof(long));			\
 	BUILD_BUG_ON(sizeof(_s) > sizeof(long));			\