Message-ID: <CAK7LNASvKdrv7tt154xh+DFLYs41Az_FbAnSXHF2pGRvh==6mg@mail.gmail.com>
Date:   Tue, 14 Nov 2023 22:28:05 +0900
From:   Masahiro Yamada <masahiroy@...nel.org>
To:     John Moon <quic_johmoo@...cinc.com>
Cc:     Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nicolas Schier <nicolas@...sle.eu>,
        Jonathan Corbet <corbet@....net>, linux-kbuild@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, kernel@...cinc.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Bjorn Andersson <andersson@...nel.org>,
        Todd Kjos <tkjos@...gle.com>,
        Matthias Maennich <maennich@...gle.com>,
        Giuliano Procida <gprocida@...gle.com>,
        kernel-team@...roid.com, libabigail@...rceware.org,
        Dodji Seketeli <dodji@...hat.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Satya Durga Srinivasu Prabhala <quic_satyap@...cinc.com>,
        Jordan Crouse <jorcrous@...zon.com>
Subject: Re: [PATCH v6 3/3] check-module-params: Introduce check-module-params.sh

On Sat, Oct 28, 2023 at 4:31 AM John Moon <quic_johmoo@...cinc.com> wrote:
>
> One part of maintaining backwards compatibility with older
> userspace programs is avoiding changes to module parameters.
>
> To that end, add a script (check-module-params.sh) which
> performs a simple check of module parameter changes across
> git references.
>
> For example, if this module parameter:
>
> module_param(max_nullfunc_tries, int, 0644);
>
> ...restricted its mode parameter:
>
> module_param(max_nullfunc_tries, int, 0600);
>
> The script would flag the change:
>
> Module parameter "max_nullfunc_tries" in net/mac80211/mlme.c changed!
>   Original args: int,0644
>        New args: int,0600



I know this is just a simple diff, and we cannot expect
accuracy from this tool.



I just tried

  $ ./scripts/check-module-params.sh  -b v6.7-rc1  -p v6.6

Then, I got

  error - 21/576 files with modules parameters appear _not_ to be
backwards compatible



In my understanding, it includes many false alarms.

In most of the cases, the driver was just removed.




[pattern 1] Driver was removed

Module parameter "sal_rec_max" in arch/ia64/kernel/mca_drv.c removed!

 or

Module parameter "up_delay" in
drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c removed!



[pattern 2] cosmetic change

Module parameter "nested" in arch/x86/kvm/vmx/vmx.c changed!
  Original args: bool,S_IRUGO
       New args: bool,0444





But, it sometimes catches real ones:


Module parameter "ublks_max" in drivers/block/ublk_drv.c changed!
  Original args: int,0444
       New args: &ublk_max_ublks_ops,&ublks_max,0644

 -->  Need a close check



Module parameter "vm_debug" in drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c removed!
  Original args: amdgpu_vm_debug,int,0644

 --> 887db1e49a73 is a breakage, but it seems to be intentional
     according to the commit log








> Signed-off-by: John Moon <quic_johmoo@...cinc.com>
> ---
>  scripts/check-module-params.sh | 295 +++++++++++++++++++++++++++++++++
>  1 file changed, 295 insertions(+)
>  create mode 100755 scripts/check-module-params.sh
>
> diff --git a/scripts/check-module-params.sh b/scripts/check-module-params.sh
> new file mode 100755
> index 000000000000..4d2b2cd483e8
> --- /dev/null
> +++ b/scripts/check-module-params.sh
> @@ -0,0 +1,295 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-only
> +# Script to check commits for UAPI backwards compatibility
> +
> +set -o errexit
> +set -o pipefail
> +
> +print_usage() {
> +       name=$(basename "$0")
> +       cat << EOF
> +$name - check for module parameters stability across git commits.
> +
> +By default, the script will check to make sure the latest commit (or current
> +dirty changes) did not introduce changes when compared to HEAD^1. You can
> +check against additional commit ranges with the -b and -p options.
> +
> +Usage: $name [-b BASE_REF] [-p PAST_REF] [-j N] [-l ERROR_LOG] [-q] [-v]
> +
> +Options:
> +    -b BASE_REF    Base git reference to use for comparison. If unspecified or empty,
> +                   will use any dirty changes in tree to UAPI files. If there are no
> +                   dirty changes, HEAD will be used.
> +    -p PAST_REF    Compare BASE_REF to PAST_REF (e.g. -p v6.1). If unspecified or empty,
> +                   will use BASE_REF^1. Must be an ancestor of BASE_REF. Only headers
> +                   that exist on PAST_REF will be checked for compatibility.
> +    -j JOBS        Number of checks to run in parallel (default: number of CPU cores).
> +    -l ERROR_LOG   Write error log to file (default: no error log is generated).
> +    -q             Quiet operation (suppress stdout, still print stderr).
> +    -v             Verbose operation (print more information about each header being checked).
> +
> +Exit codes:
> +    $SUCCESS) Success
> +    $FAILURE) Module param differences detected
> +EOF
> +}
> +
> +readonly SUCCESS=0
> +readonly FAILURE=1
> +
> +# Print to stderr
> +eprintf() {
> +       # shellcheck disable=SC2059
> +       printf "$@" >&2
> +}
> +
> +# Check if git tree is dirty
> +tree_is_dirty() {
> +       ! git diff --quiet
> +}
> +
> +file_module_params_unmodified() {
> +       local file="$1"
> +       local base_ref="$2"
> +       local past_ref="$3"
> +       local base_params_file="${TMP_DIR}/${file}.base"
> +       local past_params_file="${TMP_DIR}/${file}.past"
> +       local error_log="${TMP_DIR}/${file}.error"
> +
> +       local -r awk_cmd='/^ *module_param.*\(/,/.*\);/'
> +
> +       mkdir -p "$(dirname "$error_log")"
> +       git show "${past_ref}:${file}" 2> /dev/null \
> +               | awk "$awk_cmd" > "$past_params_file" || true
> +
> +       # Ignore files that don't exist at the past ref or don't have module params
> +       if [ ! -s "$past_params_file" ]; then
> +               return 255 # Special return code for "no-op"
> +       fi
> +
> +       if [ -z "$base_ref" ]; then
> +               awk "$awk_cmd" "${KERNEL_SRC}/${file}" \
> +                       > "$base_params_file" 2> /dev/null || true
> +       else
> +               git show "${base_ref}:${file}" 2> /dev/null \
> +                       | awk "$awk_cmd" > "$base_params_file" || true
> +       fi
> +
> +       # Process the param data to come up with an associative array of param names to param data
> +       # For example:
> +       #   module_param_call(foo, set_result, get_result, NULL, 0600);
> +       #
> +       # is processed into:
> +       #   pre_change_params[foo]="set_result,get_result,NULL,0600"
> +       local -A pre_change_params
> +       local param_name
> +       local param_params
> +       while read -r mod_param_args; do
> +               param_name="$(echo "$mod_param_args" | cut -d ',' -f 1)"
> +               param_params="$(echo "$mod_param_args" | cut -d ',' -f 2-)"
> +
> +               pre_change_params[$param_name]=$param_params
> +       done < <(tr -d '\t\n ' < "$past_params_file" | tr ';' '\n' | grep -o '(.*)' | tr -d '()')


Maybe

  grep -o '(.*)' | tr -d '()'

can become a single process,

  sed 's/.*(\(.*\))/\1/'





> +
> +       local -A post_change_params
> +       while read -r mod_param_args; do
> +               param_name="$(echo "$mod_param_args" | cut -d ',' -f 1)"
> +               param_params="$(echo "$mod_param_args" | cut -d ',' -f 2-)"
> +
> +               post_change_params[$param_name]=$param_params
> +       done < <(tr -d '\t\n ' < "$base_params_file" | tr ';' '\n' | grep -o '(.*)' | tr -d '()')
> +
> +       # Flag any module param changes that:
> +       #  - Remove/rename a parameter
> +       #  - Change the arguments of the parameter
> +       local incompat_param_changes=0
> +       local pre
> +       local post
> +       for param_name in "${!pre_change_params[@]}"; do
> +               pre="${pre_change_params[$param_name]}"
> +               if [ ! "${post_change_params[$param_name]+set}" ]; then
> +                       {
> +                               printf "Module parameter \"%s\" in %s removed!\n" "$param_name" "$file"
> +                               printf "  Original args: %s\n" "$pre"
> +                       } > "$error_log"
> +                       incompat_param_changes=$((incompat_param_changes + 1))
> +                       continue
> +               fi
> +
> +               post="${post_change_params[$param_name]}"
> +               if [ "$pre" != "$post" ]; then
> +                       {
> +                               printf "Module parameter \"%s\" in %s changed!\n" "$param_name" "$file"
> +                               printf "  Original args: %s\n" "$pre"
> +                               printf "       New args: %s\n" "$post"
> +                       } > "$error_log"
> +                       incompat_param_changes=$((incompat_param_changes + 1))
> +                       continue
> +               fi
> +       done
> +
> +       if [ "$incompat_param_changes" -gt 0 ]; then
> +               return 1
> +       fi
> +}
> +
> +run() {
> +       local base_ref="$1"
> +       local past_ref="$2"
> +       local abi_error_log="$3"
> +
> +       diff_args=("$past_ref")
> +       if [ -n "$base_ref" ]; then
> +               diff_args+=("$base_ref")
> +       fi
> +
> +       local -a threads=()
> +       local passed=0
> +       local failed=0
> +       printf "Checking files between %s and %s for module parameter compatibility...\n" \
> +               "$past_ref" "$base_ref"
> +       while read -r modified_file; do
> +               if [ "${#threads[@]}" -ge "$MAX_THREADS" ]; then
> +                       wait "${threads[0]}" && ret="$?" || ret="$?"
> +                       if [ "$ret" -eq 0 ]; then
> +                               passed=$((passed + 1))
> +                       elif [ "$ret" -eq 1 ]; then
> +                               failed=$((failed + 1))
> +                       fi
> +                       threads=("${threads[@]:1}")
> +               fi
> +
> +               file_module_params_unmodified "$modified_file" "$base_ref" "$past_ref" &
> +               threads+=("$!")
> +       done < <(git diff --diff-filter=MCD --name-only "${diff_args[@]}" -- '*.c' '*.h')
> +
> +       for t in "${threads[@]}"; do
> +               wait "$t" && ret="$?" || ret="$?"
> +               if [ "$ret" -eq 0 ]; then
> +                       passed=$((passed + 1))
> +               elif [ "$ret" -eq 1 ]; then
> +                       failed=$((failed + 1))
> +               fi
> +       done
> +
> +       total=$((passed + failed))
> +       if [ "$total" -eq 0 ]; then
> +               printf "No files with module parameters modified between %s and %s\n" \
> +                       "$past_ref" "${base_ref:-dirty tree}"
> +               exit 0
> +       fi
> +
> +       if [ -n "$abi_error_log" ]; then
> +               printf 'Generated by "%s %s" from git ref %s\n\n' \
> +                       "$0" "$*" "$(git rev-parse HEAD)" > "$abi_error_log"
> +       fi
> +
> +       while read -r error_file; do
> +               {
> +                       cat "$error_file"
> +                       printf "\n\n"
> +               } | tee -a "${abi_error_log:-/dev/null}" >&2
> +       done < <(find "$TMP_DIR" -type f -name '*.error')
> +
> +       if [ "$failed" -gt 0 ]; then
> +               eprintf "error - %d/%d files with modules parameters appear _not_ to be backwards compatible\n" \
> +                       "$failed" "$total"
> +               if [ -n "$abi_error_log" ]; then
> +                       eprintf "Failure summary saved to %s\n" "$abi_error_log"
> +               fi
> +       else
> +               printf "All %d files with module_parameters checked appear to be backwards compatible\n" \
> +                       "$total" "$ARCH"
> +       fi
> +
> +       exit "$failed"
> +}
> +
> +# Make sure the git refs we have make sense
> +check_refs() {
> +       if ! git rev-parse --is-inside-work-tree > /dev/null 2>&1; then
> +               eprintf "error - this script requires the kernel tree to be initialized with Git\n"
> +               return 1
> +       fi
> +
> +       if ! git rev-parse --verify "$past_ref" > /dev/null 2>&1; then
> +               printf 'error - invalid git reference "%s"\n' "$past_ref"
> +               return 1
> +       fi
> +
> +       if [ -n "$base_ref" ]; then
> +               if ! git merge-base --is-ancestor "$past_ref" "$base_ref" > /dev/null 2>&1; then
> +                       printf 'error - "%s" is not an ancestor of base ref "%s"\n' "$past_ref" "$base_ref"
> +                       return 1
> +               fi
> +               if [ "$(git rev-parse "$base_ref")" = "$(git rev-parse "$past_ref")" ]; then
> +                       printf 'error - "%s" and "%s" are the same reference\n' "$past_ref" "$base_ref"
> +                       return 1
> +               fi
> +       fi
> +}
> +
> +main() {
> +       MAX_THREADS=$(nproc)
> +       quiet="false"
> +       local base_ref=""
> +       while getopts "hb:p:j:l:q" opt; do
> +               case $opt in
> +               h)
> +                       print_usage
> +                       exit "$SUCCESS"
> +                       ;;
> +               b)
> +                       base_ref="$OPTARG"
> +                       ;;
> +               p)
> +                       past_ref="$OPTARG"
> +                       ;;
> +               j)
> +                       MAX_THREADS="$OPTARG"
> +                       ;;
> +               l)
> +                       abi_error_log="$OPTARG"
> +                       ;;
> +               q)
> +                       quiet="true"
> +                       ;;
> +               *)
> +                       exit "$FAIL_PREREQ"
> +               esac
> +       done
> +
> +       if [ "$quiet" = "true" ]; then
> +               exec > /dev/null 2>&1
> +       fi
> +
> +       if [ -z "$KERNEL_SRC" ]; then
> +               KERNEL_SRC="$(realpath "$(dirname "$0")"/..)"
> +       fi
> +
> +       cd "$KERNEL_SRC"
> +
> +       if [ -z "$base_ref" ] && ! tree_is_dirty; then
> +               base_ref=HEAD
> +       fi
> +
> +       if [ -z "$past_ref" ]; then
> +               if [ -n "$base_ref" ]; then
> +                       past_ref="${base_ref}^1"
> +               else
> +                       past_ref=HEAD
> +               fi
> +       fi
> +
> +       if ! check_refs; then
> +               exit "$FAIL_PREREQ"
> +       fi
> +
> +       TMP_DIR=$(mktemp -d)
> +       readonly TMP_DIR
> +       trap 'rm -rf "$TMP_DIR"' EXIT
> +
> +       run "$base_ref" "$past_ref" "$abi_error_log"
> +}
> +
> +main "$@"
> --
> 2.17.1
>


--
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists