On Tue, 10 Oct 2023 11:17:47 +0800
Tao Liu <ltao(a)redhat.com> wrote:
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?
Or you simply communicate via file on the dump target as I suggested on
the cover letter. Then you don't need to add anything to the command
line at all ;-)
>
> > +
> > 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.
I fully understand that. My main point here is that when this function
requires a callback to work properly it should raise an error if no
_func is provided. Currently it only builds kdump_conf and then does
nothing with it.
Furthermore, you are totally right, that crashkernel_size and dump_mode
are important factors for kdump. But so are the kernel version,
everything that is packed in the initrd including various config files,
the kernel command line, etc.. So what's the reason for picking exactly
those two informations?
Yeah, I will check to place it at the beginning of kdump_do_test.
Thanks
>
> > +}
> > +
> > +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.
You are right that cashing the system is a drastic operation and I don't
have a problem with asking the user to proceed (although it would be
great if the user could have a --force option to skip the question).
All I'm questioning is the timeout and countdown. Why not just wait
till the user entered something? This already would simplify the code
quite drastically.
> 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
Thanks
Philipp
>
> > + ;;
> > + 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
>
_______________________________________________
kexec mailing list -- kexec(a)lists.fedoraproject.org
To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org
Do not reply to spam, report it:
https://pagure.io/fedora-infrastructure/new_issue