Received-SPF: pass (google.com: domain of linux-kernel+bounces-195853-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 30 May 2024 16:07:20 -0400
From: Joe Lawrence <joe.lawrence@redhat.com>
To: Lukas Hruska <lhruska@suse.cz>
Cc: pmladek@suse.com, mbenes@suse.cz, jpoimboe@kernel.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kbuild@vger.kernel.org, mpdesouza@suse.com,
	Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
Message-ID: <ZljceDZ7eqEJtYhQ@redhat.com>
References: <20240516133009.20224-1-lhruska@suse.cz>
 <20240516133009.20224-3-lhruska@suse.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240516133009.20224-3-lhruska@suse.cz>

On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote:
> Livepatches need to access external symbols which can't be handled
> by the normal relocation mechanism. It is needed for two types
> of symbols:
> 
>   + Symbols which can be local for the original livepatched function.
>     The alternative implementation in the livepatch sees them
>     as external symbols.
> 
>   + Symbols in modules which are exported via EXPORT_SYMBOL*(). They
>     must be handled special way otherwise the livepatch module would
>     depend on the livepatched one. Loading such livepatch would cause
>     loading the other module as well.
> 
> The address of these symbols can be found via kallsyms. Or they can
> be relocated using livepatch specific relocation sections as specified
> in Documentation/livepatch/module-elf-format.txt.
> 
> Currently, there is no trivial way to embed the required information as
> requested in the final livepatch elf object. klp-convert solves this
> problem by using annotations in the elf object to convert the relocation
> accordingly to the specification, enabling it to be handled by the
> livepatch loader.
> 
> Given the above, create scripts/livepatch to hold tools developed for
> livepatches and add source files for klp-convert there.
> 
> Allow to annotate such external symbols in the livepatch by a macro
> KLP_RELOC_SYMBOL(). It will create symbol with all needed
> metadata. For example:
> 
>   extern char *saved_command_line \
>                  KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0);
> 
> would create symbol
> 
> $>readelf -r -W <compiled livepatch module>:
> Relocation section '.rela.text' at offset 0x32e60 contains 10 entries:
>     Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
> [...]
> 0000000000000068  0000003c00000002 R_X86_64_PC32          0000000000000000 .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4
> [...]
> 
> Also add scripts/livepatch/klp-convert. The tool transforms symbols
> created by KLP_RELOC_SYMBOL() to object specific rela sections
> and rela entries which would later be proceed when the livepatch
> or the livepatched object is loaded.
> 
> For example, klp-convert would replace the above symbol with:
> 
> $> readelf -r -W <livepatch_module_proceed_by_klp_convert>
> Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 entry:
>     Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
> 0000000000000068  0000003c00000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4
> 
> klp-convert relies on libelf and on a list implementation. Add files
> scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a libelf
> interfacing layer and scripts/livepatch/list.h, which is a list
> implementation.
> 
> Update Makefiles to correctly support the compilation of the new tool,
> update MAINTAINERS file and add a .gitignore file.
> 
> [jpoimboe@redhat.com: initial version]
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> [joe.lawrence@redhat.com: clean-up and fixes]
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> [lhruska@suse.cz: klp-convert code, minimal approach]
> Signed-off-by: Lukas Hruska <lhruska@suse.cz>
> Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  MAINTAINERS                     |   1 +
>  include/linux/livepatch.h       |  19 +
>  scripts/Makefile                |   1 +
>  scripts/livepatch/.gitignore    |   1 +
>  scripts/livepatch/Makefile      |   5 +
>  scripts/livepatch/elf.c         | 817 ++++++++++++++++++++++++++++++++
>  scripts/livepatch/elf.h         |  73 +++
>  scripts/livepatch/klp-convert.c | 284 +++++++++++
>  scripts/livepatch/klp-convert.h |  23 +
>  scripts/livepatch/list.h        | 391 +++++++++++++++
>  10 files changed, 1615 insertions(+)
>  create mode 100644 scripts/livepatch/.gitignore
>  create mode 100644 scripts/livepatch/Makefile
>  create mode 100644 scripts/livepatch/elf.c
>  create mode 100644 scripts/livepatch/elf.h
>  create mode 100644 scripts/livepatch/klp-convert.c
>  create mode 100644 scripts/livepatch/klp-convert.h
>  create mode 100644 scripts/livepatch/list.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 130b8b0bd4f7..d2facc1f4e15 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12618,6 +12618,7 @@ F:	include/uapi/linux/livepatch.h
>  F:	kernel/livepatch/
>  F:	kernel/module/livepatch.c
>  F:	samples/livepatch/
> +F:	scripts/livepatch/
>  F:	tools/testing/selftests/livepatch/
>  
>  LLC (802.2)
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 9b9b38e89563..83bbcd1c43fd 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -235,6 +235,25 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
>  			     unsigned int symindex, unsigned int secindex,
>  			     const char *objname);
>  
> +/**
> + * KLP_RELOC_SYMBOL_POS - define relocation for external symbols
> + *
> + * @LP_OBJ_NAME: name of the livepatched object where the symbol is needed
> + * @SYM_OBJ_NAME: name of the object where the symbol exists
> + * @SYM_NAME: symbol name
> + * @SYM_POS: position of the symbol in SYM_OBJ when there are more
> + *       symbols of the same name.
> + *
> + * Use for annotating external symbols used in livepatches which are
> + * not exported in vmlinux or are in livepatched modules, see
> + * Documentation/livepatch/module-elf-format.rst
> + */
> +#define KLP_RELOC_SYMBOL_POS(LP_OBJ_NAME, SYM_OBJ_NAME, SYM_NAME, SYM_POS)	\
> +	asm("\".klp.sym.rela." #LP_OBJ_NAME "." #SYM_OBJ_NAME "." #SYM_NAME "," #SYM_POS "\"")
                                                                            ^^^
I think I found a potential bug, or at least compatiblity problem with
including a comma "," character in this symbol format and older versions
of the GNU assembler.  The good news is that other delimiter characters
like "." or "#" seem to work out fine.

If you want to reproduce, you'll need a version of `as` like binutils
2.36.1 and try building the samples/livepatch/livepatch-extern-symbol.ko
and you should get an error like:

  Assembler messages:
  Warning: missing closing '"'
  Warning: missing closing '"'
  Error: too many memory references for `movq'


If you want to retrace my adventure, here are my steps:

  1) Clone klp-convert-tree repo branch containing this patchset +
  Petr's review comments + a few helpful things for klp-convert
  development:
  
    $ git clone \
        --single-branch --branch=klp-convert-minimal-v1-review --depth=9 \
        https://github.com/joe-lawrence/klp-convert-tree.git
    [ ... snip ... ]
    $ cd klp-convert-tree
  
  2) Override .cross-dev defaults:
  
    $ export BUILD_ARCHES=x86_64
    $ export COMPILER=gcc-11.1.0
    $ export URL=https://cdn.kernel.org/pub/tools/crosstool/files/bin/x86_64/
    $ export OUTDIR_PREFIX=$(pwd)/build
    $ export COMPILER_INSTALL_PATH=$(pwd)/cross-compiler
  
  3) Setup x86_64 default .config (this will download and install the
  gcc-11.1.0 compiler from cdn.kernel.org):
  
    $ ./cross-dev make defconfig
    
    x86_64 : make defconfig ...
    Compiler will be installed in /root/klp-convert-tree/cross-compiler
    [ ... snip ... ]
  
  4) Add kernel livepatching configuration options:
  
    $ ./cross-dev klp-config
    
    Configuring x86_64 ...
    [ ... snip ... ]
    
    $ grep LIVEPATCH "$OUTDIR_PREFIX"-x86_64/.config
    CONFIG_HAVE_LIVEPATCH=y
    CONFIG_LIVEPATCH=y
    CONFIG_SAMPLE_LIVEPATCH=m
  
  5) Run the cross-compiler build until it hits a build error on
  livepatch-extern-symbol.ko:
  
    $ ./cross-dev make -j$(nproc)
    [ ... snip ... ]
    make: Target '__all' not remade because of errors.
    [ x86_64 : make -j48 = FAIL ]
  
  6) With pre-requisites already built, retry the external symbol sample
  and add -save-temps to the KCFLAGS to keep the generated assembly file:
  
    $ KCFLAGS="-save-temps=obj" ./cross-dev make samples/livepatch/livepatch-extern-symbol.ko
    [ ... snip ... ]
    samples/livepatch/livepatch-extern-symbol.s: Assembler messages:
    samples/livepatch/livepatch-extern-symbol.s:103: Warning: missing closing '"'
    samples/livepatch/livepatch-extern-symbol.s:103: Warning: missing closing '"'
    samples/livepatch/livepatch-extern-symbol.s:103: Error: too many memory references for `movq'
    [ ... snip ... ]
  
  7) Which line is that?
  
    $ awk 'NR==103' "$OUTDIR_PREFIX"-x86_64/samples/livepatch/livepatch-extern-symbol.s
            movq    ".klp.sym.rela.vmlinux.vmlinux.saved_command_line,0"(%rip), %rdx


You could alternatively poke at it through the compiler explorer service
and toggle the source and binutils versions:

  (error)   binutils 2.36.1 : https://godbolt.org/z/cGGs6rfWe
  (success) binutils 2.38   : https://godbolt.org/z/ffzza3vYd

--
Joe