[PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Joel Fernandes (Google)]

Introduce in-kernel headers which are made available as an archive
through proc (/proc/kheaders.tar.xz file). This archive makes it
possible to run eBPF and other tracing programs tracing programs that
need to extend the kernel for tracing purposes without any dependency on
the file system having headers.

On Android and embedded systems, it is common to switch kernels but not
have kernel headers available on the file system. Further once a
different kernel is booted, any headers stored on the file system will
no longer be useful. By storing the headers as a compressed archive
within the kernel, we can avoid these issues that have been a hindrance
for a long time.

The best way to use this feature is by building it in. Several users
have a need for this, when they switch debug kernels, they donot want to
update the filesystem or worry about it where to store the headers on
it. However, the feature is also buildable as a module in case the user
desires it not being part of the kernel image. This makes it possible to
load and unload the headers from memory on demand. A tracing program, or
a kernel module builder can load the module, do its operations, and then
unload the module to save kernel memory. The total memory needed is 3.8MB.

By having the archive available at a fixed location independent of
filesystem dependencies and conventions, all debugging tools can
directly refer to the fixed location for the archive, without concerning
with where the headers on a typical filesystem which significantly
simplifies tooling that needs kernel headers.

The code to read the headers is based on /proc/config.gz code and uses
the same technique to embed the headers.

IKHD_ST and IKHD_ED markers as is to facilitate future patches that
would extract the headers from a kernel or module image.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---

    v5 -> v6: (Masahiro Yamada suggestions mostly)
    - Dropped support for module building.
    - Rebuild archive if script changes.
    - Move archive file list to script.
    - Move build script to kernel directory.

    v4 -> v5:
    (v4 was Tested-by the following folks)
        Tested-by: qais.yousef@arm.com
        Tested-by: dietmar.eggemann@arm.com
        Tested-by: linux@manojrajarao.com
    (Thanks to Masahiro Yamada for several excellent suggestions)
    - used incbin instead of bin2c (Masahiro did similar idea)
    - added module.lds if ia64 otherwise ia64 may fail to build.
    - added clean-files rule to Makefile
    - removed strip-comments script and doing it inline
    - added set -e to header generated to die on errorsr
    - fixed a minor issue where find command was noisy.
    - removed unneeded tar.xz rule from kernel/.gitignore
    - added Tested-by tags from ARM folks.

    Changes since v3:
    - Blank tar was being generated because of a one line I
      forgot to push. It is updated now.
    - Added module.lds since arm64 needs it to build modules.

    Changes since v2:
    (Thanks to Masahiro Yamada for several excellent suggestions)
    - Added support for out of tree builds.
    - Added incremental build support bringing down build time of
      incremental builds from 50 seconds to 5 seconds.
    - Fixed various small nits / cleanups.
    - clean ups to kheaders.c pointed by Alexey Dobriyan.
    - Fixed MODULE_LICENSE in test module and kheaders.c
    - Dropped Module.symvers from archive due to circular dependency.

    Changes since v1:
    - removed IKH_EXTRA variable, not needed (Masahiro Yamada)
    - small fix ups to selftest
       - added target to main Makefile etc
       - added MODULE_LICENSE to test module
       - made selftest more quiet

    Changes since RFC:
    Both changes bring size down to 3.8MB:
    - use xz for compression
    - strip comments except SPDX lines
    - Call out the module name in Kconfig
    - Also added selftests in second patch to ensure headers are always
    working.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

 init/Kconfig           | 11 ++++++
 kernel/.gitignore      |  1 +
 kernel/Makefile        | 10 +++++
 kernel/gen_ikh_data.sh | 84 ++++++++++++++++++++++++++++++++++++++++++
 kernel/kheaders.c      | 73 ++++++++++++++++++++++++++++++++++++
 5 files changed, 179 insertions(+)
 create mode 100755 kernel/gen_ikh_data.sh
 create mode 100644 kernel/kheaders.c

diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..ea75bfbf7dfa 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -580,6 +580,17 @@ config IKCONFIG_PROC
 	  This option enables access to the kernel configuration file
 	  through /proc/config.gz.
 
+config IKHEADERS_PROC
+	tristate "Enable kernel header artifacts through /proc/kheaders.tar.xz"
+	depends on PROC_FS
+	help
+	  This option enables access to the kernel header and other artifacts that
+          are generated during the build process. These can be used to build kernel
+          modules or by other in-kernel programs such as those generated by eBPF
+          and systemtap tools. If you build the headers as a module, a module
+          called kheaders.ko is built which can be loaded on-demand to get access
+          to the headers.
+
 config LOG_BUF_SHIFT
 	int "Kernel log buffer size (16 => 64KB, 17 => 128KB)"
 	range 12 25
diff --git a/kernel/.gitignore b/kernel/.gitignore
index 6e699100872f..34d1e77ee9df 100644
--- a/kernel/.gitignore
+++ b/kernel/.gitignore
@@ -1,5 +1,6 @@
 #
 # Generated files
 #
+kheaders.md5
 timeconst.h
 hz.bc
diff --git a/kernel/Makefile b/kernel/Makefile
index 6c57e78817da..e3c581d8cde7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_USER_NS) += user_namespace.o
 obj-$(CONFIG_PID_NS) += pid_namespace.o
 obj-$(CONFIG_IKCONFIG) += configs.o
+obj-$(CONFIG_IKHEADERS_PROC) += kheaders.o
 obj-$(CONFIG_SMP) += stop_machine.o
 obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
 obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
@@ -121,3 +122,12 @@ $(obj)/configs.o: $(obj)/config_data.gz
 targets += config_data.gz
 $(obj)/config_data.gz: $(KCONFIG_CONFIG) FORCE
 	$(call if_changed,gzip)
+
+$(obj)/kheaders.o: $(obj)/kheaders_data.tar.xz
+
+quiet_cmd_genikh = GEN     $(obj)/kheaders_data.tar.xz
+cmd_genikh = $(srctree)/kernel/gen_ikh_data.sh $@
+$(obj)/kheaders_data.tar.xz: FORCE
+	$(call cmd,genikh)
+
+clean-files := kheaders_data.tar.xz kheaders.md5
diff --git a/kernel/gen_ikh_data.sh b/kernel/gen_ikh_data.sh
new file mode 100755
index 000000000000..ef72c2740d01
--- /dev/null
+++ b/kernel/gen_ikh_data.sh
@@ -0,0 +1,84 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This script generates an archive consisting of kernel headers
+# for CONFIG_IKHEADERS_PROC.
+set -e
+spath="$(dirname "$(readlink -f "$0")")"
+kroot="$spath/.."
+outdir="$(pwd)"
+tarfile=$1
+cpio_dir=$outdir/$tarfile.tmp
+
+# Script filename relative to the kernel source root
+# We add it to the archive because it is small and any changes
+# to this script will also cause a rebuild of the archive.
+sfile="$(realpath --relative-to $kroot "$(readlink -f "$0")")"
+
+src_file_list="
+include/
+arch/$SRCARCH/include/
+$sfile
+"
+
+obj_file_list="
+include/
+arch/$SRCARCH/include/
+"
+
+# Support incremental builds by skipping archive generation
+# if timestamps of files being archived are not changed.
+
+# This block is useful for debugging the incremental builds.
+# Uncomment it for debugging.
+# iter=1
+# if [ ! -f /tmp/iter ]; then echo 1 > /tmp/iter;
+# else; 	iter=$(($(cat /tmp/iter) + 1)); fi
+# find $src_file_list -type f | xargs ls -lR > /tmp/src-ls-$iter
+# find $obj_file_list -type f | xargs ls -lR > /tmp/obj-ls-$iter
+
+# include/generated/compile.h is ignored because it is touched even when none
+# of the source files changed. This causes pointless regeneration, so let us
+# ignore them for md5 calculation.
+pushd $kroot > /dev/null
+src_files_md5="$(find $src_file_list -type f                       |
+		grep -v "include/generated/compile.h"		   |
+		xargs ls -lR | md5sum | cut -d ' ' -f1)"
+popd > /dev/null
+obj_files_md5="$(find $obj_file_list -type f                       |
+		grep -v "include/generated/compile.h"		   |
+		xargs ls -lR | md5sum | cut -d ' ' -f1)"
+
+if [ -f $tarfile ]; then tarfile_md5="$(md5sum $tarfile | cut -d ' ' -f1)"; fi
+if [ -f kernel/kheaders.md5 ] &&
+	[ "$(cat kernel/kheaders.md5|head -1)" == "$src_files_md5" ] &&
+	[ "$(cat kernel/kheaders.md5|head -2|tail -1)" == "$obj_files_md5" ] &&
+	[ "$(cat kernel/kheaders.md5|tail -1)" == "$tarfile_md5" ]; then
+		exit
+fi
+
+rm -rf $cpio_dir
+mkdir $cpio_dir
+
+pushd $kroot > /dev/null
+for f in $src_file_list;
+	do find "$f" ! -name "*.cmd" ! -name ".*";
+done | cpio --quiet -pd $cpio_dir
+popd > /dev/null
+
+# The second CPIO can complain if files already exist which can
+# happen with out of tree builds. Just silence CPIO for now.
+for f in $obj_file_list;
+	do find "$f" ! -name "*.cmd" ! -name ".*";
+done | cpio --quiet -pd $cpio_dir >/dev/null 2>&1
+
+find  $cpio_dir -type f -print0 |
+	xargs -0 -P8 -n1 perl -pi -e 'BEGIN {undef $/;}; s/\/\*((?!SPDX).)*?\*\///smg;'
+
+tar -Jcf $tarfile -C $cpio_dir/ . > /dev/null
+
+echo "$src_files_md5" > kernel/kheaders.md5
+echo "$obj_files_md5" >> kernel/kheaders.md5
+echo "$(md5sum $tarfile | cut -d ' ' -f1)" >> kernel/kheaders.md5
+
+rm -rf $cpio_dir
diff --git a/kernel/kheaders.c b/kernel/kheaders.c
new file mode 100644
index 000000000000..d072a958a8f1
--- /dev/null
+++ b/kernel/kheaders.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * kernel/kheaders.c
+ * Provide headers and artifacts needed to build kernel modules.
+ * (Borrowed code from kernel/configs.c)
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/proc_fs.h>
+#include <linux/init.h>
+#include <linux/uaccess.h>
+
+/*
+ * Define kernel_headers_data and kernel_headers_data_end, within which the the
+ * compressed kernel headers are stpred. The file is first compressed with xz.
+ */
+
+asm (
+"	.pushsection .rodata, \"a\"		\n"
+"	.global kernel_headers_data		\n"
+"kernel_headers_data:				\n"
+"	.incbin \"kernel/kheaders_data.tar.xz\"	\n"
+"	.global kernel_headers_data_end		\n"
+"kernel_headers_data_end:			\n"
+"	.popsection				\n"
+);
+
+extern char kernel_headers_data;
+extern char kernel_headers_data_end;
+
+static ssize_t
+ikheaders_read_current(struct file *file, char __user *buf,
+		      size_t len, loff_t *offset)
+{
+	return simple_read_from_buffer(buf, len, offset,
+				       &kernel_headers_data,
+				       &kernel_headers_data_end -
+				       &kernel_headers_data);
+}
+
+static const struct file_operations ikheaders_file_ops = {
+	.read = ikheaders_read_current,
+	.llseek = default_llseek,
+};
+
+static int __init ikheaders_init(void)
+{
+	struct proc_dir_entry *entry;
+
+	/* create the current headers file */
+	entry = proc_create("kheaders.tar.xz", S_IRUGO, NULL,
+			    &ikheaders_file_ops);
+	if (!entry)
+		return -ENOMEM;
+
+	proc_set_size(entry,
+		      &kernel_headers_data_end -
+		      &kernel_headers_data);
+	return 0;
+}
+
+static void __exit ikheaders_cleanup(void)
+{
+	remove_proc_entry("kheaders.tar.xz", NULL);
+}
+
+module_init(ikheaders_init);
+module_exit(ikheaders_cleanup);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Joel Fernandes");
+MODULE_DESCRIPTION("Echo the kernel header artifacts used to build the kernel");
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v6 2/2] init/config: Do not select BUILD_BIN2C for IKCONFIG

[Joel Fernandes (Google)]

Since commit 13610aa908dc ("kernel/configs: use .incbin directive to
embed config_data.gz"), IKCONFIG no longer uses BUILD_BIN2C so prevent
it from being selected in Kconfig.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 init/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/init/Kconfig b/init/Kconfig
index ea75bfbf7dfa..ffcb2f90543f 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -562,7 +562,6 @@ config BUILD_BIN2C
 
 config IKCONFIG
 	tristate "Kernel .config support"
-	select BUILD_BIN2C
 	---help---
 	  This option enables the complete Linux kernel ".config" file
 	  contents to be saved in the kernel. It provides documentation
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Qais Yousef]

On 04/08/19 17:28, Joel Fernandes (Google) wrote:
> Introduce in-kernel headers which are made available as an archive
> through proc (/proc/kheaders.tar.xz file). This archive makes it
> possible to run eBPF and other tracing programs tracing programs that
> need to extend the kernel for tracing purposes without any dependency on
> the file system having headers.
> 
> On Android and embedded systems, it is common to switch kernels but not
> have kernel headers available on the file system. Further once a
> different kernel is booted, any headers stored on the file system will
> no longer be useful. By storing the headers as a compressed archive
> within the kernel, we can avoid these issues that have been a hindrance
> for a long time.
> 
> The best way to use this feature is by building it in. Several users
> have a need for this, when they switch debug kernels, they donot want to
> update the filesystem or worry about it where to store the headers on
> it. However, the feature is also buildable as a module in case the user
> desires it not being part of the kernel image. This makes it possible to
> load and unload the headers from memory on demand. A tracing program, or
> a kernel module builder can load the module, do its operations, and then
> unload the module to save kernel memory. The total memory needed is 3.8MB.
> 
> By having the archive available at a fixed location independent of
> filesystem dependencies and conventions, all debugging tools can
> directly refer to the fixed location for the archive, without concerning
> with where the headers on a typical filesystem which significantly
> simplifies tooling that needs kernel headers.
> 
> The code to read the headers is based on /proc/config.gz code and uses
> the same technique to embed the headers.
> 
> IKHD_ST and IKHD_ED markers as is to facilitate future patches that
> would extract the headers from a kernel or module image.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

I applied both patches on 5.1-rc2 and managed to compile and run several
eBPF programs using the untarred headers from /proc/kheaders.tar.xz on my juno.

Tested-by: Qais Yousef <qais.yousef@arm.com>

> ---
> 
>     v5 -> v6: (Masahiro Yamada suggestions mostly)
>     - Dropped support for module building.
>     - Rebuild archive if script changes.
>     - Move archive file list to script.
>     - Move build script to kernel directory.
> 
>     v4 -> v5:
>     (v4 was Tested-by the following folks)
>         Tested-by: qais.yousef@arm.com
>         Tested-by: dietmar.eggemann@arm.com
>         Tested-by: linux@manojrajarao.com

Thanks!

--
Qais Yousef

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Joel Fernandes]

On Tue, Apr 09, 2019 at 04:00:34PM +0100, Qais Yousef wrote:
> On 04/08/19 17:28, Joel Fernandes (Google) wrote:
> > Introduce in-kernel headers which are made available as an archive
> > through proc (/proc/kheaders.tar.xz file). This archive makes it
> > possible to run eBPF and other tracing programs tracing programs that
> > need to extend the kernel for tracing purposes without any dependency on
> > the file system having headers.
> > 
> > On Android and embedded systems, it is common to switch kernels but not
> > have kernel headers available on the file system. Further once a
> > different kernel is booted, any headers stored on the file system will
> > no longer be useful. By storing the headers as a compressed archive
> > within the kernel, we can avoid these issues that have been a hindrance
> > for a long time.
> > 
> > The best way to use this feature is by building it in. Several users
> > have a need for this, when they switch debug kernels, they donot want to
> > update the filesystem or worry about it where to store the headers on
> > it. However, the feature is also buildable as a module in case the user
> > desires it not being part of the kernel image. This makes it possible to
> > load and unload the headers from memory on demand. A tracing program, or
> > a kernel module builder can load the module, do its operations, and then
> > unload the module to save kernel memory. The total memory needed is 3.8MB.
> > 
> > By having the archive available at a fixed location independent of
> > filesystem dependencies and conventions, all debugging tools can
> > directly refer to the fixed location for the archive, without concerning
> > with where the headers on a typical filesystem which significantly
> > simplifies tooling that needs kernel headers.
> > 
> > The code to read the headers is based on /proc/config.gz code and uses
> > the same technique to embed the headers.
> > 
> > IKHD_ST and IKHD_ED markers as is to facilitate future patches that
> > would extract the headers from a kernel or module image.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> I applied both patches on 5.1-rc2 and managed to compile and run several
> eBPF programs using the untarred headers from /proc/kheaders.tar.xz on my juno.
> 
> Tested-by: Qais Yousef <qais.yousef@arm.com>

Thank you Qais!  Masahiro, if Ok with you now could you give your tag?

thanks,

 - Joel


> > ---
> > 
> >     v5 -> v6: (Masahiro Yamada suggestions mostly)
> >     - Dropped support for module building.
> >     - Rebuild archive if script changes.
> >     - Move archive file list to script.
> >     - Move build script to kernel directory.
> > 
> >     v4 -> v5:
> >     (v4 was Tested-by the following folks)
> >         Tested-by: qais.yousef@arm.com
> >         Tested-by: dietmar.eggemann@arm.com
> >         Tested-by: linux@manojrajarao.com
> 
> Thanks!
> 
> --
> Qais Yousef

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Masahiro Yamada]

Hi Joel,

I have no objection to this patch.
I checked though it once again,
please let me point out a little more.

They are all nits.



On Tue, Apr 9, 2019 at 6:37 AM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> Introduce in-kernel headers which are made available as an archive
> through proc (/proc/kheaders.tar.xz file). This archive makes it
> possible to run eBPF and other tracing programs tracing programs that

Just one "tracing programs" is enough.


> need to extend the kernel for tracing purposes without any dependency on
> the file system having headers.
>
> On Android and embedded systems, it is common to switch kernels but not
> have kernel headers available on the file system. Further once a
> different kernel is booted, any headers stored on the file system will
> no longer be useful. By storing the headers as a compressed archive
> within the kernel, we can avoid these issues that have been a hindrance
> for a long time.
>
> The best way to use this feature is by building it in. Several users
> have a need for this, when they switch debug kernels, they donot want to

'donot' -> 'do not' ?


> update the filesystem or worry about it where to store the headers on
> it. However, the feature is also buildable as a module in case the user
> desires it not being part of the kernel image. This makes it possible to
> load and unload the headers from memory on demand. A tracing program, or
> a kernel module builder can load the module, do its operations, and then
> unload the module to save kernel memory. The total memory needed is 3.8MB.
>
> By having the archive available at a fixed location independent of
> filesystem dependencies and conventions, all debugging tools can
> directly refer to the fixed location for the archive, without concerning
> with where the headers on a typical filesystem which significantly
> simplifies tooling that needs kernel headers.
>
> The code to read the headers is based on /proc/config.gz code and uses
> the same technique to embed the headers.
>
> IKHD_ST and IKHD_ED markers as is to facilitate future patches that
> would extract the headers from a kernel or module image.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

[snip]

>
> diff --git a/init/Kconfig b/init/Kconfig
> index 4592bf7997c0..ea75bfbf7dfa 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -580,6 +580,17 @@ config IKCONFIG_PROC
>           This option enables access to the kernel configuration file
>           through /proc/config.gz.
>
> +config IKHEADERS_PROC
> +       tristate "Enable kernel header artifacts through /proc/kheaders.tar.xz"
> +       depends on PROC_FS
> +       help
> +         This option enables access to the kernel header and other artifacts that

This line is indented by a TAB, which is correct.


> +          are generated during the build process. These can be used to build kernel
> +          modules or by other in-kernel programs such as those generated by eBPF

Now that you have dropped the ability to "build kernel modules",
I'd like you to update this help message.

> +          and systemtap tools. If you build the headers as a module, a module
> +          called kheaders.ko is built which can be loaded on-demand to get access
> +          to the headers.

These lines are indented by 8-spaces instead of one TAB.
Please use TAB-indentation consistently.


[snip]


> +rm -rf $cpio_dir
> +mkdir $cpio_dir
> +
> +pushd $kroot > /dev/null
> +for f in $src_file_list;
> +       do find "$f" ! -name "*.cmd" ! -name ".*";
> +done | cpio --quiet -pd $cpio_dir
> +popd > /dev/null
> +
> +# The second CPIO can complain if files already exist which can
> +# happen with out of tree builds. Just silence CPIO for now.
> +for f in $obj_file_list;
> +       do find "$f" ! -name "*.cmd" ! -name ".*";
> +done | cpio --quiet -pd $cpio_dir >/dev/null 2>&1
> +

Could you add a simple comment about what the following code is doing?
"Remove comments except SPDX" etc.


> +find  $cpio_dir -type f -print0 |

Please replace two spaces after 'find' with one.


> +       xargs -0 -P8 -n1 perl -pi -e 'BEGIN {undef $/;}; s/\/\*((?!SPDX).)*?\*\///smg;'
> +
> +tar -Jcf $tarfile -C $cpio_dir/ . > /dev/null
> +
> +echo "$src_files_md5" > kernel/kheaders.md5
> +echo "$obj_files_md5" >> kernel/kheaders.md5
> +echo "$(md5sum $tarfile | cut -d ' ' -f1)" >> kernel/kheaders.md5
> +
> +rm -rf $cpio_dir
> diff --git a/kernel/kheaders.c b/kernel/kheaders.c
> new file mode 100644
> index 000000000000..d072a958a8f1
> --- /dev/null
> +++ b/kernel/kheaders.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * kernel/kheaders.c
> + * Provide headers and artifacts needed to build kernel modules.

Ditto. Could you update this comment ?


> + * (Borrowed code from kernel/configs.c)
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/proc_fs.h>
> +#include <linux/init.h>
> +#include <linux/uaccess.h>
> +
> +/*
> + * Define kernel_headers_data and kernel_headers_data_end, within which the the
> + * compressed kernel headers are stpred. The file is first compressed with xz.
> + */
> +
> +asm (
> +"      .pushsection .rodata, \"a\"             \n"
> +"      .global kernel_headers_data             \n"
> +"kernel_headers_data:                          \n"
> +"      .incbin \"kernel/kheaders_data.tar.xz\" \n"
> +"      .global kernel_headers_data_end         \n"
> +"kernel_headers_data_end:                      \n"
> +"      .popsection                             \n"
> +);

You mentioned "IKHD_ST and IKHD_ED markers..." in the commit description,
but I do not see them in the code.

If you plan to work on a tool to extract the headers,
I think it is OK to have the markers here.

Anyway, please make the code and the commit-log consistent.


> +
> +extern char kernel_headers_data;
> +extern char kernel_headers_data_end;
> +
> +static ssize_t
> +ikheaders_read_current(struct file *file, char __user *buf,

Could you stretch this line ?
It will still fit in 80-cols.

(This is a coding style error in kernel/configs.c)



Last thing, when CONFIG_IKHEADERS_PROC=y,
I always see:
  GEN     kernel/kheaders_data.tar.xz

which I think misleading because
the script is just checking the md5sum.



What I like to see is:
  CHK     kernel/kheaders_data.tar.xz
for checking md5sum.

And,
  GEN     kernel/kheaders_data.tar.xz
for really (re-)generating the tarball.


How about this code?

index e3c581d8cde7..12399614c350 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -125,7 +125,7 @@ $(obj)/config_data.gz: $(KCONFIG_CONFIG) FORCE

 $(obj)/kheaders.o: $(obj)/kheaders_data.tar.xz

-quiet_cmd_genikh = GEN     $(obj)/kheaders_data.tar.xz
+quiet_cmd_genikh = CHK     $(obj)/kheaders_data.tar.xz
 cmd_genikh = $(srctree)/kernel/gen_ikh_data.sh $@
 $(obj)/kheaders_data.tar.xz: FORCE
        $(call cmd,genikh)
diff --git a/kernel/gen_ikh_data.sh b/kernel/gen_ikh_data.sh
index ef72c2740d01..613960e18691 100755
--- a/kernel/gen_ikh_data.sh
+++ b/kernel/gen_ikh_data.sh
@@ -57,6 +57,10 @@ if [ -f kernel/kheaders.md5 ] &&
                exit
 fi

+if [ "${quiet}" != "silent_" ]; then
+       echo "  GEN     $tarfile"
+fi
+
 rm -rf $cpio_dir
 mkdir $cpio_dir


Thanks.

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 2/2] init/config: Do not select BUILD_BIN2C for IKCONFIG

[Masahiro Yamada]

On Tue, Apr 9, 2019 at 6:38 AM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> Since commit 13610aa908dc ("kernel/configs: use .incbin directive to
> embed config_data.gz"), IKCONFIG no longer uses BUILD_BIN2C so prevent
> it from being selected in Kconfig.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>


This patch is independent of 1/2,
and trivial enough.

Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>


> ---
>  init/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index ea75bfbf7dfa..ffcb2f90543f 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -562,7 +562,6 @@ config BUILD_BIN2C
>
>  config IKCONFIG
>         tristate "Kernel .config support"
> -       select BUILD_BIN2C
>         ---help---
>           This option enables the complete Linux kernel ".config" file
>           contents to be saved in the kernel. It provides documentation
> --
> 2.21.0.392.gf8f6787159e-goog
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Joel Fernandes]

On Thu, Apr 11, 2019 at 10:47:23AM +0900, Masahiro Yamada wrote:
> Hi Joel,
> 
> I have no objection to this patch.
> I checked though it once again,
> please let me point out a little more.
> 
> They are all nits.
> 
> 
> 
> On Tue, Apr 9, 2019 at 6:37 AM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> >
> > Introduce in-kernel headers which are made available as an archive
> > through proc (/proc/kheaders.tar.xz file). This archive makes it
> > possible to run eBPF and other tracing programs tracing programs that
> 
> Just one "tracing programs" is enough.

fixed.

> > need to extend the kernel for tracing purposes without any dependency on
> > the file system having headers.
> >
> > On Android and embedded systems, it is common to switch kernels but not
> > have kernel headers available on the file system. Further once a
> > different kernel is booted, any headers stored on the file system will
> > no longer be useful. By storing the headers as a compressed archive
> > within the kernel, we can avoid these issues that have been a hindrance
> > for a long time.
> >
> > The best way to use this feature is by building it in. Several users
> > have a need for this, when they switch debug kernels, they donot want to
> 
> 'donot' -> 'do not' ?

fixed

[snip]
> >
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 4592bf7997c0..ea75bfbf7dfa 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -580,6 +580,17 @@ config IKCONFIG_PROC
> >           This option enables access to the kernel configuration file
> >           through /proc/config.gz.
> >
> > +config IKHEADERS_PROC
> > +       tristate "Enable kernel header artifacts through /proc/kheaders.tar.xz"
> > +       depends on PROC_FS
> > +       help
> > +         This option enables access to the kernel header and other artifacts that
> 
> This line is indented by a TAB, which is correct.
> 
> 
> > +          are generated during the build process. These can be used to build kernel
> > +          modules or by other in-kernel programs such as those generated by eBPF
> 
> Now that you have dropped the ability to "build kernel modules",
> I'd like you to update this help message.

Sorry, will fix.

> > +          and systemtap tools. If you build the headers as a module, a module
> > +          called kheaders.ko is built which can be loaded on-demand to get access
> > +          to the headers.
> 
> These lines are indented by 8-spaces instead of one TAB.
> Please use TAB-indentation consistently.
> 
> 
> [snip]
> 
> 
> > +rm -rf $cpio_dir
> > +mkdir $cpio_dir
> > +
> > +pushd $kroot > /dev/null
> > +for f in $src_file_list;
> > +       do find "$f" ! -name "*.cmd" ! -name ".*";
> > +done | cpio --quiet -pd $cpio_dir
> > +popd > /dev/null
> > +
> > +# The second CPIO can complain if files already exist which can
> > +# happen with out of tree builds. Just silence CPIO for now.
> > +for f in $obj_file_list;
> > +       do find "$f" ! -name "*.cmd" ! -name ".*";
> > +done | cpio --quiet -pd $cpio_dir >/dev/null 2>&1
> > +
> 
> Could you add a simple comment about what the following code is doing?
> "Remove comments except SPDX" etc.

Ok.

> > +find  $cpio_dir -type f -print0 |
> 
> Please replace two spaces after 'find' with one.

fixed

> > +       xargs -0 -P8 -n1 perl -pi -e 'BEGIN {undef $/;}; s/\/\*((?!SPDX).)*?\*\///smg;'
> > +
> > +tar -Jcf $tarfile -C $cpio_dir/ . > /dev/null
> > +
> > +echo "$src_files_md5" > kernel/kheaders.md5
> > +echo "$obj_files_md5" >> kernel/kheaders.md5
> > +echo "$(md5sum $tarfile | cut -d ' ' -f1)" >> kernel/kheaders.md5
> > +
> > +rm -rf $cpio_dir
> > diff --git a/kernel/kheaders.c b/kernel/kheaders.c
> > new file mode 100644
> > index 000000000000..d072a958a8f1
> > --- /dev/null
> > +++ b/kernel/kheaders.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * kernel/kheaders.c
> > + * Provide headers and artifacts needed to build kernel modules.
> 
> Ditto. Could you update this comment ?

fixed.

> > + * (Borrowed code from kernel/configs.c)
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/init.h>
> > +#include <linux/uaccess.h>
> > +
> > +/*
> > + * Define kernel_headers_data and kernel_headers_data_end, within which the the
> > + * compressed kernel headers are stpred. The file is first compressed with xz.
> > + */
> > +
> > +asm (
> > +"      .pushsection .rodata, \"a\"             \n"
> > +"      .global kernel_headers_data             \n"
> > +"kernel_headers_data:                          \n"
> > +"      .incbin \"kernel/kheaders_data.tar.xz\" \n"
> > +"      .global kernel_headers_data_end         \n"
> > +"kernel_headers_data_end:                      \n"
> > +"      .popsection                             \n"
> > +);
> 
> You mentioned "IKHD_ST and IKHD_ED markers..." in the commit description,
> but I do not see them in the code.

fixed

> If you plan to work on a tool to extract the headers,
> I think it is OK to have the markers here.
> 
> Anyway, please make the code and the commit-log consistent.

yes, will do. thanks

> > +extern char kernel_headers_data;
> > +extern char kernel_headers_data_end;
> > +
> > +static ssize_t
> > +ikheaders_read_current(struct file *file, char __user *buf,
> 
> Could you stretch this line ?
> It will still fit in 80-cols.
> 
> (This is a coding style error in kernel/configs.c)

It takes 87 cols if I expand, so I'll leave it as is.

> Last thing, when CONFIG_IKHEADERS_PROC=y,
> I always see:
>   GEN     kernel/kheaders_data.tar.xz
> 
> which I think misleading because
> the script is just checking the md5sum.
> 
> 
> 
> What I like to see is:
>   CHK     kernel/kheaders_data.tar.xz
> for checking md5sum.
> 
> And,
>   GEN     kernel/kheaders_data.tar.xz
> for really (re-)generating the tarball.
> 
> 
> How about this code?

Yes this is better, I changed it to that.

Also looking forward to your tag on the v7 posting if it looks to you now.

thanks a lot for the review!

 - Joel


> index e3c581d8cde7..12399614c350 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -125,7 +125,7 @@ $(obj)/config_data.gz: $(KCONFIG_CONFIG) FORCE
> 
>  $(obj)/kheaders.o: $(obj)/kheaders_data.tar.xz
> 
> -quiet_cmd_genikh = GEN     $(obj)/kheaders_data.tar.xz
> +quiet_cmd_genikh = CHK     $(obj)/kheaders_data.tar.xz
>  cmd_genikh = $(srctree)/kernel/gen_ikh_data.sh $@
>  $(obj)/kheaders_data.tar.xz: FORCE
>         $(call cmd,genikh)
> diff --git a/kernel/gen_ikh_data.sh b/kernel/gen_ikh_data.sh
> index ef72c2740d01..613960e18691 100755
> --- a/kernel/gen_ikh_data.sh
> +++ b/kernel/gen_ikh_data.sh
> @@ -57,6 +57,10 @@ if [ -f kernel/kheaders.md5 ] &&
>                 exit
>  fi
> 
> +if [ "${quiet}" != "silent_" ]; then
> +       echo "  GEN     $tarfile"
> +fi
> +
>  rm -rf $cpio_dir
>  mkdir $cpio_dir
> 
> 
> Thanks.
> 
> -- 
> Best Regards
> Masahiro Yamada

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Masahiro Yamada]

On Sat, Apr 13, 2019 at 3:02 AM Joel Fernandes <joel@joelfernandes.org> wrote:

>
> > > +extern char kernel_headers_data;
> > > +extern char kernel_headers_data_end;
> > > +
> > > +static ssize_t
> > > +ikheaders_read_current(struct file *file, char __user *buf,
> >
> > Could you stretch this line ?
> > It will still fit in 80-cols.
> >
> > (This is a coding style error in kernel/configs.c)
>
> It takes 87 cols if I expand, so I'll leave it as is.
>


Sorry if what I said was unclear.

Since I just did not a good reason to put
"static ssize_t" in the previous line,
I meant like follows:


[Before]
static ssize_t
ikheaders_read_current(struct file *file, char __user *buf,
                       size_t len, loff_t *offset)



[After]
static ssize_t ikheaders_read_current(struct file *file, char __user *buf,
                                      size_t len, loff_t *offset)


(takes 74-cols.)


(I am sending this from Gmail, so I am not sure
how it will look like from you...)


Anyway, it is super-bikeshedding stuff.
It is OK as-is.



--
Best Regards
Masahiro Yamada

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Daniel Colascione]

On Fri, Apr 12, 2019 at 6:49 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Sat, Apr 13, 2019 at 3:02 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> >
> > > > +extern char kernel_headers_data;
> > > > +extern char kernel_headers_data_end;
> > > > +
> > > > +static ssize_t
> > > > +ikheaders_read_current(struct file *file, char __user *buf,
> > >
> > > Could you stretch this line ?
> > > It will still fit in 80-cols.
> > >
> > > (This is a coding style error in kernel/configs.c)
> >
> > It takes 87 cols if I expand, so I'll leave it as is.
> >
>
>
> Sorry if what I said was unclear.
>
> Since I just did not a good reason to put
> "static ssize_t" in the previous line,
> I meant like follows:
>
>
> [Before]
> static ssize_t
> ikheaders_read_current(struct file *file, char __user *buf,
>                        size_t len, loff_t *offset)
>
>
>
> [After]
> static ssize_t ikheaders_read_current(struct file *file, char __user *buf,
>                                       size_t len, loff_t *offset)
>
>
> (takes 74-cols.)
>
>
> (I am sending this from Gmail, so I am not sure
> how it will look like from you...)
>
>
> Anyway, it is super-bikeshedding stuff.
> It is OK as-is.
>

What about sorting the files for determinism?

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Masahiro Yamada]

On Sat, Apr 13, 2019 at 10:53 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Fri, Apr 12, 2019 at 6:49 PM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Sat, Apr 13, 2019 at 3:02 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > >
> > > > > +extern char kernel_headers_data;
> > > > > +extern char kernel_headers_data_end;
> > > > > +
> > > > > +static ssize_t
> > > > > +ikheaders_read_current(struct file *file, char __user *buf,
> > > >
> > > > Could you stretch this line ?
> > > > It will still fit in 80-cols.
> > > >
> > > > (This is a coding style error in kernel/configs.c)
> > >
> > > It takes 87 cols if I expand, so I'll leave it as is.
> > >
> >
> >
> > Sorry if what I said was unclear.
> >
> > Since I just did not a good reason to put
> > "static ssize_t" in the previous line,
> > I meant like follows:
> >
> >
> > [Before]
> > static ssize_t
> > ikheaders_read_current(struct file *file, char __user *buf,
> >                        size_t len, loff_t *offset)
> >
> >
> >
> > [After]
> > static ssize_t ikheaders_read_current(struct file *file, char __user *buf,
> >                                       size_t len, loff_t *offset)
> >
> >
> > (takes 74-cols.)
> >
> >
> > (I am sending this from Gmail, so I am not sure
> > how it will look like from you...)
> >
> >
> > Anyway, it is super-bikeshedding stuff.
> > It is OK as-is.
> >
>
> What about sorting the files for determinism?


Do you mean the order of files in the tar archive?
I think it is a good idea for reproducible build.



It looks like the tar command supports --sort=name option,
but I did not test it at all.



Perhaps, the sorting policy could be affected by locale.
We saw it for the 'find' command.
I am not sure about the tar command though.



commit f55f2328bb28a8517620518c5d124f5194673309
Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Date:   Fri Aug 17 14:03:19 2018 +0200

    kbuild: make sorting initramfs contents independent of locale








--
Best Regards
Masahiro Yamada

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Masahiro Yamada]

On Sat, Apr 13, 2019 at 11:58 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Sat, Apr 13, 2019 at 10:53 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Fri, Apr 12, 2019 at 6:49 PM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > >
> > > On Sat, Apr 13, 2019 at 3:02 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > >
> > > > > > +extern char kernel_headers_data;
> > > > > > +extern char kernel_headers_data_end;
> > > > > > +
> > > > > > +static ssize_t
> > > > > > +ikheaders_read_current(struct file *file, char __user *buf,
> > > > >
> > > > > Could you stretch this line ?
> > > > > It will still fit in 80-cols.
> > > > >
> > > > > (This is a coding style error in kernel/configs.c)
> > > >
> > > > It takes 87 cols if I expand, so I'll leave it as is.
> > > >
> > >
> > >
> > > Sorry if what I said was unclear.
> > >
> > > Since I just did not a good reason to put
> > > "static ssize_t" in the previous line,
> > > I meant like follows:
> > >
> > >
> > > [Before]
> > > static ssize_t
> > > ikheaders_read_current(struct file *file, char __user *buf,
> > >                        size_t len, loff_t *offset)
> > >
> > >
> > >
> > > [After]
> > > static ssize_t ikheaders_read_current(struct file *file, char __user *buf,
> > >                                       size_t len, loff_t *offset)
> > >
> > >
> > > (takes 74-cols.)
> > >
> > >
> > > (I am sending this from Gmail, so I am not sure
> > > how it will look like from you...)
> > >
> > >
> > > Anyway, it is super-bikeshedding stuff.
> > > It is OK as-is.
> > >
> >
> > What about sorting the files for determinism?
>
>
> Do you mean the order of files in the tar archive?
> I think it is a good idea for reproducible build.
>
>
>
> It looks like the tar command supports --sort=name option,
> but I did not test it at all.
>
>
>
> Perhaps, the sorting policy could be affected by locale.
> We saw it for the 'find' command.


'sort' command is affected by locale.



> I am not sure about the tar command though.
>
>
>
> commit f55f2328bb28a8517620518c5d124f5194673309
> Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Date:   Fri Aug 17 14:03:19 2018 +0200
>
>     kbuild: make sorting initramfs contents independent of locale
>
>
>
>
>
>
>
>
> --
> Best Regards
> Masahiro Yamada



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 1/2] Provide in-kernel headers to make extending kernel easier

[Daniel Colascione]

On Fri, Apr 12, 2019 at 8:10 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Sat, Apr 13, 2019 at 11:58 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Sat, Apr 13, 2019 at 10:53 AM Daniel Colascione <dancol@google.com> wrote:
> > >
> > > On Fri, Apr 12, 2019 at 6:49 PM Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > > >
> > > > On Sat, Apr 13, 2019 at 3:02 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > >
> > > > > > > +extern char kernel_headers_data;
> > > > > > > +extern char kernel_headers_data_end;
> > > > > > > +
> > > > > > > +static ssize_t
> > > > > > > +ikheaders_read_current(struct file *file, char __user *buf,
> > > > > >
> > > > > > Could you stretch this line ?
> > > > > > It will still fit in 80-cols.
> > > > > >
> > > > > > (This is a coding style error in kernel/configs.c)
> > > > >
> > > > > It takes 87 cols if I expand, so I'll leave it as is.
> > > > >
> > > >
> > > >
> > > > Sorry if what I said was unclear.
> > > >
> > > > Since I just did not a good reason to put
> > > > "static ssize_t" in the previous line,
> > > > I meant like follows:
> > > >
> > > >
> > > > [Before]
> > > > static ssize_t
> > > > ikheaders_read_current(struct file *file, char __user *buf,
> > > >                        size_t len, loff_t *offset)
> > > >
> > > >
> > > >
> > > > [After]
> > > > static ssize_t ikheaders_read_current(struct file *file, char __user *buf,
> > > >                                       size_t len, loff_t *offset)
> > > >
> > > >
> > > > (takes 74-cols.)
> > > >
> > > >
> > > > (I am sending this from Gmail, so I am not sure
> > > > how it will look like from you...)
> > > >
> > > >
> > > > Anyway, it is super-bikeshedding stuff.
> > > > It is OK as-is.
> > > >
> > >
> > > What about sorting the files for determinism?
> >
> >
> > Do you mean the order of files in the tar archive?

Yes.

> > I think it is a good idea for reproducible build.
> >
> >
> >
> > It looks like the tar command supports --sort=name option,
> > but I did not test it at all.
> >
> >
> >
> > Perhaps, the sorting policy could be affected by locale.
> > We saw it for the 'find' command.
>
>
> 'sort' command is affected by locale.

It should be independent of locale if you set LC_ALL=C in the
environment while running it.