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

wardenjohn · 2024-10-17T01:42:28Z

…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 · 2024-10-17T19:24:16Z

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:

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).
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.
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 · 2024-10-23T10:06:05Z

@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 · 2024-10-23T17:00:58Z

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 · 2024-10-25T01:49:12Z

@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 · 2024-10-31T06:49:24Z

@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 · 2024-11-05T15:01:05Z

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 · 2024-11-06T01:48:28Z

@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 · 2024-11-06T13:28:23Z

@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 · 2024-11-07T21:55:45Z

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 · 2024-11-12T14:29:38Z

@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 · 2024-11-12T17:20:01Z

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:

Cleanup of existing code (already merged code) is fine to isolate to its own patch for clarity.
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.

wardenjohn · 2024-12-04T02:22:07Z

@joe-lawrence
Hi Joe. I am sorry for I am quite busy that days.

I will soon follow up this MR after the patch of 'stack_order' is pushed to linux-6.14.

Thanks.
Wardenjohn,

wardenjohn · 2024-12-23T03:51:42Z

@joe-lawrence
Hi, Joe. Following your suggestion, I submmit a newer version of this commit.

I have cleanup of any potential commit in one open PR and in one commit right now.

When kpatch run in a kernel not supporting 'stack_order': the output is the original type without any change.

kpatch list
Loaded patch modules:
livepatch_0001_1 [enabled]

Installed patch modules:

And tested with the patched you provided before of different symbol pos. The output is like:

Loaded patch modules:
livepatch_0001_1 [enabled]
livepatch_0001_2 [enabled]
livepatch_0001_nop1 [enabled]
livepatch_0001_nop2 [enabled]

Installed patch modules:

Currently livepatched functions:
Module               Object   Function/Occurrence
livepatch_0001_1     vmlinux  meminfo_proc_show,1
livepatch_0001_nop1  vmlinux  version_show,2
livepatch_0001_nop2  vmlinux  version_show,1

This livepatch modules are built by REPLACE=0 for better testing result.

And yet, for the original suggestion, the Module column output with object name which is repeated in Object column. So, I remove the obj_name in Module column.

And again, sorry for answering late....:)

…ivepatch Add function of 'kpatch list' to adjust kernel new attribute 'stack_order' of livepatch of kernel v6.14 or later. Now, using 'kpatch list' can output the enabling function in the system and the relationship from the enabling function to its object and its related module. This feature only support the kernel with 'stack_order' attribute. Suggested-by: Joe Lawrence <[email protected]> Signed-off-by: Wardenjohn <[email protected]>

wardenjohn · 2025-01-08T01:37:51Z

@joe-lawrence Hi,Joe. How about this commit now? 😄😄

joe-lawrence

Hi @wardenjohn , thanks for sticking with this MR and updating as per the reviews.

I think the code is almost good to go (see one small bug and a few other nitpicks). Double check that the kpatch manual page matches the new usage. And then, the commit message could be revised for clarity, like:

kpatch: add currenly livepatched function report to 'kpatch list'

The upstream kernel introduced the livepatch 'stack_order' sysfs
attribute in v6.14. This value provides the order in which a live patch
module was loaded into the system. With 'stack_order', a user can
determine an active live patched version of a function when several
livepatches that modify the same function have been loaded.

For systems supporting 'stack_order', provide additional 'kpatch list'
information about the currently livepatched functions in the system
(livepatch module, target kernel object, and function/symbol-position).
By reporting the highest 'stack_order' for a given function, the user
can learn precisely which kpatch module(s) / function(s) are currently
active.

joe-lawrence · 2025-01-09T21:01:03Z

kpatch/kpatch

@@ -55,7 +55,7 @@ usage () {
 	echo >&2
 	usage_cmd "info <module>" "show information about a patch module"
 	echo >&2
-	usage_cmd "list" "list installed patch modules"
+	usage_cmd "list" "list installed patch modules, and list the enabling functions and its relationship from patch module to the function enabling in the system if 'stack_order' attribute is supported."


I'd simplify this to, "If the system supports the livepatch 'stack_order' sysfs attribute, provide the list of currently livepatched functions".

Also, this usage text change should be reflected in the manpage file: man/kpatch.1

joe-lawrence · 2025-01-09T21:08:57Z

kpatch/kpatch

+		fi
+		stack_order=$(cat "$module_dir/stack_order")
+		module_name=$(basename "$module_dir")
+		for obj_dir in "$module_dir"/*/; do


nit: all the other directory iterators do so on the base directory and then check that the entry is a directory itself (for example, $func_dir). Might as well make it consistent here.

joe-lawrence · 2025-01-09T22:30:00Z

kpatch/kpatch

+				else
+					# Update the map only iff this livepatch has a
+					# higher stack_order value
+					IFS=':' read -r recorded_order module_name <<< "${function_map[$key]}"


I think there is a subtle bug here, where module_name is read in from the function_map, but then a few lines later, the map is updated with $stack_order:$module_name:$obj_name so the net effect is that the map gets the previous (now lower stack_order) livepatch module name.

I think the simplest fix would be to rename this sole variable instance like:
IFS=':' read -r recorded_order recorded_module_name ... that way it's read and stashed in an unused temporary variable. The higher stack_order livepatch in question module_name is left intact and the map update should now be accurate.

Bravo! I didn't realize that there may have such problem. Thank you for pointing it out!

wardenjohn force-pushed the stack_order branch from a2f442f to e9b233d Compare October 31, 2024 06:44

wardenjohn force-pushed the stack_order branch from e9b233d to 50e4a0a Compare October 31, 2024 07:05

wardenjohn force-pushed the stack_order branch 3 times, most recently from cbf1ccf to 6f037b6 Compare November 6, 2024 13:24

wardenjohn force-pushed the stack_order branch from 6f037b6 to 6659f33 Compare December 23, 2024 03:42

wardenjohn force-pushed the stack_order branch from 6659f33 to 5e63479 Compare December 24, 2024 07:43

joe-lawrence requested changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wardenjohn commented Oct 17, 2024

joe-lawrence commented Oct 17, 2024

wardenjohn commented Oct 23, 2024

joe-lawrence commented Oct 23, 2024

wardenjohn commented Oct 25, 2024 •

edited

Loading

wardenjohn commented Oct 31, 2024 •

edited

Loading

joe-lawrence commented Nov 5, 2024

wardenjohn commented Nov 6, 2024 •

edited

Loading

wardenjohn commented Nov 6, 2024 •

edited

Loading

joe-lawrence commented Nov 7, 2024

wardenjohn commented Nov 12, 2024

joe-lawrence commented Nov 12, 2024

wardenjohn commented Dec 4, 2024

wardenjohn commented Dec 23, 2024 •

edited

Loading

wardenjohn commented Jan 8, 2025

joe-lawrence left a comment

joe-lawrence Jan 9, 2025

joe-lawrence Jan 9, 2025

joe-lawrence Jan 9, 2025

joe-lawrence Jan 9, 2025

wardenjohn Jan 10, 2025

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

Are you sure you want to change the base?

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

Conversation

wardenjohn commented Oct 17, 2024

joe-lawrence commented Oct 17, 2024

wardenjohn commented Oct 23, 2024

joe-lawrence commented Oct 23, 2024

wardenjohn commented Oct 25, 2024 • edited Loading

wardenjohn commented Oct 31, 2024 • edited Loading

joe-lawrence commented Nov 5, 2024

wardenjohn commented Nov 6, 2024 • edited Loading

wardenjohn commented Nov 6, 2024 • edited Loading

joe-lawrence commented Nov 7, 2024

wardenjohn commented Nov 12, 2024

joe-lawrence commented Nov 12, 2024

wardenjohn commented Dec 4, 2024

wardenjohn commented Dec 23, 2024 • edited Loading

wardenjohn commented Jan 8, 2025

joe-lawrence left a comment

Choose a reason for hiding this comment

joe-lawrence Jan 9, 2025

Choose a reason for hiding this comment

joe-lawrence Jan 9, 2025

Choose a reason for hiding this comment

joe-lawrence Jan 9, 2025

Choose a reason for hiding this comment

joe-lawrence Jan 9, 2025

Choose a reason for hiding this comment

wardenjohn Jan 10, 2025

Choose a reason for hiding this comment

wardenjohn commented Oct 25, 2024 •

edited

Loading

wardenjohn commented Oct 31, 2024 •

edited

Loading

wardenjohn commented Nov 6, 2024 •

edited

Loading

wardenjohn commented Nov 6, 2024 •

edited

Loading

wardenjohn commented Dec 23, 2024 •

edited

Loading