[PATCH] mtd: cfi: convert inline functions to macros

[PATCH] mtd: cfi: convert inline functions to macros

[Arnd Bergmann]

The map_word_() functions, dating back to linux-2.6.8, try to perform
bitwise operations on a 'map_word' structure. This may have worked
with compilers that were current then (gcc-3.4 or earlier), but end
up being rather inefficient on any version I could try now (gcc-4.4 or
higher). Specifically we hit a problem analyzed in gcc PR81715 where we
fail to reuse the stack space for local variables.

This can be seen immediately in the stack consumption for
cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
can be up to 2200 bytes. Changing the inline functions into macros brings
this down to 1280 bytes.  Without KASAN, the same problem exists, but
the stack consumption is lower to start with, my patch shrinks it from
920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
structures for each call to one of these helpers.

With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
but nobody uses that yet, so we should still work around it in mainline
kernels and probably backport the workaround to stable kernels as well.
We had a couple of other functions that suffered from the same gcc bug,
and all of those had a simpler workaround involving dummy variables
in the inline function. Unfortunately that did not work here, the
macro hack was the best I could come up with.

It would also be helpful to have someone to a little performance testing
on the patch, to see how much it helps in terms of CPU utilitzation.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/mtd/map.h | 130 +++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 69 deletions(-)

diff --git a/include/linux/mtd/map.h b/include/linux/mtd/map.h
index 3aa56e3104bb..b5b43f94f311 100644
--- a/include/linux/mtd/map.h
+++ b/include/linux/mtd/map.h
@@ -270,75 +270,67 @@ void map_destroy(struct mtd_info *mtd);
 #define INVALIDATE_CACHED_RANGE(map, from, size) \
 	do { if (map->inval_cache) map->inval_cache(map, from, size); } while (0)
 
-
-static inline int map_word_equal(struct map_info *map, map_word val1, map_word val2)
-{
-	int i;
-
-	for (i = 0; i < map_words(map); i++) {
-		if (val1.x[i] != val2.x[i])
-			return 0;
-	}
-
-	return 1;
-}
-
-static inline map_word map_word_and(struct map_info *map, map_word val1, map_word val2)
-{
-	map_word r;
-	int i;
-
-	for (i = 0; i < map_words(map); i++)
-		r.x[i] = val1.x[i] & val2.x[i];
-
-	return r;
-}
-
-static inline map_word map_word_clr(struct map_info *map, map_word val1, map_word val2)
-{
-	map_word r;
-	int i;
-
-	for (i = 0; i < map_words(map); i++)
-		r.x[i] = val1.x[i] & ~val2.x[i];
-
-	return r;
-}
-
-static inline map_word map_word_or(struct map_info *map, map_word val1, map_word val2)
-{
-	map_word r;
-	int i;
-
-	for (i = 0; i < map_words(map); i++)
-		r.x[i] = val1.x[i] | val2.x[i];
-
-	return r;
-}
-
-static inline int map_word_andequal(struct map_info *map, map_word val1, map_word val2, map_word val3)
-{
-	int i;
-
-	for (i = 0; i < map_words(map); i++) {
-		if ((val1.x[i] & val2.x[i]) != val3.x[i])
-			return 0;
-	}
-
-	return 1;
-}
-
-static inline int map_word_bitsset(struct map_info *map, map_word val1, map_word val2)
-{
-	int i;
-
-	for (i = 0; i < map_words(map); i++) {
-		if (val1.x[i] & val2.x[i])
-			return 1;
-	}
-
-	return 0;
-}
+#define map_word_equal(map, val1, val2)					\
+({									\
+	int i, ret = 1;							\
+	for (i = 0; i < map_words(map); i++)				\
+		if ((val1).x[i] != (val2).x[i]) {			\
+			ret = 0;					\
+			break;						\
+		}							\
+	ret;								\
+})
+
+#define map_word_and(map, val1, val2)					\
+({									\
+	map_word r;							\
+	int i;								\
+	for (i = 0; i < map_words(map); i++)				\
+		r.x[i] = (val1).x[i] & (val2).x[i];			\
+	r;								\
+})
+
+#define map_word_clr(map, val1, val2)					\
+({									\
+	map_word r;							\
+	int i;								\
+	for (i = 0; i < map_words(map); i++)				\
+		r.x[i] = (val1).x[i] & ~(val2).x[i];			\
+	r;								\
+})
+
+#define map_word_or(map, val1, val2)					\
+({									\
+	map_word r;							\
+	int i;								\
+	for (i = 0; i < map_words(map); i++)				\
+		r.x[i] = (val1).x[i] | (val2).x[i];			\
+	r;								\
+})
+
+#define map_word_andequal(map, val1, val2, val3)			\
+({									\
+	int i, ret = 1;							\
+	for (i = 0; i < map_words(map); i++) {				\
+		if (((val1).x[i] & (val2).x[i]) != (val2).x[i]) {	\
+			ret = 0;					\
+			break;						\
+		}							\
+	}								\
+	ret;								\
+})
+
+#define map_word_bitsset(map, val1, val2)				\
+({									\
+	int i, ret = 0;							\
+	for (i = 0; i < map_words(map); i++) {				\
+		if ((val1).x[i] & (val2).x[i]) {			\
+			ret = 1;					\
+			break;						\
+		}							\
+	}								\
+	ret;								\
+})
 
 static inline map_word map_word_load(struct map_info *map, const void *ptr)
 {
-- 
2.9.0

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Marek Vasut]

On 10/11/2017 03:54 PM, Arnd Bergmann wrote:
> The map_word_() functions, dating back to linux-2.6.8, try to perform
> bitwise operations on a 'map_word' structure. This may have worked
> with compilers that were current then (gcc-3.4 or earlier), but end
> up being rather inefficient on any version I could try now (gcc-4.4 or
> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
> fail to reuse the stack space for local variables.
> 
> This can be seen immediately in the stack consumption for
> cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
> can be up to 2200 bytes. Changing the inline functions into macros brings
> this down to 1280 bytes.  Without KASAN, the same problem exists, but
> the stack consumption is lower to start with, my patch shrinks it from
> 920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
> 1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
> structures for each call to one of these helpers.
> 
> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
> but nobody uses that yet, so we should still work around it in mainline
> kernels and probably backport the workaround to stable kernels as well.
> We had a couple of other functions that suffered from the same gcc bug,
> and all of those had a simpler workaround involving dummy variables
> in the inline function. Unfortunately that did not work here, the
> macro hack was the best I could come up with.
> 
> It would also be helpful to have someone to a little performance testing
> on the patch, to see how much it helps in terms of CPU utilitzation.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Don't you lose type-checking with this conversion to macros ?

-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Boris Brezillon]

Hi Marek,

On Wed, 11 Oct 2017 23:34:33 +0200
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 10/11/2017 03:54 PM, Arnd Bergmann wrote:
> > The map_word_() functions, dating back to linux-2.6.8, try to perform
> > bitwise operations on a 'map_word' structure. This may have worked
> > with compilers that were current then (gcc-3.4 or earlier), but end
> > up being rather inefficient on any version I could try now (gcc-4.4 or
> > higher). Specifically we hit a problem analyzed in gcc PR81715 where we
> > fail to reuse the stack space for local variables.
> > 
> > This can be seen immediately in the stack consumption for
> > cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
> > can be up to 2200 bytes. Changing the inline functions into macros brings
> > this down to 1280 bytes.  Without KASAN, the same problem exists, but
> > the stack consumption is lower to start with, my patch shrinks it from
> > 920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
> > 1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
> > structures for each call to one of these helpers.
> > 
> > With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
> > but nobody uses that yet, so we should still work around it in mainline
> > kernels and probably backport the workaround to stable kernels as well.
> > We had a couple of other functions that suffered from the same gcc bug,
> > and all of those had a simpler workaround involving dummy variables
> > in the inline function. Unfortunately that did not work here, the
> > macro hack was the best I could come up with.
> > 
> > It would also be helpful to have someone to a little performance testing
> > on the patch, to see how much it helps in terms of CPU utilitzation.
> > 
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
> 
> Don't you lose type-checking with this conversion to macros ?
> 

Yes, we loose strict type checking, but if you look at the code, you'll
see that the macros do (valN).x[i], so, if valN is not a struct or
a union containing a field named x, the compiler will complain. That
should save us from devs passing random arguments to those macros.

Anyway, this code is not seeing a lot of changes lately, so I wouldn't
be so worried by the lack of strict type-checking implied by this
transition to macros. 

Regards,

Boris

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Marek Vasut]

On 12/17/2017 09:43 AM, Boris Brezillon wrote:
> Hi Marek,
> 
> On Wed, 11 Oct 2017 23:34:33 +0200
> Marek Vasut <marek.vasut@gmail.com> wrote:
> 
>> On 10/11/2017 03:54 PM, Arnd Bergmann wrote:
>>> The map_word_() functions, dating back to linux-2.6.8, try to perform
>>> bitwise operations on a 'map_word' structure. This may have worked
>>> with compilers that were current then (gcc-3.4 or earlier), but end
>>> up being rather inefficient on any version I could try now (gcc-4.4 or
>>> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
>>> fail to reuse the stack space for local variables.
>>>
>>> This can be seen immediately in the stack consumption for
>>> cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
>>> can be up to 2200 bytes. Changing the inline functions into macros brings
>>> this down to 1280 bytes.  Without KASAN, the same problem exists, but
>>> the stack consumption is lower to start with, my patch shrinks it from
>>> 920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
>>> 1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
>>> structures for each call to one of these helpers.
>>>
>>> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
>>> but nobody uses that yet, so we should still work around it in mainline
>>> kernels and probably backport the workaround to stable kernels as well.
>>> We had a couple of other functions that suffered from the same gcc bug,
>>> and all of those had a simpler workaround involving dummy variables
>>> in the inline function. Unfortunately that did not work here, the
>>> macro hack was the best I could come up with.
>>>
>>> It would also be helpful to have someone to a little performance testing
>>> on the patch, to see how much it helps in terms of CPU utilitzation.
>>>
>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
>>
>> Don't you lose type-checking with this conversion to macros ?
>>
> 
> Yes, we loose strict type checking, but if you look at the code, you'll
> see that the macros do (valN).x[i], so, if valN is not a struct or
> a union containing a field named x, the compiler will complain. That
> should save us from devs passing random arguments to those macros.

You could add some typeof() checking into those macros to make it even
more secure ... or, you could keep them as functions.

> Anyway, this code is not seeing a lot of changes lately, so I wouldn't
> be so worried by the lack of strict type-checking implied by this
> transition to macros. 

Well this transition makes the typechecking worse, not better, and I'm
not super happy about that.

-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Richard Weinberger]

Am Mittwoch, 11. Oktober 2017, 15:54:10 CET schrieb Arnd Bergmann:
> The map_word_() functions, dating back to linux-2.6.8, try to perform
> bitwise operations on a 'map_word' structure. This may have worked
> with compilers that were current then (gcc-3.4 or earlier), but end
> up being rather inefficient on any version I could try now (gcc-4.4 or
> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
> fail to reuse the stack space for local variables.
> 
> This can be seen immediately in the stack consumption for
> cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
> can be up to 2200 bytes. Changing the inline functions into macros brings
> this down to 1280 bytes.  Without KASAN, the same problem exists, but
> the stack consumption is lower to start with, my patch shrinks it from
> 920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
> 1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
> structures for each call to one of these helpers.
> 
> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
> but nobody uses that yet, so we should still work around it in mainline
> kernels and probably backport the workaround to stable kernels as well.
> We had a couple of other functions that suffered from the same gcc bug,
> and all of those had a simpler workaround involving dummy variables
> in the inline function. Unfortunately that did not work here, the
> macro hack was the best I could come up with.
> 
> It would also be helpful to have someone to a little performance testing
> on the patch, to see how much it helps in terms of CPU utilitzation.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Richard Weinberger <richard@nod.at>

Marek, I know you are not super happy with this patch but IMHO this is the 
solution with the least hassle.
While functions offer better type checking I think this functions are trivial 
enough to exist as macros too.
Also forcing users to upgrade/fix their compilers is only possible in a 
perfect world.

Thanks,
//richard

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Arnd Bergmann]

On Sun, Dec 17, 2017 at 9:34 PM, Richard Weinberger <richard@nod.at> wrote:
> Am Mittwoch, 11. Oktober 2017, 15:54:10 CET schrieb Arnd Bergmann:
>> The map_word_() functions, dating back to linux-2.6.8, try to perform
>> bitwise operations on a 'map_word' structure. This may have worked
>> with compilers that were current then (gcc-3.4 or earlier), but end
>> up being rather inefficient on any version I could try now (gcc-4.4 or
>> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
>> fail to reuse the stack space for local variables.
>>
>> This can be seen immediately in the stack consumption for
>> cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
>> can be up to 2200 bytes. Changing the inline functions into macros brings
>> this down to 1280 bytes.  Without KASAN, the same problem exists, but
>> the stack consumption is lower to start with, my patch shrinks it from
>> 920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
>> 1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
>> structures for each call to one of these helpers.
>>
>> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
>> but nobody uses that yet, so we should still work around it in mainline
>> kernels and probably backport the workaround to stable kernels as well.
>> We had a couple of other functions that suffered from the same gcc bug,
>> and all of those had a simpler workaround involving dummy variables
>> in the inline function. Unfortunately that did not work here, the
>> macro hack was the best I could come up with.
>>
>> It would also be helpful to have someone to a little performance testing
>> on the patch, to see how much it helps in terms of CPU utilitzation.
>>
>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Acked-by: Richard Weinberger <richard@nod.at>

Thanks!

> Marek, I know you are not super happy with this patch but IMHO this is the
> solution with the least hassle.
> While functions offer better type checking I think this functions are trivial
> enough to exist as macros too.
> Also forcing users to upgrade/fix their compilers is only possible in a
> perfect world.

Right. To clarify, this is a potential security issue, as it might be used to
construct a stack overflow to cause privilege escalation when combined
with some other vulnerabilities. I'd definitely want this backported to
stable kernels as a precaution, and I'm preparing a patch to warn
about this kind of problem again in 'allmodconfig' kernels that
currently disable the warning on arm64 and x86.

       Arnd

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Marek Vasut]

On 12/18/2017 10:16 AM, Arnd Bergmann wrote:
> On Sun, Dec 17, 2017 at 9:34 PM, Richard Weinberger <richard@nod.at> wrote:
>> Am Mittwoch, 11. Oktober 2017, 15:54:10 CET schrieb Arnd Bergmann:
>>> The map_word_() functions, dating back to linux-2.6.8, try to perform
>>> bitwise operations on a 'map_word' structure. This may have worked
>>> with compilers that were current then (gcc-3.4 or earlier), but end
>>> up being rather inefficient on any version I could try now (gcc-4.4 or
>>> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
>>> fail to reuse the stack space for local variables.
>>>
>>> This can be seen immediately in the stack consumption for
>>> cfi_staa_erase_varsize() and other functions that (with CONFIG_KASAN)
>>> can be up to 2200 bytes. Changing the inline functions into macros brings
>>> this down to 1280 bytes.  Without KASAN, the same problem exists, but
>>> the stack consumption is lower to start with, my patch shrinks it from
>>> 920 to 496 bytes on with arm-linux-gnueabi-gcc-5.4, and saves around
>>> 1KB in .text size for cfi_cmdset_0020.c, as it avoids copying map_word
>>> structures for each call to one of these helpers.
>>>
>>> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
>>> but nobody uses that yet, so we should still work around it in mainline
>>> kernels and probably backport the workaround to stable kernels as well.
>>> We had a couple of other functions that suffered from the same gcc bug,
>>> and all of those had a simpler workaround involving dummy variables
>>> in the inline function. Unfortunately that did not work here, the
>>> macro hack was the best I could come up with.
>>>
>>> It would also be helpful to have someone to a little performance testing
>>> on the patch, to see how much it helps in terms of CPU utilitzation.
>>>
>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> Acked-by: Richard Weinberger <richard@nod.at>
> 
> Thanks!
> 
>> Marek, I know you are not super happy with this patch but IMHO this is the
>> solution with the least hassle.
>> While functions offer better type checking I think this functions are trivial
>> enough to exist as macros too.
>> Also forcing users to upgrade/fix their compilers is only possible in a
>> perfect world.
> 
> Right. To clarify, this is a potential security issue, as it might be used to
> construct a stack overflow to cause privilege escalation when combined
> with some other vulnerabilities. I'd definitely want this backported to
> stable kernels as a precaution, and I'm preparing a patch to warn
> about this kind of problem again in 'allmodconfig' kernels that
> currently disable the warning on arm64 and x86.

Wouldn't it make more sense to fix the compiler instead ?
This still feels like we're fixing a bug at the wrong place ...

-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Arnd Bergmann]

On Mon, Dec 18, 2017 at 10:18 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 12/18/2017 10:16 AM, Arnd Bergmann wrote:
>> On Sun, Dec 17, 2017 at 9:34 PM, Richard Weinberger <richard@nod.at> wrote:
>>> Am Mittwoch, 11. Oktober 2017, 15:54:10 CET schrieb Arnd Bergmann:
>>>> The map_word_() functions, dating back to linux-2.6.8, try to perform
>>>> bitwise operations on a 'map_word' structure. This may have worked
>>>> with compilers that were current then (gcc-3.4 or earlier), but end
>>>> up being rather inefficient on any version I could try now (gcc-4.4 or
>>>> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
>>>> fail to reuse the stack space for local variables.
...
>>>>
>>>> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
>>>> but nobody uses that yet, so we should still work around it in mainline
>>>> kernels and probably backport the workaround to stable kernels as well.
>>>> We had a couple of other functions that suffered from the same gcc bug,
>>>> and all of those had a simpler workaround involving dummy variables
>>>> in the inline function. Unfortunately that did not work here, the
>>>> macro hack was the best I could come up with.
>>>>
>>>> It would also be helpful to have someone to a little performance testing
>>>> on the patch, to see how much it helps in terms of CPU utilitzation.
>>>>
>>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>>
>>> Acked-by: Richard Weinberger <richard@nod.at>
>>
>> Thanks!
>>
>>> Marek, I know you are not super happy with this patch but IMHO this is the
>>> solution with the least hassle.
>>> While functions offer better type checking I think this functions are trivial
>>> enough to exist as macros too.
>>> Also forcing users to upgrade/fix their compilers is only possible in a
>>> perfect world.
>>
>> Right. To clarify, this is a potential security issue, as it might be used to
>> construct a stack overflow to cause privilege escalation when combined
>> with some other vulnerabilities. I'd definitely want this backported to
>> stable kernels as a precaution, and I'm preparing a patch to warn
>> about this kind of problem again in 'allmodconfig' kernels that
>> currently disable the warning on arm64 and x86.
>
> Wouldn't it make more sense to fix the compiler instead ?
> This still feels like we're fixing a bug at the wrong place ...

See above: the compiler is fixed in the gcc-8.x release branch,
which won't be out until next spring. People use all kinds of versions
as old as gcc-4.3, even if the fix was backported to older compilers
(which it is not), most users never rebuild their toolchains to get the
latest bugfix releases.

For instance, the Android SDK comes with prebuilt binaries of
a gcc-4.9-prerelease version that has many known bugs that
were fixed either by the time the official 4.9 release happened,
or in one of the bugfix releases following it.

      Arnd

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Marek Vasut]

On 12/18/2017 11:29 AM, Arnd Bergmann wrote:
> On Mon, Dec 18, 2017 at 10:18 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 12/18/2017 10:16 AM, Arnd Bergmann wrote:
>>> On Sun, Dec 17, 2017 at 9:34 PM, Richard Weinberger <richard@nod.at> wrote:
>>>> Am Mittwoch, 11. Oktober 2017, 15:54:10 CET schrieb Arnd Bergmann:
>>>>> The map_word_() functions, dating back to linux-2.6.8, try to perform
>>>>> bitwise operations on a 'map_word' structure. This may have worked
>>>>> with compilers that were current then (gcc-3.4 or earlier), but end
>>>>> up being rather inefficient on any version I could try now (gcc-4.4 or
>>>>> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
>>>>> fail to reuse the stack space for local variables.
> ...
>>>>>
>>>>> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
>>>>> but nobody uses that yet, so we should still work around it in mainline
>>>>> kernels and probably backport the workaround to stable kernels as well.
>>>>> We had a couple of other functions that suffered from the same gcc bug,
>>>>> and all of those had a simpler workaround involving dummy variables
>>>>> in the inline function. Unfortunately that did not work here, the
>>>>> macro hack was the best I could come up with.
>>>>>
>>>>> It would also be helpful to have someone to a little performance testing
>>>>> on the patch, to see how much it helps in terms of CPU utilitzation.
>>>>>
>>>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>>>
>>>> Acked-by: Richard Weinberger <richard@nod.at>
>>>
>>> Thanks!
>>>
>>>> Marek, I know you are not super happy with this patch but IMHO this is the
>>>> solution with the least hassle.
>>>> While functions offer better type checking I think this functions are trivial
>>>> enough to exist as macros too.
>>>> Also forcing users to upgrade/fix their compilers is only possible in a
>>>> perfect world.
>>>
>>> Right. To clarify, this is a potential security issue, as it might be used to
>>> construct a stack overflow to cause privilege escalation when combined
>>> with some other vulnerabilities. I'd definitely want this backported to
>>> stable kernels as a precaution, and I'm preparing a patch to warn
>>> about this kind of problem again in 'allmodconfig' kernels that
>>> currently disable the warning on arm64 and x86.
>>
>> Wouldn't it make more sense to fix the compiler instead ?
>> This still feels like we're fixing a bug at the wrong place ...
> 
> See above: the compiler is fixed in the gcc-8.x release branch,
> which won't be out until next spring. People use all kinds of versions
> as old as gcc-4.3, even if the fix was backported to older compilers
> (which it is not), most users never rebuild their toolchains to get the
> latest bugfix releases.
> 
> For instance, the Android SDK comes with prebuilt binaries of
> a gcc-4.9-prerelease version that has many known bugs that
> were fixed either by the time the official 4.9 release happened,
> or in one of the bugfix releases following it.

But doesn't this mean we're taking the OpenSSL path (which didn't work
out well for them IIRC) ?

I don't have a better solution for this though ...

-- 
Best regards,
Marek Vasut

Re: [PATCH] mtd: cfi: convert inline functions to macros

[Boris Brezillon]

Hi Marek,

On Mon, 18 Dec 2017 11:38:20 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 12/18/2017 11:29 AM, Arnd Bergmann wrote:
> > On Mon, Dec 18, 2017 at 10:18 AM, Marek Vasut <marek.vasut@gmail.com> wrote:  
> >> On 12/18/2017 10:16 AM, Arnd Bergmann wrote:  
> >>> On Sun, Dec 17, 2017 at 9:34 PM, Richard Weinberger <richard@nod.at> wrote:  
> >>>> Am Mittwoch, 11. Oktober 2017, 15:54:10 CET schrieb Arnd Bergmann:  
> >>>>> The map_word_() functions, dating back to linux-2.6.8, try to perform
> >>>>> bitwise operations on a 'map_word' structure. This may have worked
> >>>>> with compilers that were current then (gcc-3.4 or earlier), but end
> >>>>> up being rather inefficient on any version I could try now (gcc-4.4 or
> >>>>> higher). Specifically we hit a problem analyzed in gcc PR81715 where we
> >>>>> fail to reuse the stack space for local variables.  
> > ...  
> >>>>>
> >>>>> With the latest gcc-8 snapshot, the problem is fixed in upstream gcc,
> >>>>> but nobody uses that yet, so we should still work around it in mainline
> >>>>> kernels and probably backport the workaround to stable kernels as well.
> >>>>> We had a couple of other functions that suffered from the same gcc bug,
> >>>>> and all of those had a simpler workaround involving dummy variables
> >>>>> in the inline function. Unfortunately that did not work here, the
> >>>>> macro hack was the best I could come up with.
> >>>>>
> >>>>> It would also be helpful to have someone to a little performance testing
> >>>>> on the patch, to see how much it helps in terms of CPU utilitzation.
> >>>>>
> >>>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715
> >>>>> Cc: stable@vger.kernel.org
> >>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
> >>>>
> >>>> Acked-by: Richard Weinberger <richard@nod.at>  
> >>>
> >>> Thanks!
> >>>  
> >>>> Marek, I know you are not super happy with this patch but IMHO this is the
> >>>> solution with the least hassle.
> >>>> While functions offer better type checking I think this functions are trivial
> >>>> enough to exist as macros too.
> >>>> Also forcing users to upgrade/fix their compilers is only possible in a
> >>>> perfect world.  
> >>>
> >>> Right. To clarify, this is a potential security issue, as it might be used to
> >>> construct a stack overflow to cause privilege escalation when combined
> >>> with some other vulnerabilities. I'd definitely want this backported to
> >>> stable kernels as a precaution, and I'm preparing a patch to warn
> >>> about this kind of problem again in 'allmodconfig' kernels that
> >>> currently disable the warning on arm64 and x86.  
> >>
> >> Wouldn't it make more sense to fix the compiler instead ?
> >> This still feels like we're fixing a bug at the wrong place ...  
> > 
> > See above: the compiler is fixed in the gcc-8.x release branch,
> > which won't be out until next spring. People use all kinds of versions
> > as old as gcc-4.3, even if the fix was backported to older compilers
> > (which it is not), most users never rebuild their toolchains to get the
> > latest bugfix releases.
> > 
> > For instance, the Android SDK comes with prebuilt binaries of
> > a gcc-4.9-prerelease version that has many known bugs that
> > were fixed either by the time the official 4.9 release happened,
> > or in one of the bugfix releases following it.  
> 
> But doesn't this mean we're taking the OpenSSL path (which didn't work
> out well for them IIRC) ?
> 
> I don't have a better solution for this though ...
> 

I know you don't like this solution, but until you propose a real
alternative I decided to apply it. If you come up with something
better, I'll consider reverting this patch and applying yours.

Regards,

Boris