linux-kernel - Re: [PATCH v2] kbuild: pahole-version: improve overall checking and error messages

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK7LNAQf4xpnypbBNA0PJqeBnHqCn7YnAg8kcay6dyYYSSLDmg@mail.gmail.com>
Date: Wed, 4 Sep 2024 11:04:18 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Miguel Ojeda <ojeda@...nel.org>
Cc: Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas@...sle.eu>, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH v2] kbuild: pahole-version: improve overall checking and
 error messages

On Tue, Sep 3, 2024 at 1:09 AM Miguel Ojeda <ojeda@...nel.org> wrote:
>
> Like patch "rust: suppress error messages from
> CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1],

Better to point the commit instead of the patch in ML.


Like commit 5ce86c6c8613 ("rust: suppress error messages
from CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT"),









> do not assume the file existing
> and being executable implies executing it will succeed.
>
> Instead, bail out if executing it fails for any reason, as well as if
> the program does not print to standard output what we are expecting from
> `pahole --version`. The script needs to ensure that it always returns
> an integer, since its output will go into a Kconfig `int`-type symbol.
>
> In addition, check whether the program exists first, and provide
> error messages for each error condition, similar to how it is done in
> e.g. `scripts/rust_is_available.sh`.
>
> For instance, currently `pahole` may be built for another architecture
> or may be a program we do not expect that fails:
>
>     $ echo 'bad' > bad-pahole
>     $ chmod u+x bad-pahole
>     $ make PAHOLE=./bad-pahole defconfig
>     ...
>     ./bad-pahole: 1: bad: not found
>     init/Kconfig:112: syntax error
>     init/Kconfig:112: invalid statement
>
> With this applied, we get instead:
>
>     ***
>     *** Running './bad-pahole' to check the pahole version failed with
>     *** code 127. pahole will not be used.
>     ***
>     ...
>     $ grep CONFIG_PAHOLE_VERSION .config
>     CONFIG_PAHOLE_VERSION=0
>
> Similarly, `pahole` currently may be a program that returns successfully,
> but that does not print the version that we would expect:
>
>     $ echo 'echo' > bad-pahole
>     $ chmod u+x bad-pahole
>     $ make PAHOLE=./bad-pahole defconfig
>     ...
>     init/Kconfig:114: syntax error
>     init/Kconfig:114: invalid statement
>
> With this applied, we get instead:
>
>     ***
>     *** pahole './bad-pahole' returned an unexpected version output.
>     *** pahole will not be used.
>     ***
>     ...
>     $ grep CONFIG_PAHOLE_VERSION .config
>     CONFIG_PAHOLE_VERSION=0
>
> Finally, with this applied, if the program does not exist, we get:
>
>     $ make PAHOLE=./bad-pahole defconfig
>     ...
>     ***
>     *** pahole './bad-pahole' could not be found. pahole will not be used.
>     ***
>     ...
>     $ grep CONFIG_PAHOLE_VERSION .config
>     CONFIG_PAHOLE_VERSION=0
>
> Link: https://lore.kernel.org/rust-for-linux/20240727140302.1806011-1-masahiroy@kernel.org/ [1]
> Co-developed-by: Nicolas Schier <nicolas@...sle.eu>
> Signed-off-by: Nicolas Schier <nicolas@...sle.eu>
> Signed-off-by: Miguel Ojeda <ojeda@...nel.org>
> ---
> v1: https://lore.kernel.org/all/20240728125527.690726-1-ojeda@kernel.org/
> v2:
>
> Reworked to catch successful programs that may not output what we expect from
> pahole as well as to do the checking step-by-step (for better error messages).
>
> I didn't change the regular expression to reduce the changes (except adding
> `p`) -- it can be improved separately.
>
> Also, please note that I added Nicolas as co-author since he proposed part of
> the solution, but he has not formally signed off yet.
>
>  scripts/pahole-version.sh | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh
> index f8a32ab93ad1..cdb80a3d6e4f 100755
> --- a/scripts/pahole-version.sh
> +++ b/scripts/pahole-version.sh
> @@ -5,9 +5,33 @@
>  #
>  # Prints pahole's version in a 3-digit form, such as 119 for v1.19.
>
> -if [ ! -x "$(command -v "$@")" ]; then
> -       echo 0
> +set -e
> +trap "echo 0; exit 1" EXIT
> +
> +if ! command -v "$@" >/dev/null; then
> +       echo >&2 "***"
> +       echo >&2 "*** pahole '$@' could not be found. pahole will not be used."
> +       echo >&2 "***"
> +       exit 1
> +fi
> +
> +output=$("$@" --version 2>/dev/null) || code=$?
> +if [ -n "$code" ]; then
> +       echo >&2 "***"
> +       echo >&2 "*** Running '$@' to check the pahole version failed with"
> +       echo >&2 "*** code $code. pahole will not be used."
> +       echo >&2 "***"
> +       exit 1
> +fi
> +
> +output=$(echo "$output" | sed -nE 's/v([0-9]+)\.([0-9]+)/\1\2/p')
> +if [ -z "${output}" ]; then
> +       echo >&2 "***"
> +       echo >&2 "*** pahole '$@' returned an unexpected version output."
> +       echo >&2 "*** pahole will not be used."
> +       echo >&2 "***"
>         exit 1
>  fi
>
> -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
> +echo "${output}"
> +trap EXIT
>
> base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
> --
> 2.46.0



-- 
Best Regards
Masahiro Yamada