kpatch: Add subcommand '--enabling' to adjust new sysfs attribute 'st…

[wardenjohn]

…ack_order' of livepatch

Add an sub command of kpatch list with '--enabling' option to adjust kernel new attribute 'stack_order' of livepatch of kernel v6.13 or later.

Now, using 'kpatch list --enabling' can output the enabling function in the system and the relationship from the livepatch module -> livepatch object -> livepatch function.

PS: This is a pre-commit. This patch should be merged after the 'stack_order' patch is merged into kernel v6.13 or later.

[joe-lawrence]

fyi for reviewers, here is a sample output when loading the kernel's kselftest modules used during the test of this sysfs feature:

$ kpatch list --enabling
Loaded patch modules:
test_klp_callbacks_demo [enabled]
test_klp_livepatch [enabled]
test_klp_syscall [enabled]

Installed patch modules:

The function enabling in the system:
Module                   Object                   Function
test_klp_syscall         vmlinux                  __x64_sys_getpid
test_klp_callbacks_demo  test_klp_callbacks_busy  busymod_work_func
test_klp_livepatch       vmlinux                  cmdline_proc_show

A few thoughts:

1. I don't think this patch handles the in-transition case. If I understand the associated kernel change, if the livepatch is currently transitioning, then it's stack order shouldn't be considered (otherwise this display would incorrectly imply that it's active).
2. Would similar information be useful even without the livepatch stack_order sysfs feature. By this I mean, it might be useful if patchset commit 1 introduced the display of the livepatched functions (regardless of which one is actually active). Then patchset commit 2 would take into considering the stack_order (if present) to note the currently active or inactive ones.
3. The answer to (2) would better phrase "The function enabling in the system" heading to something like "Currently livepatched functions". Likewise for the subcommand, "--enabling" would be "--functions".

Note that these aren't necessarily changes that I'm requesting, but I think a greater conversation around the display and feature would make sense first.

[wardenjohn]

@joe-lawrence Hi,joe!
For point 1: I do think transition patch should not be considered. Because it is still not exactly running now. Good point. I will fix it later.
For point 2: If there is just one patch in the system, the output is the function using now(even though we take stack_order into consider, there still no change to the output of this patch becase there is just one patch in system and no bigger stack_order will effect the output..Please see if I understand your suggestion clearly).
For point 3: If using 'functions' a little strange? How about "--enabling-functions"?

[joe-lawrence]

2 - The question is whether the list of livepatched functions (per loaded livepatch) is still useful to the user regardless of kernel support for stack order. That would be 99% the same code as reporting with stack order considered and offer potential value to any (older) kernel that didn't have such support.

3 - I find --enabling a little strange because it doesn't describe what it is that is doing the enabling, nor what this enabling entails. Given the output, it's implied that it means enabled livepatched functions. In which case, maybe --active-functions would be better. However if (2) were implemented, then it would be displaying both active and inactive functions, and hence my suggestion of --functions.

[wardenjohn]

@joe-lawrence
Hi, Joe.
For point 3, I support it. I will change this option to '--active-functions' or '--functions'.

However, for point 2, older kernel version which is not support this attribute can just output "unsupported"
information of this option?
Or maybe for a older version, we make a judge: if this system have only one livepatch module loaded, we can
output all the function enabling. If there are more than one livepatch module enabling, we can not show which
function is now enabling, because we can not support the correct information without 'stack_order' attribute.
At this point, we just need to output "unsupported".

Thank you
Wardenjohn.

[wardenjohn]

@joe-lawrence
I resubmit a newer version of this commit, which support the situation that only one klp module in the system, can output the active functions. If there are more than one klp moudles loaded in the system, we should not support it because the information is not accurate without 'stack_order' attribute.

Under older version: the output can be:

[root@iZwz9hvr38g8x8kvccszciZ kpatch]# kpatch list --active-functions
Loaded patch modules:
livepatch_fuse [enabled]
livepatch_vfio [enabled]

Installed patch modules:
This kernel don't support stack_order attribute, we don't support situation that more than one patch loaded.

If there are just only one module loaded:

[root@iZwz9hvr38g8x8kvccszciZ kpatch]# kpatch list --active-functions
Loaded patch modules:
livepatch_vfio [enabled]

Installed patch modules:

The function enabling in the system:
Module          Object            Function
livepatch_vfio  vfio_pci          vfio_pci_mmap
livepatch_vfio  vfio_iommu_type1  vfio_unpin_pages_remote
livepatch_vfio  vfio_iommu_type1  vfio_iommu_type1_ioctl
livepatch_vfio  vfio_iommu_type1  vfio_pin_map_dma
livepatch_vfio  vfio_iommu_type1  vfio_dma_do_map

[joe-lawrence]

Ok nobody else seems to have a UX opinion, so you're stuck with my thoughts, @wardenjohn 😆

In that case, let's keep it as simple as possible:

• Drop the command flag --active-functions and modify the code so that if it detects /sys/kernel/livepatch/*/stack_order, print the table. If stack_order is missing, no table and the output looks like it did before.
• Wording nitpick: s/The function enabling in the system/Currently livepatched functions/
• Finally, check the shellcheck complaints for the updated shell code

[wardenjohn]

@joe-lawrence
Bravo! It's good to hear from you and make this functions better! 👍

Ok, next version I will fix according to the suggestion you given.

I looked into the shellcheck complaints from github. It seems that it complaints because I don't use the variable declared in the shell code. If it is necessary, I will fix it the next version.

And I don't know when will Petr push the 'stack_order' patch to linux branch. So, I suggest this patch should be merged after the attribute is pushed into the linux kernel branch. 😄

Thanks!

[wardenjohn]

@joe-lawrence
Hi!Joe.

I resubmit an newer version of this function.
This version just use 'kpatch list' for all.

However, I separate this commit into to parts because the first commit is the real function commit.
The second commit is just to fix the complaints of shellcheck code.

The second patch fix the complaints of using 'find' instead of 'ls' and fix the complaint about unused variables.

This tow commits do the different things. So, I make them tow commits here.

Thanks.

[joe-lawrence]

When I suggested simpler, it was a reduced and slightly refactored version like:

show_enabled_function() {

	# Create a map that associates a [livepatched object, function
	# name, symbol position] with its [module, stack_order], i.e.
	#   (vmlinux,meminfo_proc_show,1) -> (test_klp_atomic_replace, 1)
	declare -A function_map
        for module_dir in "$SYSFS"/*; do
		if [[ ! -d "$module_dir" ]] || \
		   [[ ! -e "$module_dir/stack_order" ]]; then
			continue
		fi
		stack_order=$(cat "$module_dir/stack_order")
		module_name=$(basename "$module_dir")
		for obj_dir in "$module_dir"/*/; do
			obj_name=$(basename "$obj_dir")
			for func_dir in "$obj_dir"/*; do
				if [[ ! -d "$func_dir" ]]; then
					continue
				fi
				func_name_and_pos=$(basename "$func_dir")
				key="$obj_name:$func_name_and_pos"
				if [[ -z "${function_map[$key]}" ]]; then
					function_map[$key]="$stack_order:$module_name"
				else
					# Update the map only iff this livepatch has a
					# higher stack_order value
					IFS=':' read -r recorded_order <<< "${function_map[$key]}"
					if [[ $recorded_order -lt $stack_order ]]; then
						function_map[$key]="$stack_order:$module_name:$obj_name"
					fi
				fi
			done
		done

	done

	# Pretty print the function map if it has any contents
	if [[ ${#function_map[@]} -ne 0 ]]; then
		echo ""
		echo "Currently livepatched functions:"
		declare -a output_data=("Module Object Function/Occurrence")
		for key in "${!function_map[@]}"; do
			IFS=':' read -r stack_order module_name <<< "${function_map[$key]}"
			obj_name=${key%%:*}
			func_name_and_pos=${key##*:}
			output_data+=("$module_name $obj_name $func_name_and_pos")
		done
		printf "%s\n" "${output_data[@]}" | column -t
	fi
}

A few notes:

• The original indentation made it hard to read. Inverting the conditionals and taking early exits / continues helps keep it all within a few levels of indentation (typically what you see in the kernel).
• The function_map needs to consider the possibility of livepatches to two functions with the same name and in the same object file. The livepatching kernel code differentiates the two symbol by their position in the symbol table. This is why the /sys/kernel/livepatch/<module>/<object>/<function,pos> has the Nth occurrence tacked onto the end.
• If there is no stack_order sysfs variable, then don't add any extra output.

[wardenjohn]

@joe-lawrence
Hi, Joe.

I am quite curious about the 'pos'. I am a little confuse about point 2. If there is a same name in the same object file. It should be patched?

And for point 3, if system have no stack_order, what if there is only one livepatch module loaded? Maybe we can show the active function if there just one patch loaded because we can do it.

[joe-lawrence]

I am quite curious about the 'pos'. I am a little confuse about point 2. If there is a same name in the same object file. It should be patched?

Consider a patch to two functions of the same name that end up in the same livepatch target (vmlinux):

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 232026a239a6..b72c51f1a4b8 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -722,6 +722,7 @@ static ssize_t version_show(struct device *dev,
 {
        struct ucode_cpu_info *uci = ucode_cpu_info + dev->id;

+       nop();
        return sprintf(buf, "0x%x\n", uci->cpu_sig.rev);
 }

diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
index 257892fcefa7..a36fcece7344 100644
--- a/arch/x86/kernel/ksysfs.c
+++ b/arch/x86/kernel/ksysfs.c
@@ -22,6 +22,7 @@
 static ssize_t version_show(struct kobject *kobj,
                            struct kobj_attribute *attr, char *buf)
 {
+       nop();
        return sprintf(buf, "0x%04x\n", boot_params.hdr.version);
 }

It is represented in sysfs representation like so:

$ tree /sys/kernel/livepatch/livepatch_version_show/
/sys/kernel/livepatch/livepatch_version_show/
├── enabled
├── force
├── transition
└── vmlinux
    ├── patched
    ├── version_show,1    # corresponds to ksysfs.c's version_show()
    └── version_show,2    # corresponds to core.c's version_show()

The <function, sympos> gives us a way to differentiate otherwise homonym symbols like version_show. You can verify the sympos value by reading the object's symbol table and noting the ordering:

$ readelf --wide --symbols vmlinux | awk '$4 == "FILE" { f=$0 } $NF == "version_show" { print f; print $0 }'
  4772: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS ksysfs.c
  4776: ffffffff81036420    37 FUNC    LOCAL  DEFAULT    1 version_show
  7468: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS core.c
  7476: ffffffff81057d90    48 FUNC    LOCAL  DEFAULT    1 version_show
  ...

And for point 3, if system have no stack_order, what if there is only one livepatch module loaded? Maybe we can show the active function if there just one patch loaded because we can do it.

Since nobody else has commented on the PR, I would just go with whatever is the simplest to implement and review for now. Fancier behavior can be added if someone requests it (and let them code and test it :).

(From previous comments):

However, I separate this commit into to parts because the first commit is the real function commit. The second commit is just to fix the complaints of shellcheck code.

In future code reviews, I believe that:

1. Cleanup of existing code (already merged code) is fine to isolate to its own patch for clarity.
2. Cleanup of a potential commit in an open PR (ie, code under review), can be confusing for the reviewer as it introduces bad code that they may comment on before noticing the later cleanup. In general, it is best practice to ensure any make check or similar sanity check cleanups are squashed into the commits -- you are presenting a mostly finished idea and not a diary of first, second, etc. drafts.