next-20241216: drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto statement to its label

next-20241216: drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto statement to its label

[Naresh Kamboju]

The arm and arm64 builds failed on Linux next-20241216 due to following
build warnings / errors with clang-19 and clang-nightly toolchain.
Whereas the gcc-13 builds pass.

arm, arm64:
  * build/clang-19-defconfig
  * build/clang-nightly-defconfig

First seen on Linux next-20241216.
  Good: next-20241216
  Bad:  next-20241213

Build log:
-----------
fs/netfs/read_retry.c:235:20: warning: variable 'subreq' is
uninitialized when used here [-Wuninitialized]
  235 |         if (list_is_last(&subreq->rreq_link, &stream->subrequests))
      |                           ^~~~~~
fs/netfs/read_retry.c:28:36: note: initialize the variable 'subreq' to
silence this warning
   28 |         struct netfs_io_subrequest *subreq;
      |                                           ^
      |                                            = NULL
1 warning generated.
drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto
statement to its label
  365 |                 goto err_free_ahash;
      |                 ^
drivers/crypto/qce/sha.c:373:6: note: jump bypasses initialization of
variable with __attribute__((cleanup))
  373 |         u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
      |             ^
1 error generated.

Links:
-------
 - https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20241216/testrun/26350650/suite/build/test/clang-19-defconfig/log
 - https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20241216/testrun/26350650/suite/build/test/clang-19-defconfig/history/
 - https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20241216/testrun/26351207/suite/build/test/clang-19-defconfig/details/
 - https://storage.tuxsuite.com/public/linaro/lkft/builds/2qHsa6j5c1HY3GRZFGrxu2ELo3f/

Steps to reproduce:
------------
# tuxmake --runtime podman --target-arch arm64 --toolchain clang-19
--kconfig defconfig LLVM=1 LLVM_IAS=1

metadata:
----
  git describe: next-20241216
  git repo: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
  git sha: e25c8d66f6786300b680866c0e0139981273feba
  kernel config:
https://storage.tuxsuite.com/public/linaro/lkft/builds/2qHsa8wFmfrlj0VdDqAG408o3l2/config
  build url: https://storage.tuxsuite.com/public/linaro/lkft/builds/2qHsa8wFmfrlj0VdDqAG408o3l2/
  toolchain: clang-19
  config: clang-19-defconfig
  arch: arm64, arm

 --
Linaro LKFT
https://lkft.linaro.org

Re: next-20241216: drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto statement to its label

[Nathan Chancellor]

Hi Naresh,

Thanks for the report.

+ Bartosz as author of ce8fd0500b74

On Mon, Dec 16, 2024 at 10:04:05PM +0530, Naresh Kamboju wrote:
> The arm and arm64 builds failed on Linux next-20241216 due to following
> build warnings / errors with clang-19 and clang-nightly toolchain.
> Whereas the gcc-13 builds pass.
> 
> arm, arm64:
>   * build/clang-19-defconfig
>   * build/clang-nightly-defconfig
> 
> First seen on Linux next-20241216.
>   Good: next-20241216
>   Bad:  next-20241213
> 
> Build log:
> -----------
<trimmed irrelevant warning>
> drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto
> statement to its label
>   365 |                 goto err_free_ahash;
>       |                 ^
> drivers/crypto/qce/sha.c:373:6: note: jump bypasses initialization of
> variable with __attribute__((cleanup))
>   373 |         u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
>       |             ^
> 1 error generated.

It is a bug to jump over the initialization of a cleanup variable
because the cleanup function will be called on an uninitialized pointer
in those cases. GCC does not catch this at compile time like clang does
(it would be nice if we could document this somewhere and really
encourage people doing cleanup annotations to ensure their patches pass
a clang build except in architecture code where clang does not support
that target):

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951

It may be worth just reverting commit ce8fd0500b74 ("crypto: qce - use
__free() for a buffer that's always freed") since it seems like little
value in this case but if we want to forward fix it, I think we could
just mirror what the rest of the kernel does and keep the declaration at
the top of the function and initialize the pointer to NULL. The diff
below resolves the issue for me, which I don't mind sending as a formal
patch.

Cheers,
Nathan

diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
index c4ddc3b265ee..e251f0f9a4fd 100644
--- a/drivers/crypto/qce/sha.c
+++ b/drivers/crypto/qce/sha.c
@@ -337,6 +337,7 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
 	struct scatterlist sg;
 	unsigned int blocksize;
 	struct crypto_ahash *ahash_tfm;
+	u8 *buf __free(kfree) = NULL;
 	int ret;
 	const char *alg_name;
 
@@ -370,8 +371,7 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
 				   crypto_req_done, &wait);
 	crypto_ahash_clear_flags(ahash_tfm, ~0);
 
-	u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
-					GFP_KERNEL);
+	buf = kzalloc(keylen + QCE_MAX_ALIGN_SIZE, GFP_KERNEL);
 	if (!buf) {
 		ret = -ENOMEM;
 		goto err_free_req;

Re: next-20241216: drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto statement to its label

[Bartosz Golaszewski]

On Mon, 16 Dec 2024 at 19:02, Nathan Chancellor <nathan@kernel.org> wrote:
>
> Hi Naresh,
>
> Thanks for the report.
>
> + Bartosz as author of ce8fd0500b74
>
> On Mon, Dec 16, 2024 at 10:04:05PM +0530, Naresh Kamboju wrote:
> > The arm and arm64 builds failed on Linux next-20241216 due to following
> > build warnings / errors with clang-19 and clang-nightly toolchain.
> > Whereas the gcc-13 builds pass.
> >
> > arm, arm64:
> >   * build/clang-19-defconfig
> >   * build/clang-nightly-defconfig
> >
> > First seen on Linux next-20241216.
> >   Good: next-20241216
> >   Bad:  next-20241213
> >
> > Build log:
> > -----------
> <trimmed irrelevant warning>
> > drivers/crypto/qce/sha.c:365:3: error: cannot jump from this goto
> > statement to its label
> >   365 |                 goto err_free_ahash;
> >       |                 ^
> > drivers/crypto/qce/sha.c:373:6: note: jump bypasses initialization of
> > variable with __attribute__((cleanup))
> >   373 |         u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
> >       |             ^
> > 1 error generated.
>
> It is a bug to jump over the initialization of a cleanup variable
> because the cleanup function will be called on an uninitialized pointer
> in those cases. GCC does not catch this at compile time like clang does
> (it would be nice if we could document this somewhere and really
> encourage people doing cleanup annotations to ensure their patches pass
> a clang build except in architecture code where clang does not support
> that target):
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951
>
> It may be worth just reverting commit ce8fd0500b74 ("crypto: qce - use
> __free() for a buffer that's always freed") since it seems like little
> value in this case but if we want to forward fix it, I think we could
> just mirror what the rest of the kernel does and keep the declaration at
> the top of the function and initialize the pointer to NULL. The diff
> below resolves the issue for me, which I don't mind sending as a formal
> patch.
>
> Cheers,
> Nathan

I'm fine with dropping that commit from next.

Bartosz

>
> diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
> index c4ddc3b265ee..e251f0f9a4fd 100644
> --- a/drivers/crypto/qce/sha.c
> +++ b/drivers/crypto/qce/sha.c
> @@ -337,6 +337,7 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
>         struct scatterlist sg;
>         unsigned int blocksize;
>         struct crypto_ahash *ahash_tfm;
> +       u8 *buf __free(kfree) = NULL;
>         int ret;
>         const char *alg_name;
>
> @@ -370,8 +371,7 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
>                                    crypto_req_done, &wait);
>         crypto_ahash_clear_flags(ahash_tfm, ~0);
>
> -       u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
> -                                       GFP_KERNEL);
> +       buf = kzalloc(keylen + QCE_MAX_ALIGN_SIZE, GFP_KERNEL);
>         if (!buf) {
>                 ret = -ENOMEM;
>                 goto err_free_req;