On Tue, Oct 10, 2023 at 12:28 AM Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Tao,
On Thu, 28 Sep 2023 21:20:28 +0800
Tao Liu <ltao(a)redhat.com> wrote:
> The kdumpctl test involves the following procedures:
>
> 1) Output the current kdump config to console, including crash kernel size
> and effective fields of kdump.conf.
> 2) A countdown function, to wait for the crash & dump confirmation from user.
> 3) Generating a timestamp as the id for the current kdump test. The id will
> be in kexec-dmesg.log for kdump or vmcore-dmesg.txt for fadump, and also
> in KDUMP_TEST_ENV file. kdumpctl test-check will get the test id from
> KDUMP_TEST_ENV file, and check if the test id exists in kexec-dmesg.log
> or vmcore-dmesg.txt.
>
> Signed-off-by: Tao Liu <ltao(a)redhat.com>
> ---
> kdumpctl | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 140 insertions(+), 1 deletion(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 6e4685f..b54b5ae 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -16,6 +16,8 @@ KDUMP_INITRD=""
> TARGET_INITRD=""
> #kdump shall be the default dump mode
> DEFAULT_DUMP_MODE="kdump"
> +KDUMP_TEST_ENV="/var/kdump/.kdump_test"
> +KDUMP_TEST_PREFIX="kdump_test_"
>
> standard_kexec_args="-d -p"
>
> @@ -1073,6 +1075,10 @@ reload()
> dwarn "Kdump was not running: [WARNING]"
> fi
>
> + if [[ -f "$KDUMP_TEST_ENV" ]]; then
> + . "$KDUMP_TEST_ENV"
> + fi
Sorry, but I'm strongly against this. You are executing unconditionally
a file with well known name. Throw in a (accidental) problem with file
permissions and you have a vulnerability that allows execution of
arbitrary code as root!
Yeah, thanks for raising the security issue, which I wasn't aware of.
Maybe performing a file owner and permission check before sourcing the
file can solve this?
> +
> if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
> reload_fadump
> return
> @@ -1665,6 +1671,136 @@ reset_crashkernel_for_installed_kernel()
> _update_crashkernel "$_installed_kernel"
> }
>
> +countdown()
> +{
> + local _sec=$1 _msg=$2
> +
> + while [[ $_sec -gt 0 ]]; do
> + printf "\r$_msg %ds " "$_sec"
> + sleep 1
> + _sec=$((_sec - 1))
> + done
> +}
> +
> +kill_countdown()
> +{
> + local _pid=$1
> +
> + kill "$_pid" 2>/dev/null
> + if [[ -n "$2" ]]; then
> + echo -e "$2"
> + fi
> + return 0
> +}
> +
> +output_conf()
> +{
> + local -n conf=$1
> + local _width _max_width=0 _line_width _elem _key
> +
> + for _elem in "${!conf[@]}"; do
> + _width=${#_elem}
> + if (( _width > _max_width )); then
> + _max_width=$_width
> + fi
> + done
> + _line_width=$_max_width
> + _max_width=0
> + for _elem in "${conf[@]}"; do
> + _width=${#_elem}
> + if (( _width > _max_width )); then
> + _max_width=$_width
> + fi
> + done
> + _line_width=$((_line_width + _max_width))
> +
> + echo -e "\nCurrent kdump configuration:"
> + printf -- '-%.0s' $(seq -s " " $((_line_width + 1))); echo
> + for _key in "${!conf[@]}"; do
> + printf "%-$(( _line_width - _max_width ))s %s\n"
"$_key" "${conf[$_key]}"
> + done
> + printf -- '-%.0s' $(seq -s " " $((_line_width + 1))); echo
> +}
> +
> +get_all_kdump_conf()
> +{
> + local _max_width=0 _width _elem _line_width _key _func=$1
Looks like most of the variables were move to output_conf. You should
clean them up.
> + declare -A kdump_conf
kdump_conf should be local as well.
> + parse_config || return
> +
> + for _elem in "${!OPT[@]}"; do
> + kdump_conf["$_elem"]="${OPT[$_elem]}"
> + done
> +
> + if [[ "$DEFAULT_DUMP_MODE" == "kdump" ||
"$DEFAULT_DUMP_MODE" == "fadump" ]]; then
> +
kdump_conf["crashkernel_size"]="$(($(get_reserved_mem_size) / 1024 /
1024))M"
> + kdump_conf["dump_mode"]="$DEFAULT_DUMP_MODE"
> + else
> + return
> + fi
> +
> + if [ -n "$_func" ]; then
> + $_func kdump_conf
> + fi
What's the point of this function when no _func is provided? The way I
see it then turns in a very complicated call to parse_config. But that
should probably be done explicitly at the beginning of kdump_do_test as
you will access $OPT later anyways.
The purpose of the function is to create a kdump_conf array which
include all kdump configs, including the fields of /etc/kdump.conf and
also crashkernel size and dump mode(kdump/fadump). As you see,
crashkernel is also an important factor to decide the success of
kdump. ${!OPT[@]} is only a copy of /etc/kdump.conf.
I would expect the $_func is never empty. Since it is a callback
function which handles ${kdump_conf}, either output it or generate
md5sum for kdump conf modification check.
Yeah, I will check to place it at the beginning of kdump_do_test.
> +}
> +
> +kdump_do_test()
> +{
> + local _input _countdown_pid
> + local _TIMEOUT=10 _TEST_ID="$KDUMP_TEST_PREFIX""$(date
+%Y-%m-%d-%T)"
> +
> + is_kernel_loaded "$DEFAULT_DUMP_MODE"
> + case "$?" in
> + 0)
> + get_all_kdump_conf output_conf
> + countdown "$_TIMEOUT" "Will test kdump based on the
config, a crash dump & reboot will be performed, proceed? (yes/[no]):" &
> + _countdown_pid="$!"
> +
> + read -t "$_TIMEOUT" _input
> +
> + if [[ -z "$_input" ]]; then
> + kill_countdown "$_countdown_pid" "\nKdump test
aborted." && exit 0
> + elif [[ "$_input" == "no" ]]; then
> + kill_countdown "$_countdown_pid" "Kdump test
aborted." && exit 0
> + elif [[ "$_input" == "yes" ]]; then
> + kill_countdown "$_countdown_pid"
> + countdown 2 "After machine restart, input \"kdumpctl
test-check\" to get the test result."
> + else
> + kill_countdown "$_countdown_pid" "Please input
yes or no." && exit 0
> + fi
> + wait
Is it really worth having this countdown? It adds quite a bit of
overhead. In fact I don't understand fully why there is a timeout in
the first place. I simply don't see any benefit it provides.
Yeah, the count down is making the code a bit overhead. Since it will
make the system crash and dump, is a dangerous operation. So I set a
bit more barriers to reach the final "echo c > /proc/sysrq-trigger".
If user didn't enter yes in a given time, the operation will abort. If
you think it is useless, we can simply remove it.
On a side note, I would really love that there is a more consistent way
in handling user input. In kdump.conf for example
auto_reset_crashkernel also uses yes|no while force_rebuild uses 1|0.
That's not really a problem with your code but still something we
should keep in mind.
Agreed
> + ;;
> + 1)
> + dinfo "Kdump should be enabled before testing"
> + exit 1
> + ;;
> + esac
The switch case is identical to
if !is_kernel_loaded "$DEFAULT_DUMP_MODE"; then
dinfo "Kdump should be enabled before testing"
exit 1
fi
get_all_kdump_conf output_conf
[...]
with the latter being much better readable.
Agreed, thanks!
> +
> + [[ ! -d $(dirname "$KDUMP_TEST_ENV") ]] && mkdir -p
$(dirname "$KDUMP_TEST_ENV")
> + echo "KDUMP_COMMANDLINE_APPEND+=\" $_TEST_ID \"" >
"$KDUMP_TEST_ENV"
> + if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
> + echo "$_TEST_ID" > /dev/kmsg
> + fi
> + sync
> +
check_and_wait_network_ready
> + setup_initrd || return 1
> + need_initrd_rebuild
> + case "$?" in
> + 0)
> + # Nothing to do
> + ;;
> + 1)
> + rebuild_initrd || return
> + ;;
> + *)
> + return
> + ;;
> + esac
> + reload
Thanks
Philipp
> + echo c > /proc/sysrq-trigger
> +}
> +
> main()
> {
> # Determine if the dump mode is kdump or fadump
> @@ -1740,8 +1876,11 @@ main()
> reset_crashkernel_for_installed_kernel "$2"
> fi
> ;;
> + test)
> + kdump_do_test
> + ;;
> *)
> - dinfo $"Usage: $0
{estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}"
> + dinfo $"Usage: $0
{estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem|test|test-check}"
> exit 1
> ;;
> esac