CONFIG_TRIM_UNUSED_KSYMS currently removes all unused exported symbols
from ksymtab. This works really well when using in-tree drivers, but
cannot be used in its current form if some of them are out-of-tree.
Indeed, even if the list of symbols required by out-of-tree drivers is
known at compile time, the only solution today to guarantee these don't
get trimmed is to set CONFIG_TRIM_UNUSED_KSYMS=n. This not only wastes
space, but also makes it difficult to control the ABI usable by vendor
modules in distribution kernels such as Android. Being able to control
the kernel ABI surface is particularly useful to ship a unique Generic
Kernel Image (GKI) for all vendors.
As such, attempt to improve the situation by enabling users to specify a
symbol 'whitelist' at compile time. Any symbol specified in this
whitelist will be kept exported when CONFIG_TRIM_UNUSED_KSYMS is set,
even if it has no in-tree user. The whitelist is defined as a simple
text file, listing symbols, one per line.
Signed-off-by: Quentin Perret <[email protected]>
---
Not sure if this was relevant for the commit message so I'll put it
here: more context about the GKI effort in Android can found in these
talk at LPC2018 [1] and LPC2019 [2].
[1] https://linuxplumbersconf.org/event/2/contributions/61/
[2] https://linuxplumbersconf.org/event/4/contributions/401/
---
init/Kconfig | 12 ++++++++++++
scripts/adjust_autoksyms.sh | 1 +
2 files changed, 13 insertions(+)
diff --git a/init/Kconfig b/init/Kconfig
index a34064a031a5..d9c977ef7de5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2180,6 +2180,18 @@ config TRIM_UNUSED_KSYMS
If unsure, or if you need to build out-of-tree modules, say N.
+config UNUSED_KSYMS_WHITELIST
+ string "Whitelist of symbols to keep in ksymtab"
+ depends on TRIM_UNUSED_KSYMS
+ help
+ By default, all unused exported symbols will be trimmed from the
+ build when TRIM_UNUSED_KSYMS is selected.
+
+ UNUSED_KSYMS_WHITELIST allows to whitelist symbols that must be kept
+ exported at all times, even in absence of in-tree users. The value to
+ set here is the path to a text file containing the list of symbols,
+ one per line.
+
endif # MODULES
config MODULES_TREE_LOOKUP
diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
index a904bf1f5e67..1a6f7f377230 100755
--- a/scripts/adjust_autoksyms.sh
+++ b/scripts/adjust_autoksyms.sh
@@ -48,6 +48,7 @@ cat > "$new_ksyms_file" << EOT
EOT
sed 's/ko$/mod/' modules.order |
xargs -n1 sed -n -e '2{s/ /\n/g;/^$/!p;}' -- |
+cat - $CONFIG_UNUSED_KSYMS_WHITELIST |
sort -u |
sed -e 's/\(.*\)/#define __KSYM_\1 1/' >> "$new_ksyms_file"
--
2.25.0.341.g760bfbb309-goog
On Wed, 29 Jan 2020, Quentin Perret wrote:
> CONFIG_TRIM_UNUSED_KSYMS currently removes all unused exported symbols
> from ksymtab. This works really well when using in-tree drivers, but
> cannot be used in its current form if some of them are out-of-tree.
>
> Indeed, even if the list of symbols required by out-of-tree drivers is
> known at compile time, the only solution today to guarantee these don't
> get trimmed is to set CONFIG_TRIM_UNUSED_KSYMS=n. This not only wastes
> space, but also makes it difficult to control the ABI usable by vendor
> modules in distribution kernels such as Android. Being able to control
> the kernel ABI surface is particularly useful to ship a unique Generic
> Kernel Image (GKI) for all vendors.
>
> As such, attempt to improve the situation by enabling users to specify a
> symbol 'whitelist' at compile time. Any symbol specified in this
> whitelist will be kept exported when CONFIG_TRIM_UNUSED_KSYMS is set,
> even if it has no in-tree user. The whitelist is defined as a simple
> text file, listing symbols, one per line.
The idea is sound to me. But...
> diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> index a904bf1f5e67..1a6f7f377230 100755
> --- a/scripts/adjust_autoksyms.sh
> +++ b/scripts/adjust_autoksyms.sh
> @@ -48,6 +48,7 @@ cat > "$new_ksyms_file" << EOT
> EOT
> sed 's/ko$/mod/' modules.order |
> xargs -n1 sed -n -e '2{s/ /\n/g;/^$/!p;}' -- |
> +cat - $CONFIG_UNUSED_KSYMS_WHITELIST |
This is a nice trick, however it'll fail if the file path contains
spaces or other shell special characters. You could try something like
this:
[ -z "$CONFIG_UNUSED_KSYMS_WHITELIST" ] \
&& whitelist= \
|| whitelist="\"$CONFIG_UNUSED_KSYMS_WHITELIST\""
And then...
eval cat - $whitelist | ...
This way, if $CONFIG_UNUSED_KSYMS_WHITELIST is non empty, it'll get
quoted.
Nicolas
On Wed, 29 Jan 2020, Quentin Perret wrote:
> On Wednesday 29 Jan 2020 at 12:11:12 (-0500), Nicolas Pitre wrote:
> > This way, if $CONFIG_UNUSED_KSYMS_WHITELIST is non empty, it'll get
> > quoted.
>
> A shorter alternative would be something a little like so perhaps ?
>
> diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> index 1a6f7f377230..8e1b7f70e800 100755
> --- a/scripts/adjust_autoksyms.sh
> +++ b/scripts/adjust_autoksyms.sh
> @@ -48,7 +48,7 @@ cat > "$new_ksyms_file" << EOT
> EOT
> sed 's/ko$/mod/' modules.order |
> xargs -n1 sed -n -e '2{s/ /\n/g;/^$/!p;}' -- |
> -cat - $CONFIG_UNUSED_KSYMS_WHITELIST |
> +cat - "${CONFIG_UNUSED_KSYMS_WHITELIST:-/dev/null}" |
That's even better.
Nicolas
On Wednesday 29 Jan 2020 at 12:11:12 (-0500), Nicolas Pitre wrote:
> > diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> > index a904bf1f5e67..1a6f7f377230 100755
> > --- a/scripts/adjust_autoksyms.sh
> > +++ b/scripts/adjust_autoksyms.sh
> > @@ -48,6 +48,7 @@ cat > "$new_ksyms_file" << EOT
> > EOT
> > sed 's/ko$/mod/' modules.order |
> > xargs -n1 sed -n -e '2{s/ /\n/g;/^$/!p;}' -- |
> > +cat - $CONFIG_UNUSED_KSYMS_WHITELIST |
>
> This is a nice trick, however it'll fail if the file path contains
> spaces or other shell special characters.
Argh! Right, that's a very good point.
> You could try something like
> this:
>
> [ -z "$CONFIG_UNUSED_KSYMS_WHITELIST" ] \
> && whitelist= \
> || whitelist="\"$CONFIG_UNUSED_KSYMS_WHITELIST\""
>
> And then...
>
> eval cat - $whitelist | ...
>
> This way, if $CONFIG_UNUSED_KSYMS_WHITELIST is non empty, it'll get
> quoted.
A shorter alternative would be something a little like so perhaps ?
diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
index 1a6f7f377230..8e1b7f70e800 100755
--- a/scripts/adjust_autoksyms.sh
+++ b/scripts/adjust_autoksyms.sh
@@ -48,7 +48,7 @@ cat > "$new_ksyms_file" << EOT
EOT
sed 's/ko$/mod/' modules.order |
xargs -n1 sed -n -e '2{s/ /\n/g;/^$/!p;}' -- |
-cat - $CONFIG_UNUSED_KSYMS_WHITELIST |
+cat - "${CONFIG_UNUSED_KSYMS_WHITELIST:-/dev/null}" |
sort -u |
sed -e 's/\(.*\)/#define __KSYM_\1 1/' >> "$new_ksyms_file"
No strong opinion, though.
Thanks for the review,
Quentin
On Wed, Jan 29, 2020 at 03:06:12PM +0000, Quentin Perret wrote:
> CONFIG_TRIM_UNUSED_KSYMS currently removes all unused exported symbols
> from ksymtab. This works really well when using in-tree drivers, but
> cannot be used in its current form if some of them are out-of-tree.
NAK. The state policy is that we don't care for out of tree modules,
and this is useful for nothing but. In fact we should remove the
CONFIG_TRIM_UNUSED_KSYMS=n option soon.
On Thursday 30 Jan 2020 at 07:45:30 (-0800), Christoph Hellwig wrote:
> On Wed, Jan 29, 2020 at 03:06:12PM +0000, Quentin Perret wrote:
> > CONFIG_TRIM_UNUSED_KSYMS currently removes all unused exported symbols
> > from ksymtab. This works really well when using in-tree drivers, but
> > cannot be used in its current form if some of them are out-of-tree.
>
> NAK. The state policy is that we don't care for out of tree modules,
> and this is useful for nothing but.
That is correct. Now, I clearly failed to explain this patch properly,
but the long term goal here _is_ to get vendors to contribute more code
upstream, and have a lot less code downstream / out-of-tree. And I hope
we can agree this would be a good thing. So let me try again, and sorry
if I missed the mark the first time.
As you probably know, the current norm on Android and many other systems
is for vendors to introduce significant changes to their downstream
kernels, and to contribute very little (if any) code back upstream.
One of the goals of the Generic Kernel Image (GKI) project together with
updatability and such is to close the gap between vendor-specific
downstream kernels and upstream. Having one unique kernel for all android
devices of the same architecture regardless of the vendor will
mechanically force all interested parties to agree on a common solution.
And we _are_ pushing for all this to reach upstream and be available to
use by the wider community.
The kernel-to-drivers ABI on Android devices varies significantly from
one vendor kernel to another today (because of changes to exported
symbols, dependencies on vendor symbols, and surely other things). The
first step for GKI is to try and put some order into this by agreeing on
one version of the ABI that works for everybody.
For practical reasons, we need to reduce the ABI surface to a subset of
the exported symbols, simply to make the problem realistically solvable.
Hence this patch.
I understand your point of view, and quite frankly agree with the
message. But I think this patch pushes in the right direction. As I see
it, things like GKI really are significant improvements, so preventing
them from happening by refusing trivial patches such as this one will,
in the long term, do more harm than good to the cause.
Thank you for your time,
Quentin