Re: [PATCH 4/4] create-diff-object: also include relas that point to changed sections

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Thu, Nov 7, 2024 at 3:15 PM Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
>
> create-diff-object has a special handling for some specific sections, like
> .altinstructions or .livepatch.hooks.*.  The contents of those sections are in
> the form of array elements, where each element can be processed independently
> of the rest.  For example an element in .altinstructions is a set of
> replacement coordinates, with the layout specified by the alt_instr struct.  
> In
> the case of .livepatch.hooks.* each element is a pointer to a hook function to
> call.
>
> The contents of this array is processed element wise, so that
> create-diff-object can decide whether the element relates to the content in 
> the
> livepatch and thus needs keeping.  Such relation is driven based on the
> contents of the relocations for the special sections.  If a relocation to be
> applied to a special section element depends on any symbol to be included in
> the livepatch then the special element is also considered required and thus
> added to the livepatch contents.
>
> However relocations don't always reference function type symbols, they can 
> also
> reference sections type symbols, and that's usually the case with hook symbols
> that have relocations based on section symbols, as an example:
>
> RELOCATION RECORDS FOR [.livepatch.hooks.load]:
> OFFSET           TYPE              VALUE
> 0000000000000000 R_X86_64_64       .text.foobar
>
> Symbol information for .text.foobar:
>
> 0000000000000000 l    d  .text.foobar      0000000000000000 .text.foobar
>
> As seen above, the .livepatch.hooks.load relocation uses a non-function 
> symbol,
> which given the current code in should_keep_rela_group() would mean it's not
> considered for inclusion in the livepatch.
>
> Fix this by allowing should_keep_rela_group() to also keep relocations if they
> either point to function or section symbols.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  create-diff-object.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 924059a1842b..c21cc576052a 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1158,11 +1158,17 @@ static int should_keep_rela_group(struct section 
> *sec, int start, int size)
>         struct rela *rela;
>         int found = 0;
>
> -       /* check if any relas in the group reference any changed functions */
> +       /*
> +        * Check if any relas in the group reference any changed functions or
> +        * sections.  As seen by hook related relocations 
> (.livepatch.hooks.*),
> +        * it's possible they use the section symbol as a reference rather 
> than
> +        * the function symbol.
> +        */
>         list_for_each_entry(rela, &sec->relas, list) {
>                 if (rela->offset >= start &&
>                     rela->offset < start + size &&
> -                   rela->sym->type == STT_FUNC &&
> +                   (rela->sym->type == STT_FUNC ||
> +                    rela->sym->type == STT_SECTION) &&
>                     rela->sym->sec->include) {
>                         found = 1;
>                         log_debug("new/changed symbol %s found in special 
> section %s\n",
> --
> 2.46.0
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>

Thanks