[PATCH] kdumpctl: Detect block device driver change for initramfs rebuild

[Kairui Song]

Previous we rebuild the initramfs when kenrel load module list changed,
but this is not very stable as some async services may load/unload
kernel modules, and cause unnecessary initramfs rebuild.

Instead, it's better to just check if the module required to dump to
the dump target is loaded or not, and rebuild if not loaded. This
avoids most false-positives, and ensure local target change is always
covered.

Currently only local fs dump target is covered, because this check
requires the dump target to be mounted when building the initramfs,
this guarantee that the module is in the loaded kernel module list,
else we may still get some false positive.

dracut-install could be leveraged to combine the modalias list with
kernel loaded module list as a more stable module list in the initramfs,
but upstream dracut change need to be done first.

Passed tests on a KVM VM, changing the storage between SATA/USB/VirtIO
will trigger initramfs rebuild when new module is required for target
block device, and didn't notice any false-positive on any machines.

Signed-off-by: Kairui Song <kasong(a)redhat.com&gt;
---
 kdumpctl | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index 3f80ba4..846a45b 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -357,6 +357,9 @@ check_dump_fs_modified()
 	local _old_dev _old_mntpoint _old_fstype
 	local _new_dev _new_mntpoint _new_fstype
 	local _target _path _dracut_args
+	local _target_drivers _module_name
+
+	local _old_drivers="$(lsinitrd $TARGET_INITRD -f
/usr/lib/dracut/loaded-kernel-modules.txt | tr '\n' ' ')"
 
 	# No need to check in case of mount target specified via "dracut_args".
 	if is_mount_in_dracut_args; then
@@ -385,6 +388,36 @@ check_dump_fs_modified()
 		fi
 	fi
 
+	_record_block_drivers() {
+		local _drivers
+		if [[ -b /dev/block/$1 ]]; then
+			_drivers=$(udevadm info -a "/dev/block/$1" | sed -n
's/\s*DRIVERS=="\(\S\+\)"/\1/p')
+		fi
+		if [[ -b $1 ]]; then
+			_drivers=$(udevadm info -a "$1" | sed -n
's/\s*DRIVERS=="\(\S\+\)"/\1/p')
+		fi
+		for _driver in $_drivers; do
+			if ! [[ " $_target_drivers " == *" $_driver "* ]]; then
+				_target_drivers="$_target_drivers $_driver"
+			fi
+		done
+		return 1
+	}
+
+	check_block_and_slaves_all _record_block_drivers "$(get_maj_min
"$_target")"
+	for _driver in $_target_drivers; do
+		# Target is mounted already, if module is not included by current kernel,
+		# could be a deprecated/invalid driver name
+		_module_name=$(modinfo --set-version "$kdump_kver" -F name $_driver
2>/dev/null)
+		if [ $? -ne 0 ] || [ -z "$_module_name" ]; then
+			continue
+		fi
+		if ! [[ " $_old_drivers " == *" $_module_name "* ]]; then
+			echo "Detected change in block device driver, $_module_name is not
included"
+			return 1
+		fi
+	done
+
 	if [[ $(expr substr $_new_fstype 1 3) = "nfs" ]];then
 		_new_dev=$_target
 	else
-- 
2.20.1

[Dave Young]

On 05/07/19 at 05:14pm, Kairui Song wrote:
>  Previous we rebuild the initramfs when kenrel load module list
> changed,
>  but this is not very stable as some async services may load/unload
>  kernel modules, and cause unnecessary initramfs rebuild.
>  
>  Instead, it's better to just check if the module required to dump to
>  the dump target is loaded or not, and rebuild if not loaded. This
>  avoids most false-positives, and ensure local target change is always
>  covered.
>  
>  Currently only local fs dump target is covered, because this check
>  requires the dump target to be mounted when building the initramfs,
>  this guarantee that the module is in the loaded kernel module list,
>  else we may still get some false positive.
>  
>  dracut-install could be leveraged to combine the modalias list with
>  kernel loaded module list as a more stable module list in the initramfs,
>  but upstream dracut change need to be done first.
>  
>  Passed tests on a KVM VM, changing the storage between SATA/USB/VirtIO
>  will trigger initramfs rebuild when new module is required for target
>  block device, and didn't notice any false-positive on any machines.
>  
>  Signed-off-by: Kairui Song <kasong(a)redhat.com&gt;
>  ---
>   kdumpctl | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>  
>  diff --git a/kdumpctl b/kdumpctl
>  index 3f80ba4..846a45b 100755
>  --- a/kdumpctl
>  +++ b/kdumpctl
>  @@ -357,6 +357,9 @@ check_dump_fs_modified()
>   	local _old_dev _old_mntpoint _old_fstype
>   	local _new_dev _new_mntpoint _new_fstype
>   	local _target _path _dracut_args
>  +	local _target_drivers _module_name
>  +
>  +	local _old_drivers="$(lsinitrd $TARGET_INITRD -f
> /usr/lib/dracut/loaded-kernel-modules.txt | tr '\n' ' ')"
>   
>   	# No need to check in case of mount target specified via "dracut_args".
>   	if is_mount_in_dracut_args; then
>  @@ -385,6 +388,36 @@ check_dump_fs_modified()
>   		fi
>   	fi
>   
>  +	_record_block_drivers() {
>  +		local _drivers
>  +		if [[ -b /dev/block/$1 ]]; then
>  +			_drivers=$(udevadm info -a "/dev/block/$1" | sed -n
> 's/\s*DRIVERS=="\(\S\+\)"/\1/p')
>  +		fi
>  +		if [[ -b $1 ]]; then
>  +			_drivers=$(udevadm info -a "$1" | sed -n
> 's/\s*DRIVERS=="\(\S\+\)"/\1/p')
>  +		fi
>  +		for _driver in $_drivers; do
>  +			if ! [[ " $_target_drivers " == *" $_driver "* ]]; then
>  +				_target_drivers="$_target_drivers $_driver"
>  +			fi
>  +		done
>  +		return 1
>  +	}
>  +
>  +	check_block_and_slaves_all _record_block_drivers "$(get_maj_min
> "$_target")"
>  +	for _driver in $_target_drivers; do
>  +		# Target is mounted already, if module is not included by current kernel,
>  +		# could be a deprecated/invalid driver name 
nitpick: could be a deprecated/invalid driver name or built-in module

Otherwise:

Ack.

Thanks!

> +		_module_name=$(modinfo --set-version "$kdump_kver" -F name $_driver
2>/dev/null)
> +		if [ $? -ne 0 ] || [ -z "$_module_name" ]; then
> +			continue
> +		fi
> +		if ! [[ " $_old_drivers " == *" $_module_name "* ]]; then
> +			echo "Detected change in block device driver, $_module_name is not
included"
> +			return 1
> +		fi
> +	done
> +
>  	if [[ $(expr substr $_new_fstype 1 3) = "nfs" ]];then
>  		_new_dev=$_target
>  	else
> -- 
> 2.20.1
>

[Kairui Song]

Thanks, just found that older version of kmod won't work with "-F
name", so will update the patch to cover this corner case and fix the
message.

On Tue, May 7, 2019 at 5:52 PM Dave Young <dyoung(a)redhat.com&gt; wrote:
>  On 05/07/19 at 05:14pm, Kairui Song wrote:
>  > Previous we rebuild the initramfs when kenrel load module list changed,
>  > but this is not very stable as some async services may load/unload
>  > kernel modules, and cause unnecessary initramfs rebuild.
>  >
>  > Instead, it's better to just check if the module required to dump to
>  > the dump target is loaded or not, and rebuild if not loaded. This
>  > avoids most false-positives, and ensure local target change is always
>  > covered.
>  >
>  > Currently only local fs dump target is covered, because this check
>  > requires the dump target to be mounted when building the initramfs,
>  > this guarantee that the module is in the loaded kernel module list,
>  > else we may still get some false positive.
>  >
>  > dracut-install could be leveraged to combine the modalias list with
>  > kernel loaded module list as a more stable module list in the initramfs,
>  > but upstream dracut change need to be done first.
>  >
>  > Passed tests on a KVM VM, changing the storage between SATA/USB/VirtIO
>  > will trigger initramfs rebuild when new module is required for target
>  > block device, and didn't notice any false-positive on any machines.
>  >
>  > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt;
>  > ---
>  >  kdumpctl | 33 +++++++++++++++++++++++++++++++++
>  >  1 file changed, 33 insertions(+)
>  >
>  > diff --git a/kdumpctl b/kdumpctl
>  > index 3f80ba4..846a45b 100755
>  > --- a/kdumpctl
>  > +++ b/kdumpctl
>  > @@ -357,6 +357,9 @@ check_dump_fs_modified()
>  >       local _old_dev _old_mntpoint _old_fstype
>  >       local _new_dev _new_mntpoint _new_fstype
>  >       local _target _path _dracut_args
>  > +     local _target_drivers _module_name
>  > +
>  > +     local _old_drivers="$(lsinitrd $TARGET_INITRD -f
> /usr/lib/dracut/loaded-kernel-modules.txt | tr '\n' ' ')"
>  >
>  >       # No need to check in case of mount target specified via
> "dracut_args".
>  >       if is_mount_in_dracut_args; then
>  > @@ -385,6 +388,36 @@ check_dump_fs_modified()
>  >               fi
>  >       fi
>  >
>  > +     _record_block_drivers() {
>  > +             local _drivers
>  > +             if [[ -b /dev/block/$1 ]]; then
>  > +                     _drivers=$(udevadm info -a "/dev/block/$1" | sed -n
> 's/\s*DRIVERS=="\(\S\+\)"/\1/p')
>  > +             fi
>  > +             if [[ -b $1 ]]; then
>  > +                     _drivers=$(udevadm info -a "$1" | sed -n
> 's/\s*DRIVERS=="\(\S\+\)"/\1/p')
>  > +             fi
>  > +             for _driver in $_drivers; do
>  > +                     if ! [[ " $_target_drivers " == *" $_driver
> "* ]]; then
>  > +                             _target_drivers="$_target_drivers $_driver"
>  > +                     fi
>  > +             done
>  > +             return 1
>  > +     }
>  > +
>  > +     check_block_and_slaves_all _record_block_drivers "$(get_maj_min
> "$_target")"
>  > +     for _driver in $_target_drivers; do
>  > +             # Target is mounted already, if module is not included by current
> kernel,
>  > +             # could be a deprecated/invalid driver name
>
>  nitpick: could be a deprecated/invalid driver name or built-in module
>
>  Otherwise:
>
>  Ack.
>
>  Thanks!
>
>  > +             _module_name=$(modinfo --set-version "$kdump_kver" -F name
> $_driver 2>/dev/null)
>  > +             if [ $? -ne 0 ] || [ -z "$_module_name" ]; then
>  > +                     continue
>  > +             fi
>  > +             if ! [[ " $_old_drivers " == *" $_module_name "*
> ]]; then
>  > +                     echo "Detected change in block device driver,
> $_module_name is not included"
>  > +                     return 1
>  > +             fi
>  > +     done
>  > +
>  >       if [[ $(expr substr $_new_fstype 1 3) = "nfs" ]];then
>  >               _new_dev=$_target
>  >       else
>  > --
>  > 2.20.1
>  > 


-- 
Best Regards,
Kairui Song