[PATCH 1/3] media: saa7146: avoid high stack usage with clang

[PATCH 1/3] media: saa7146: avoid high stack usage with clang

[Arnd Bergmann]

Two saa7146/hexium files contain a construct that causes a warning
when built with clang:

drivers/media/pci/saa7146/hexium_orion.c:210:12: error: stack frame size of 2272 bytes in function 'hexium_probe'
      [-Werror,-Wframe-larger-than=]
static int hexium_probe(struct saa7146_dev *dev)
           ^
drivers/media/pci/saa7146/hexium_gemini.c:257:12: error: stack frame size of 2304 bytes in function 'hexium_attach'
      [-Werror,-Wframe-larger-than=]
static int hexium_attach(struct saa7146_dev *dev, struct saa7146_pci_extension_data *info)
           ^

This one happens regardless of KASAN, and the problem is that a
constructor to initalize a dynamically allocated structure leads
to a copy of that structure on the stack, whereas gcc initializes
it in place.

Link: https://bugs.llvm.org/show_bug.cgi?id=40776
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/pci/saa7146/hexium_gemini.c | 4 +---
 drivers/media/pci/saa7146/hexium_orion.c  | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/pci/saa7146/hexium_gemini.c b/drivers/media/pci/saa7146/hexium_gemini.c
index 5817d9cde4d0..f7ce0e1770bf 100644
--- a/drivers/media/pci/saa7146/hexium_gemini.c
+++ b/drivers/media/pci/saa7146/hexium_gemini.c
@@ -270,9 +270,7 @@ static int hexium_attach(struct saa7146_dev *dev, struct saa7146_pci_extension_d
 	/* enable i2c-port pins */
 	saa7146_write(dev, MC1, (MASK_08 | MASK_24 | MASK_10 | MASK_26));
 
-	hexium->i2c_adapter = (struct i2c_adapter) {
-		.name = "hexium gemini",
-	};
+	strscpy(hexium->i2c_adapter.name, "hexium gemini", sizeof(hexium->i2c_adapter.name));
 	saa7146_i2c_adapter_prepare(dev, &hexium->i2c_adapter, SAA7146_I2C_BUS_BIT_RATE_480);
 	if (i2c_add_adapter(&hexium->i2c_adapter) < 0) {
 		DEB_S("cannot register i2c-device. skipping.\n");
diff --git a/drivers/media/pci/saa7146/hexium_orion.c b/drivers/media/pci/saa7146/hexium_orion.c
index 0a05176c18ab..b9f4a09c744d 100644
--- a/drivers/media/pci/saa7146/hexium_orion.c
+++ b/drivers/media/pci/saa7146/hexium_orion.c
@@ -231,9 +231,7 @@ static int hexium_probe(struct saa7146_dev *dev)
 	saa7146_write(dev, DD1_STREAM_B, 0x00000000);
 	saa7146_write(dev, MC2, (MASK_09 | MASK_25 | MASK_10 | MASK_26));
 
-	hexium->i2c_adapter = (struct i2c_adapter) {
-		.name = "hexium orion",
-	};
+	strscpy(hexium->i2c_adapter.name, "hexium orion", sizeof(hexium->i2c_adapter.name));
 	saa7146_i2c_adapter_prepare(dev, &hexium->i2c_adapter, SAA7146_I2C_BUS_BIT_RATE_480);
 	if (i2c_add_adapter(&hexium->i2c_adapter) < 0) {
 		DEB_S("cannot register i2c-device. skipping.\n");
-- 
2.20.0

[PATCH 2/3] media: vicodec: avoic clang frame size warning

[Arnd Bergmann]

Clang-9 makes some different inlining decisions compared to gcc, which
leads to a warning about a possible stack overflow problem when building
with CONFIG_KASAN, including when setting asan-stack=0, which avoids
most other frame overflow warnings:

drivers/media/platform/vicodec/codec-fwht.c:673:12: error: stack frame size of 2224 bytes in function 'encode_plane'

Manually adding noinline_for_stack annotations in those functions
called by encode_plane() or decode_plane() that require a significant
amount of kernel stack makes this impossible to happen with any
compiler.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/platform/vicodec/codec-fwht.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index d1d6085da9f1..135d56bcc2c5 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -47,7 +47,7 @@ static const uint8_t zigzag[64] = {
 };
 
 
-static int rlc(const s16 *in, __be16 *output, int blocktype)
+static int noinline_for_stack rlc(const s16 *in, __be16 *output, int blocktype)
 {
 	s16 block[8 * 8];
 	s16 *wp = block;
@@ -106,8 +106,8 @@ static int rlc(const s16 *in, __be16 *output, int blocktype)
  * This function will worst-case increase rlc_in by 65*2 bytes:
  * one s16 value for the header and 8 * 8 coefficients of type s16.
  */
-static u16 derlc(const __be16 **rlc_in, s16 *dwht_out,
-		 const __be16 *end_of_input)
+static noinline_for_stack u16
+derlc(const __be16 **rlc_in, s16 *dwht_out, const __be16 *end_of_input)
 {
 	/* header */
 	const __be16 *input = *rlc_in;
@@ -373,7 +373,8 @@ static void fwht(const u8 *block, s16 *output_block, unsigned int stride,
  * Furthermore values can be negative... This is just a version that
  * works with 16 signed data
  */
-static void fwht16(const s16 *block, s16 *output_block, int stride, int intra)
+static void noinline_for_stack
+fwht16(const s16 *block, s16 *output_block, int stride, int intra)
 {
 	/* we'll need more than 8 bits for the transformed coefficients */
 	s32 workspace1[8], workspace2[8];
@@ -456,7 +457,8 @@ static void fwht16(const s16 *block, s16 *output_block, int stride, int intra)
 	}
 }
 
-static void ifwht(const s16 *block, s16 *output_block, int intra)
+static noinline_for_stack void
+ifwht(const s16 *block, s16 *output_block, int intra)
 {
 	/*
 	 * we'll need more than 8 bits for the transformed coefficients
@@ -604,9 +606,9 @@ static int var_inter(const s16 *old, const s16 *new)
 	return ret;
 }
 
-static int decide_blocktype(const u8 *cur, const u8 *reference,
-			    s16 *deltablock, unsigned int stride,
-			    unsigned int input_step)
+static noinline_for_stack int
+decide_blocktype(const u8 *cur, const u8 *reference, s16 *deltablock,
+		 unsigned int stride, unsigned int input_step)
 {
 	s16 tmp[64];
 	s16 old[64];
-- 
2.20.0

[PATCH 3/3] media: go7007: avoid clang frame overflow warning with KASAN

[Arnd Bergmann]

clang-8 warns about one function here when KASAN is enabled, even
without the 'asan-stack' option:

drivers/media/usb/go7007/go7007-fw.c:1551:5: warning: stack frame size of 2656 bytes in function

I have reported this issue in the llvm bugzilla, but to make
it work with the clang-8 release, a small annotation is still
needed.

Link: https://bugs.llvm.org/show_bug.cgi?id=38809
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/usb/go7007/go7007-fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/go7007/go7007-fw.c b/drivers/media/usb/go7007/go7007-fw.c
index 24f5b615dc7a..c7201e93c331 100644
--- a/drivers/media/usb/go7007/go7007-fw.c
+++ b/drivers/media/usb/go7007/go7007-fw.c
@@ -1499,7 +1499,7 @@ static int modet_to_package(struct go7007 *go, __le16 *code, int space)
 	return cnt;
 }
 
-static int do_special(struct go7007 *go, u16 type, __le16 *code, int space,
+static noinline_for_stack int do_special(struct go7007 *go, u16 type, __le16 *code, int space,
 			int *framelen)
 {
 	switch (type) {
-- 
2.20.0

Re: [PATCH 1/3] media: saa7146: avoid high stack usage with clang

[Nick Desaulniers]

On Tue, Feb 19, 2019 at 9:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Two saa7146/hexium files contain a construct that causes a warning
> when built with clang:
>
> drivers/media/pci/saa7146/hexium_orion.c:210:12: error: stack frame size of 2272 bytes in function 'hexium_probe'
>       [-Werror,-Wframe-larger-than=]
> static int hexium_probe(struct saa7146_dev *dev)
>            ^
> drivers/media/pci/saa7146/hexium_gemini.c:257:12: error: stack frame size of 2304 bytes in function 'hexium_attach'
>       [-Werror,-Wframe-larger-than=]
> static int hexium_attach(struct saa7146_dev *dev, struct saa7146_pci_extension_data *info)
>            ^
>
> This one happens regardless of KASAN, and the problem is that a
> constructor to initalize a dynamically allocated structure leads
> to a copy of that structure on the stack, whereas gcc initializes
> it in place.
>
> Link: https://bugs.llvm.org/show_bug.cgi?id=40776

oof, great bug report by the way!  Thanks for the fix.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/pci/saa7146/hexium_gemini.c | 4 +---
>  drivers/media/pci/saa7146/hexium_orion.c  | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/pci/saa7146/hexium_gemini.c b/drivers/media/pci/saa7146/hexium_gemini.c
> index 5817d9cde4d0..f7ce0e1770bf 100644
> --- a/drivers/media/pci/saa7146/hexium_gemini.c
> +++ b/drivers/media/pci/saa7146/hexium_gemini.c
> @@ -270,9 +270,7 @@ static int hexium_attach(struct saa7146_dev *dev, struct saa7146_pci_extension_d
>         /* enable i2c-port pins */
>         saa7146_write(dev, MC1, (MASK_08 | MASK_24 | MASK_10 | MASK_26));
>
> -       hexium->i2c_adapter = (struct i2c_adapter) {
> -               .name = "hexium gemini",
> -       };
> +       strscpy(hexium->i2c_adapter.name, "hexium gemini", sizeof(hexium->i2c_adapter.name));
>         saa7146_i2c_adapter_prepare(dev, &hexium->i2c_adapter, SAA7146_I2C_BUS_BIT_RATE_480);
>         if (i2c_add_adapter(&hexium->i2c_adapter) < 0) {
>                 DEB_S("cannot register i2c-device. skipping.\n");
> diff --git a/drivers/media/pci/saa7146/hexium_orion.c b/drivers/media/pci/saa7146/hexium_orion.c
> index 0a05176c18ab..b9f4a09c744d 100644
> --- a/drivers/media/pci/saa7146/hexium_orion.c
> +++ b/drivers/media/pci/saa7146/hexium_orion.c
> @@ -231,9 +231,7 @@ static int hexium_probe(struct saa7146_dev *dev)
>         saa7146_write(dev, DD1_STREAM_B, 0x00000000);
>         saa7146_write(dev, MC2, (MASK_09 | MASK_25 | MASK_10 | MASK_26));
>
> -       hexium->i2c_adapter = (struct i2c_adapter) {
> -               .name = "hexium orion",
> -       };
> +       strscpy(hexium->i2c_adapter.name, "hexium orion", sizeof(hexium->i2c_adapter.name));

Note that "sparse" designated initialization zero initializes unnnamed members:
https://godbolt.org/z/LkSpJp
This transform you've done is safe because hexium was zero initialized
via kzalloc, and struct hexium contains a struct i2c_adapter (as
opposed to  a pointer to a struct i2c_adapter).  The same is true for
both translation units you've touched. LGTM

>         saa7146_i2c_adapter_prepare(dev, &hexium->i2c_adapter, SAA7146_I2C_BUS_BIT_RATE_480);
>         if (i2c_add_adapter(&hexium->i2c_adapter) < 0) {
>                 DEB_S("cannot register i2c-device. skipping.\n");
> --
> 2.20.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 2/3] media: vicodec: avoic clang frame size warning

[Nick Desaulniers]

On Tue, Feb 19, 2019 at 9:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Clang-9 makes some different inlining decisions compared to gcc, which
> leads to a warning about a possible stack overflow problem when building
> with CONFIG_KASAN, including when setting asan-stack=0, which avoids
> most other frame overflow warnings:
>
> drivers/media/platform/vicodec/codec-fwht.c:673:12: error: stack frame size of 2224 bytes in function 'encode_plane'
>
> Manually adding noinline_for_stack annotations in those functions

Thanks for the fix! In general, for -Wstack-frame-larger-than=
warnings, is it possible that these sets of stack frames are already
too large if entered?  Sure, inlining was a little aggressive, causing
more stack space use than maybe otherwise necessary at runtime, but
isn't it also possible that "no inlining" a stack frame can still be a
problem should the stack frame be entered?  Doesn't the kernel have a
way of estimating the stack depth for any given frame?  I guess I was
always curious if the best fix for these kind of warnings was to
non-stack allocate (kmalloc) certain locally allocated structs, or
no-inline the function.  Surely there's cases where no-inlining is
safe, but I was curious if it's still maybe dangerous to enter the
problematic child most stack frame?

> called by encode_plane() or decode_plane() that require a significant
> amount of kernel stack makes this impossible to happen with any
> compiler.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/platform/vicodec/codec-fwht.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
> index d1d6085da9f1..135d56bcc2c5 100644
> --- a/drivers/media/platform/vicodec/codec-fwht.c
> +++ b/drivers/media/platform/vicodec/codec-fwht.c
> @@ -47,7 +47,7 @@ static const uint8_t zigzag[64] = {
>  };
>
>
> -static int rlc(const s16 *in, __be16 *output, int blocktype)
> +static int noinline_for_stack rlc(const s16 *in, __be16 *output, int blocktype)
>  {
>         s16 block[8 * 8];
>         s16 *wp = block;
> @@ -106,8 +106,8 @@ static int rlc(const s16 *in, __be16 *output, int blocktype)
>   * This function will worst-case increase rlc_in by 65*2 bytes:
>   * one s16 value for the header and 8 * 8 coefficients of type s16.
>   */
> -static u16 derlc(const __be16 **rlc_in, s16 *dwht_out,
> -                const __be16 *end_of_input)
> +static noinline_for_stack u16
> +derlc(const __be16 **rlc_in, s16 *dwht_out, const __be16 *end_of_input)
>  {
>         /* header */
>         const __be16 *input = *rlc_in;
> @@ -373,7 +373,8 @@ static void fwht(const u8 *block, s16 *output_block, unsigned int stride,
>   * Furthermore values can be negative... This is just a version that
>   * works with 16 signed data
>   */
> -static void fwht16(const s16 *block, s16 *output_block, int stride, int intra)
> +static void noinline_for_stack
> +fwht16(const s16 *block, s16 *output_block, int stride, int intra)
>  {
>         /* we'll need more than 8 bits for the transformed coefficients */
>         s32 workspace1[8], workspace2[8];
> @@ -456,7 +457,8 @@ static void fwht16(const s16 *block, s16 *output_block, int stride, int intra)
>         }
>  }
>
> -static void ifwht(const s16 *block, s16 *output_block, int intra)
> +static noinline_for_stack void
> +ifwht(const s16 *block, s16 *output_block, int intra)
>  {
>         /*
>          * we'll need more than 8 bits for the transformed coefficients
> @@ -604,9 +606,9 @@ static int var_inter(const s16 *old, const s16 *new)
>         return ret;
>  }
>
> -static int decide_blocktype(const u8 *cur, const u8 *reference,
> -                           s16 *deltablock, unsigned int stride,
> -                           unsigned int input_step)
> +static noinline_for_stack int
> +decide_blocktype(const u8 *cur, const u8 *reference, s16 *deltablock,
> +                unsigned int stride, unsigned int input_step)
>  {
>         s16 tmp[64];
>         s16 old[64];
> --
> 2.20.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 2/3] media: vicodec: avoic clang frame size warning

[Arnd Bergmann]

On Tue, Feb 19, 2019 at 8:02 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> On Tue, Feb 19, 2019 at 9:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > Clang-9 makes some different inlining decisions compared to gcc, which
> > leads to a warning about a possible stack overflow problem when building
> > with CONFIG_KASAN, including when setting asan-stack=0, which avoids
> > most other frame overflow warnings:
> >
> > drivers/media/platform/vicodec/codec-fwht.c:673:12: error: stack frame size of 2224 bytes in function 'encode_plane'
> >
> > Manually adding noinline_for_stack annotations in those functions
>
> Thanks for the fix! In general, for -Wstack-frame-larger-than=
> warnings, is it possible that these sets of stack frames are already
> too large if entered?  Sure, inlining was a little aggressive, causing
> more stack space use than maybe otherwise necessary at runtime, but
> isn't it also possible that "no inlining" a stack frame can still be a
> problem should the stack frame be entered?  Doesn't the kernel have a
> way of estimating the stack depth for any given frame?  I guess I was
> always curious if the best fix for these kind of warnings was to
> non-stack allocate (kmalloc) certain locally allocated structs, or
> no-inline the function.  Surely there's cases where no-inlining is
> safe, but I was curious if it's still maybe dangerous to enter the
> problematic child most stack frame?

What I think is happening here is that llvm fails to combine the
stack allocations for the inlined functions in certain conditions,
while gcc can reuse it here. We had similar issues in gcc
a few years ago, and they got fixed there, but I have not looked
at this one in more detail. My guess is that it's related to
the bug I mentioned in patch 3.

      Arnd

Re: [PATCH 2/3] media: vicodec: avoic clang frame size warning

[Hans Verkuil]

On 2/19/19 6:01 PM, Arnd Bergmann wrote:
> Clang-9 makes some different inlining decisions compared to gcc, which
> leads to a warning about a possible stack overflow problem when building
> with CONFIG_KASAN, including when setting asan-stack=0, which avoids
> most other frame overflow warnings:
> 
> drivers/media/platform/vicodec/codec-fwht.c:673:12: error: stack frame size of 2224 bytes in function 'encode_plane'
> 
> Manually adding noinline_for_stack annotations in those functions
> called by encode_plane() or decode_plane() that require a significant
> amount of kernel stack makes this impossible to happen with any
> compiler.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/platform/vicodec/codec-fwht.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
> index d1d6085da9f1..135d56bcc2c5 100644
> --- a/drivers/media/platform/vicodec/codec-fwht.c
> +++ b/drivers/media/platform/vicodec/codec-fwht.c
> @@ -47,7 +47,7 @@ static const uint8_t zigzag[64] = {
>  };
>  
>  
> -static int rlc(const s16 *in, __be16 *output, int blocktype)
> +static int noinline_for_stack rlc(const s16 *in, __be16 *output, int blocktype)
>  {
>  	s16 block[8 * 8];
>  	s16 *wp = block;
> @@ -106,8 +106,8 @@ static int rlc(const s16 *in, __be16 *output, int blocktype)
>   * This function will worst-case increase rlc_in by 65*2 bytes:
>   * one s16 value for the header and 8 * 8 coefficients of type s16.
>   */
> -static u16 derlc(const __be16 **rlc_in, s16 *dwht_out,
> -		 const __be16 *end_of_input)
> +static noinline_for_stack u16
> +derlc(const __be16 **rlc_in, s16 *dwht_out, const __be16 *end_of_input)
>  {
>  	/* header */
>  	const __be16 *input = *rlc_in;
> @@ -373,7 +373,8 @@ static void fwht(const u8 *block, s16 *output_block, unsigned int stride,
>   * Furthermore values can be negative... This is just a version that
>   * works with 16 signed data
>   */
> -static void fwht16(const s16 *block, s16 *output_block, int stride, int intra)
> +static void noinline_for_stack
> +fwht16(const s16 *block, s16 *output_block, int stride, int intra)
>  {
>  	/* we'll need more than 8 bits for the transformed coefficients */
>  	s32 workspace1[8], workspace2[8];
> @@ -456,7 +457,8 @@ static void fwht16(const s16 *block, s16 *output_block, int stride, int intra)
>  	}
>  }
>  
> -static void ifwht(const s16 *block, s16 *output_block, int intra)
> +static noinline_for_stack void
> +ifwht(const s16 *block, s16 *output_block, int intra)
>  {

Please add it for fwht as well. It makes no sense to have it for fwht16, ifwht
but not the fwht function.

Got to say this is all very magic...

I think it would be good to perhaps have a comment at the start of the source
that explains why noinline_for_stack is added to selected functions.

Patches 1 & 3 are fine, BTW.

Regards,

	Hans

>  	/*
>  	 * we'll need more than 8 bits for the transformed coefficients
> @@ -604,9 +606,9 @@ static int var_inter(const s16 *old, const s16 *new)
>  	return ret;
>  }
>  
> -static int decide_blocktype(const u8 *cur, const u8 *reference,
> -			    s16 *deltablock, unsigned int stride,
> -			    unsigned int input_step)
> +static noinline_for_stack int
> +decide_blocktype(const u8 *cur, const u8 *reference, s16 *deltablock,
> +		 unsigned int stride, unsigned int input_step)
>  {
>  	s16 tmp[64];
>  	s16 old[64];
>