Merge pull request #1114 from julien-thierry/fix-symvers

Fix symvers
This commit is contained in:
Joe Lawrence 2020-08-24 09:31:15 -04:00 committed by GitHub
commit cd2c3ede75
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 452 additions and 3 deletions

View File

@ -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 modules `__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:
```
<module>: disagrees about version of symbol <symbol>
<module>: Unknown symbol <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 <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.

View File

@ -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+\[<localentry>: 8\]//' | \
awk '($4=="FUNC" || $4=="OBJECT") && ($5=="GLOBAL" || $5=="WEAK") && $7!="UND" {print $NF}' \

View File

@ -0,0 +1,51 @@
From bcb86fa4e9c31379a9e2716eae29cd53ccca064f Mon Sep 17 00:00:00 2001
From: Julien Thierry <jthierry@redhat.com>
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 <linux/blktrace_api.h>
+
#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

View File

@ -0,0 +1,51 @@
From d5513eae5155c6e7e884554d5e3e2c65a7b39cbe Mon Sep 17 00:00:00 2001
From: Julien Thierry <jthierry@redhat.com>
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 <linux/blktrace_api.h>
+
#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

View File

@ -0,0 +1,49 @@
From da109d66a890373d0aa831f97b49aaffcc4eeb45 Mon Sep 17 00:00:00 2001
From: Julien Thierry <jthierry@redhat.com>
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 <linux/blktrace_api.h>
+
#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

View File

@ -0,0 +1,51 @@
From 7085a655b8d665b6314e8dab2f803bac0aea04ec Mon Sep 17 00:00:00 2001
From: Julien Thierry <jthierry@redhat.com>
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 <linux/blktrace_api.h>
+
#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

View File

@ -0,0 +1,51 @@
From c63702554e54b992793fe3598ea8c8c415bef908 Mon Sep 17 00:00:00 2001
From: Julien Thierry <jthierry@redhat.com>
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 <linux/blktrace_api.h>
+
#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

View File

@ -0,0 +1,51 @@
From 2d6b7bce089e52563bd9c67df62f48e90b48047d Mon Sep 17 00:00:00 2001
From: Julien Thierry <jthierry@redhat.com>
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 <linux/blktrace_api.h>
+
#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