diff --git a/doc/patch-author-guide.md b/doc/patch-author-guide.md index 45b54d5..33c1b27 100644 --- a/doc/patch-author-guide.md +++ b/doc/patch-author-guide.md @@ -774,3 +774,92 @@ excellent example and description of this problem with annotated disassembly. Adding `__attribute__((optimize("-fno-optimize-sibling-calls")))` instructs GCC to turn off the optimization for the given function. + +Exported symbol versioning +-------------------------- + +### Background + +`CONFIG_MODVERSIONS` enables an ABI check between exported kernel symbols and +modules referencing those symbols, enforced on module load. When building the +kernel, preprocessor output from `gcc -E` for each source file is passed to +scripts/genksyms. The genksyms script recursively expands each exported symbol +to its basic types. A hash is generated for each symbol as it traverses back up +the symbol tree. The end result is a CRC for each exported function in +the Module.symvers file and embedded in the vmlinux kernel object itself. + +A similar checksumming is performed when building modules: referenced exported +symbol CRCs are stored in the module’s `__versions` section (you can also find +these in plain-text intermediate \*.mod.c files.) + +When the kernel loads a module, the symbol CRCs found in its `__versions` are +compared to those of the kernel, if the two do not match, the kernel will refuse +to load it: +``` +: disagrees about version of symbol +: Unknown symbol (err -22) +``` + +### Kpatch detection + +After building the original and patched sources, kpatch-build compares the +newly calculated Module.symvers against the original. Discrepancies are +reported: + +``` +ERROR: Version disagreement for symbol +``` + +These reports should be addressed to ensure that the resulting kpatch module +can be loaded. + +#### False positives + +It is rare, but possible for a kpatch to introduce inadvertent symbol CRC +changes that are not true ABI changes. The following conditions must occur: + +1. The kpatch must modify the definition of an exported symbol. For example, + introducing a new header file may further define an opaque data type: + Before the kpatch, compilation unit U from the original kernel build only + knew about a `struct S` declaration (not its complete type). At the same + time, U contains function F, which has an interface that references S. If + the kpatch adds a header file to U that now fully defines `struct S { int + a, b, c; }`, its symbol type graph changes, CRCs generated for F are updated, + but its ABI remains consistent. + +2. The kpatch must introduce either a change or reference to F such that it is + included in the resulting kpatch module. This will force a `__version` + entry based on the new CRC. + + Note: if a kpatch doesn't change or reference F such that it is **not** + included in the resulting kpatch module, the new CRC value won't be added + to the module's `__version` table. However, if a future accumulative patch + does add a new change or reference to F, the new CRC will become a problem. + +#### Avoidance + +Kpatches should introduce new `#include` directives sparingly. Whenever +possible, extract the required definitions from header filers into kpatched +compilation units directly. + +If additional header files or symbol definitions cannot be avoided, consider +surrounding the offending include/definitions in an `#ifndef __GENKSYMS__` +macro. The genksyms script will skip over those blocks when performing its CRC +calculations. + +### But what about a real ABI change? + +If a kpatch introduces a true ABI change, each of calling functions would +consequently need to be updated in the kpatch module. For unexported functions, +this may be handled safely if the kpatch does indeed update all callers. +However, since motivation behind `CONFIG_MODVERSIONS` is to provide basic ABI +verification between the kernel and modules for **exported** functions, kpatch +cannot safely change this ABI without worrying about breaking other out-of-tree +drivers. Those drivers have been built against the reference kernel's original +set of CRCs and expect the original ABI. + +To track down specifically what caused a symbol CRC change, tools like +[kabi-dw](https://github.com/skozina/kabi-dw) can be employed to produce a +detailed symbol definition report. For a kpatch-build, kabi-dw can be modified +to operate on .o object files (not just .ko and vmlinux files) and the +`$CACHEDIR/tmp/{orig, patched}` directories compared. diff --git a/kpatch-build/kpatch-build b/kpatch-build/kpatch-build index e521c0e..0caf747 100755 --- a/kpatch-build/kpatch-build +++ b/kpatch-build/kpatch-build @@ -745,6 +745,7 @@ USE_KLP_ARCH=0 CONFIG_PARAVIRT=0 CONFIG_UNWINDER_ORC=0 CONFIG_JUMP_LABEL=0 +CONFIG_MODVERSIONS=0 if grep -q "CONFIG_LIVEPATCH=y" "$CONFIGFILE" && (kernel_is_rhel || kernel_version_gte 4.9.0); then @@ -770,6 +771,7 @@ fi grep -q "CONFIG_PARAVIRT=y" "$CONFIGFILE" && CONFIG_PARAVIRT=1 grep -q "CONFIG_UNWINDER_ORC=y" "$CONFIGFILE" && CONFIG_UNWINDER_ORC=1 grep -q "CONFIG_JUMP_LABEL=y" "$CONFIGFILE" && CONFIG_JUMP_LABEL=1 +grep -q "CONFIG_MODVERSIONS=y" "$CONFIGFILE" && CONFIG_MODVERSIONS=1 # unsupported kernel option checking grep -q "CONFIG_DEBUG_INFO_SPLIT=y" "$CONFIGFILE" && die "kernel option 'CONFIG_DEBUG_INFO_SPLIT' not supported" @@ -805,6 +807,9 @@ unset KPATCH_GCC_TEMPDIR # shellcheck disable=SC2086 CROSS_COMPILE="$TOOLSDIR/kpatch-gcc " make "-j$CPUS" $TARGETS 2>&1 | logger || die +# Save original module symvers +cp "$SRCDIR/Module.symvers" "$TEMPDIR/Module.symvers" + echo "Building patched source" apply_patches mkdir -p "$TEMPDIR/orig" "$TEMPDIR/patched" @@ -831,6 +836,29 @@ fi [[ -n "$OOT_MODULE" ]] || grep -q vmlinux "$SRCDIR/Module.symvers" || die "truncated $SRCDIR/Module.symvers file" +if [[ "$CONFIG_MODVERSIONS" -eq 1 ]]; then + while read -ra sym_line; do + if [[ ${#sym_line[@]} -lt 4 ]]; then + die "Malformed ${TEMPDIR}/Module.symvers file" + fi + + sym=${sym_line[1]} + + read -ra patched_sym_line <<< "$(grep "\s$sym\s" "$SRCDIR/Module.symvers")" + if [[ ${#patched_sym_line[@]} -lt 4 ]]; then + die "Malformed symbol entry for ${sym} in ${SRCDIR}/Module.symvers file" + fi + + # Assume that both original and patched symvers have the same format. + # In both cases, the symbol should have the same CRC, belong to the same + # Module/Namespace and have the same export type. + if [[ ${#sym_line[@]} -ne ${#patched_sym_line[@]} || \ + "${sym_line[*]}" != "${patched_sym_line[*]}" ]]; then + warn "Version disagreement for symbol ${sym}" + fi + done < "${TEMPDIR}/Module.symvers" +fi + # Read as words, no quotes. # shellcheck disable=SC2013 for i in $(cat "$TEMPDIR/changed_objs") @@ -868,6 +896,14 @@ mkdir output declare -a objnames CHANGED=0 ERROR=0 + +# Prepare OOT module symvers file +if [[ -n "$OOT_MODULE" ]]; then + BUILDDIR="/lib/modules/$ARCHVERSION/build/" + cp "$SRCDIR/Module.symvers" "$TEMPDIR/Module.symvers" + awk '{ print $1 "\t" $2 "\t" $3 "\t" $4}' "${BUILDDIR}/Module.symvers" >> "$TEMPDIR/Module.symvers" +fi + for i in $FILES; do # In RHEL 7 based kernels, copy_user_64.o misuses the .fixup section, # which confuses create-diff-object. It's fine to skip it, it's an @@ -892,9 +928,6 @@ for i in $FILES; do KOBJFILE_PATH="$OOT_MODULE" SYMTAB="${TEMPDIR}/module/${KOBJFILE_NAME}.symtab" SYMVERS_FILE="$TEMPDIR/Module.symvers" - BUILDDIR="/lib/modules/$ARCHVERSION/build/" - cp "$SRCDIR/Module.symvers" "$SYMVERS_FILE" - awk '{ print $1 "\t" $2 "\t" $3 "\t" $4}' "${BUILDDIR}/Module.symvers" >> "$SYMVERS_FILE" else KOBJFILE_NAME=$(basename "${KOBJFILE%.ko}") KOBJFILE_NAME="${KOBJFILE_NAME//-/_}" @@ -994,6 +1027,29 @@ if [[ "$USE_KLP" -eq 1 ]]; then check_pipe_status create-klp-module fi +if [[ "$CONFIG_MODVERSIONS" -eq 1 ]]; then + # Check that final module does not reference symbols with different version + # than the target kernel + KP_MOD_VALID=true + # shellcheck disable=SC2086 + while read -ra mod_symbol; do + if [[ ${#mod_symbol[@]} -lt 2 ]]; then + continue + fi + + # Check if the symbol exists in the old Module.symvers, and if it does + # check that the CRCs are unchanged. + if ! awk -v sym="${mod_symbol[1]}" -v crc="${mod_symbol[0]}" \ + '$2==sym && $1!=crc { exit 1 }' "$TEMPDIR/Module.symvers"; then + warn "Patch module references ${mod_symbol[1]} with invalid version" + KP_MOD_VALID=false + fi + done <<< "$(modprobe --dump-modversions $TEMPDIR/patch/$MODNAME.ko)" + if ! $KP_MOD_VALID; then + die "Patch module referencing altered exported kernel symbols cannot be loaded" + fi +fi + readelf --wide --symbols "$TEMPDIR/patch/$MODNAME.ko" 2>/dev/null | \ sed -r 's/\s+\[: 8\]//' | \ awk '($4=="FUNC" || $4=="OBJECT") && ($5=="GLOBAL" || $5=="WEAK") && $7!="UND" {print $NF}' \ diff --git a/test/integration/rhel-7.6/symvers-disagreement-FAIL.patch b/test/integration/rhel-7.6/symvers-disagreement-FAIL.patch new file mode 100644 index 0000000..d8ef754 --- /dev/null +++ b/test/integration/rhel-7.6/symvers-disagreement-FAIL.patch @@ -0,0 +1,51 @@ +From bcb86fa4e9c31379a9e2716eae29cd53ccca064f Mon Sep 17 00:00:00 2001 +From: Julien Thierry +Date: Wed, 6 May 2020 14:30:57 +0100 +Subject: [PATCH] Symbol version change + +This change causes: +1) Some exported symbols in drivers/base/core.c to see their CRCs + change. +2) Changes usb_get_dev() referencing a get_device() whose CRC has + changed, causing the symbol and the new CRC to be included in the + __version section of the final module. + +This makes the final module unloadable for the target kernel. + +See "Exported symbol versioning" of the patch author guide for more +detail. + +--- + drivers/base/core.c | 2 ++ + drivers/usb/core/usb.c | 2 ++ + 2 files changed, 4 insertions(+) + +diff --git a/drivers/base/core.c b/drivers/base/core.c +index b9a71137208..4af27e069c2 100644 +--- a/drivers/base/core.c ++++ b/drivers/base/core.c +@@ -31,6 +31,8 @@ + #include "base.h" + #include "power/power.h" + ++#include ++ + #ifdef CONFIG_SYSFS_DEPRECATED + #ifdef CONFIG_SYSFS_DEPRECATED_V2 + long sysfs_deprecated = 1; +diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c +index f9db3660999..bece0bf4f5a 100644 +--- a/drivers/usb/core/usb.c ++++ b/drivers/usb/core/usb.c +@@ -691,6 +691,8 @@ EXPORT_SYMBOL_GPL(usb_alloc_dev); + */ + struct usb_device *usb_get_dev(struct usb_device *dev) + { ++ barrier(); ++ + if (dev) + get_device(&dev->dev); + return dev; +-- +2.21.1 + diff --git a/test/integration/rhel-7.7/symvers-disagreement-FAIL.patch b/test/integration/rhel-7.7/symvers-disagreement-FAIL.patch new file mode 100644 index 0000000..e940645 --- /dev/null +++ b/test/integration/rhel-7.7/symvers-disagreement-FAIL.patch @@ -0,0 +1,51 @@ +From d5513eae5155c6e7e884554d5e3e2c65a7b39cbe Mon Sep 17 00:00:00 2001 +From: Julien Thierry +Date: Wed, 6 May 2020 14:30:57 +0100 +Subject: [PATCH] Symbol version change + +This change causes: +1) Some exported symbols in drivers/base/core.c to see their CRCs + change. +2) Changes usb_get_dev() referencing a get_device() whose CRC has + changed, causing the symbol and the new CRC to be included in the + __version section of the final module. + +This makes the final module unloadable for the target kernel. + +See "Exported symbol versioning" of the patch author guide for more +detail. + +--- + drivers/base/core.c | 2 ++ + drivers/usb/core/usb.c | 2 ++ + 2 files changed, 4 insertions(+) + +diff --git a/drivers/base/core.c b/drivers/base/core.c +index b9a71137208..4af27e069c2 100644 +--- a/drivers/base/core.c ++++ b/drivers/base/core.c +@@ -31,6 +31,8 @@ + #include "base.h" + #include "power/power.h" + ++#include ++ + #ifdef CONFIG_SYSFS_DEPRECATED + #ifdef CONFIG_SYSFS_DEPRECATED_V2 + long sysfs_deprecated = 1; +diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c +index d6337db2164..0ff9722dfa2 100644 +--- a/drivers/usb/core/usb.c ++++ b/drivers/usb/core/usb.c +@@ -693,6 +693,8 @@ EXPORT_SYMBOL_GPL(usb_alloc_dev); + */ + struct usb_device *usb_get_dev(struct usb_device *dev) + { ++ barrier(); ++ + if (dev) + get_device(&dev->dev); + return dev; +-- +2.21.3 + diff --git a/test/integration/rhel-7.8/symvers-disagreement-FAIL.patch b/test/integration/rhel-7.8/symvers-disagreement-FAIL.patch new file mode 100644 index 0000000..300f8b9 --- /dev/null +++ b/test/integration/rhel-7.8/symvers-disagreement-FAIL.patch @@ -0,0 +1,49 @@ +From da109d66a890373d0aa831f97b49aaffcc4eeb45 Mon Sep 17 00:00:00 2001 +From: Julien Thierry +Date: Wed, 6 May 2020 14:30:57 +0100 +Subject: [PATCH] Symbol version change + +This change causes: +1) Some exported symbols in drivers/base/core.c to see their CRCs + change. +2) Changes a function referencing a function whose CRC has changed, + causing the symbol and the new CRC to be included in the __version + section of the final module. + +See "Exported symbol versioning" of the patch author guide for more +detail. + +--- + drivers/base/core.c | 2 ++ + drivers/usb/core/usb.c | 2 ++ + 2 files changed, 4 insertions(+) + +diff --git a/drivers/base/core.c b/drivers/base/core.c +index b9a71137208..4af27e069c2 100644 +--- a/drivers/base/core.c ++++ b/drivers/base/core.c +@@ -31,6 +31,8 @@ + #include "base.h" + #include "power/power.h" + ++#include ++ + #ifdef CONFIG_SYSFS_DEPRECATED + #ifdef CONFIG_SYSFS_DEPRECATED_V2 + long sysfs_deprecated = 1; +diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c +index 8b1c4a5ee78..1b7997b58c0 100644 +--- a/drivers/usb/core/usb.c ++++ b/drivers/usb/core/usb.c +@@ -693,6 +693,8 @@ EXPORT_SYMBOL_GPL(usb_alloc_dev); + */ + struct usb_device *usb_get_dev(struct usb_device *dev) + { ++ barrier(); ++ + if (dev) + get_device(&dev->dev); + return dev; +-- +2.21.3 + diff --git a/test/integration/rhel-8.0/symvers-disagreement-FAIL.patch b/test/integration/rhel-8.0/symvers-disagreement-FAIL.patch new file mode 100644 index 0000000..e3a672a --- /dev/null +++ b/test/integration/rhel-8.0/symvers-disagreement-FAIL.patch @@ -0,0 +1,51 @@ +From 7085a655b8d665b6314e8dab2f803bac0aea04ec Mon Sep 17 00:00:00 2001 +From: Julien Thierry +Date: Wed, 6 May 2020 14:30:57 +0100 +Subject: [PATCH] Symbol version change + +This change causes: +1) Some exported symbols in drivers/base/core.c to see their CRCs + change. +2) Changes usb_get_dev() referencing a get_device() whose CRC has + changed, causing the symbol and the new CRC to be included in the + __version section of the final module. + +This makes the final module unloadable for the target kernel. + +See "Exported symbol versioning" of the patch author guide for more +detail. + +--- + drivers/base/core.c | 2 ++ + drivers/usb/core/usb.c | 2 ++ + 2 files changed, 4 insertions(+) + +diff --git a/drivers/base/core.c b/drivers/base/core.c +index df3e1a44707a..15c9d6e2e1e0 100644 +--- a/drivers/base/core.c ++++ b/drivers/base/core.c +@@ -29,6 +29,8 @@ + #include "base.h" + #include "power/power.h" + ++#include ++ + #ifdef CONFIG_SYSFS_DEPRECATED + #ifdef CONFIG_SYSFS_DEPRECATED_V2 + long sysfs_deprecated = 1; +diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c +index 623be3174fb3..4ddd74ae0bb9 100644 +--- a/drivers/usb/core/usb.c ++++ b/drivers/usb/core/usb.c +@@ -685,6 +685,8 @@ EXPORT_SYMBOL_GPL(usb_alloc_dev); + */ + struct usb_device *usb_get_dev(struct usb_device *dev) + { ++ barrier(); ++ + if (dev) + get_device(&dev->dev); + return dev; +-- +2.21.3 + diff --git a/test/integration/rhel-8.1/symvers-disagreement-FAIL.patch b/test/integration/rhel-8.1/symvers-disagreement-FAIL.patch new file mode 100644 index 0000000..9527641 --- /dev/null +++ b/test/integration/rhel-8.1/symvers-disagreement-FAIL.patch @@ -0,0 +1,51 @@ +From c63702554e54b992793fe3598ea8c8c415bef908 Mon Sep 17 00:00:00 2001 +From: Julien Thierry +Date: Wed, 6 May 2020 14:30:57 +0100 +Subject: [PATCH] Symbol version change + +This change causes: +1) Some exported symbols in drivers/base/core.c to see their CRCs + change. +2) Changes usb_get_dev() referencing a get_device() whose CRC has + changed, causing the symbol and the new CRC to be included in the + __version section of the final module. + +This makes the final module unloadable for the target kernel. + +See "Exported symbol versioning" of the patch author guide for more +detail. + +--- + drivers/base/core.c | 2 ++ + drivers/usb/core/usb.c | 2 ++ + 2 files changed, 4 insertions(+) + +diff --git a/drivers/base/core.c b/drivers/base/core.c +index 2ab316d85651..2ef19920f6ab 100644 +--- a/drivers/base/core.c ++++ b/drivers/base/core.c +@@ -29,6 +29,8 @@ + #include "base.h" + #include "power/power.h" + ++#include ++ + #ifdef CONFIG_SYSFS_DEPRECATED + #ifdef CONFIG_SYSFS_DEPRECATED_V2 + long sysfs_deprecated = 1; +diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c +index 0a2b261a27c9..51a1868c9cea 100644 +--- a/drivers/usb/core/usb.c ++++ b/drivers/usb/core/usb.c +@@ -685,6 +685,8 @@ EXPORT_SYMBOL_GPL(usb_alloc_dev); + */ + struct usb_device *usb_get_dev(struct usb_device *dev) + { ++ barrier(); ++ + if (dev) + get_device(&dev->dev); + return dev; +-- +2.21.3 + diff --git a/test/integration/rhel-8.2/symvers-disagreement-FAIL.patch b/test/integration/rhel-8.2/symvers-disagreement-FAIL.patch new file mode 100644 index 0000000..798d21f --- /dev/null +++ b/test/integration/rhel-8.2/symvers-disagreement-FAIL.patch @@ -0,0 +1,51 @@ +From 2d6b7bce089e52563bd9c67df62f48e90b48047d Mon Sep 17 00:00:00 2001 +From: Julien Thierry +Date: Wed, 6 May 2020 14:30:57 +0100 +Subject: [PATCH] Symbol version change + +This change causes: +1) Some exported symbols in drivers/base/core.c to see their CRCs + change. +2) Changes usb_get_dev() referencing a get_device() whose CRC has + changed, causing the symbol and the new CRC to be included in the + __version section of the final module. + +This makes the final module unloadable for the target kernel. + +See "Exported symbol versioning" of the patch author guide for more +detail. + +--- + drivers/base/core.c | 2 ++ + drivers/usb/core/usb.c | 2 ++ + 2 files changed, 4 insertions(+) + +diff --git a/drivers/base/core.c b/drivers/base/core.c +index 26bae20f0553..506ebbf0a210 100644 +--- a/drivers/base/core.c ++++ b/drivers/base/core.c +@@ -30,6 +30,8 @@ + #include "base.h" + #include "power/power.h" + ++#include ++ + #ifdef CONFIG_SYSFS_DEPRECATED + #ifdef CONFIG_SYSFS_DEPRECATED_V2 + long sysfs_deprecated = 1; +diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c +index f74e6bda1788..86f7d453549c 100644 +--- a/drivers/usb/core/usb.c ++++ b/drivers/usb/core/usb.c +@@ -685,6 +685,8 @@ EXPORT_SYMBOL_GPL(usb_alloc_dev); + */ + struct usb_device *usb_get_dev(struct usb_device *dev) + { ++ barrier(); ++ + if (dev) + get_device(&dev->dev); + return dev; +-- +2.21.3 +