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@xxxxxxxxxxxxxxxxx> 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