| Message ID | 20190416122241.28342-1-wipawel@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [livepatch-build-tools,part3,1/3] create-diff-object: Do not create empty .livepatch.funcs section | expand |
On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote: > When there is no changed function in the generated payload, do not > create an empty .livepatch.funcs section. Hypervisor code considers > such payloads as broken and rejects to load them. > > Such payloads without any changed functions may appear when only > hooks are specified. Ross, I am going to push this in next week unless you have other thoughts? > > Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de> > Reviewed-by: Martin Mazein <amazein@amazon.de> > Reviewed-by: Martin Pohlack <mpohlack@amazon.de> > > CR: https://code.amazon.com/reviews/CR-7368634 > --- > create-diff-object.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/create-diff-object.c b/create-diff-object.c > index 82f777e..af2245c 100644 > --- a/create-diff-object.c > +++ b/create-diff-object.c > @@ -1744,6 +1744,11 @@ static void livepatch_create_patches_sections(struct kpatch_elf *kelf, > if (sym->type == STT_FUNC && sym->status == CHANGED) > nr++; > > + if (nr == 0) { > + log_debug("No changed functions found. Skipping .livepatch.funcs section creation\n"); > + return; > + } > + > /* create text/rela section pair */ > sec = create_section_pair(kelf, ".livepatch.funcs", sizeof(*funcs), nr); > relasec = sec->rela; > -- > 2.16.5 > > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich > Ust-ID: DE 289 237 879 > Eingetragen am Amtsgericht Charlottenburg HRB 149173 B > >
On 25/04/2019 05:51, Konrad Rzeszutek Wilk wrote: > On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote: >> When there is no changed function in the generated payload, do not >> create an empty .livepatch.funcs section. Hypervisor code considers >> such payloads as broken and rejects to load them. >> >> Such payloads without any changed functions may appear when only >> hooks are specified. > Ross, I am going to push this in next week unless you have other thoughts? No objections, but as the CR: links are internal links, which get the following message when followed: > Non-Amazon Employees, you have been mistakenly directed to an > internal-only Amazon system. If you were given a link that brought you > to this page, please let the source responsible for the link know that > the link they have provided does not work for non-Amazon employees. They should be stripped out of the eventual commit messages. ~Andrew
On 4/25/19 5:51 AM, Konrad Rzeszutek Wilk wrote: > On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote: >> When there is no changed function in the generated payload, do not >> create an empty .livepatch.funcs section. Hypervisor code considers >> such payloads as broken and rejects to load them. >> >> Such payloads without any changed functions may appear when only >> hooks are specified. > > Ross, I am going to push this in next week unless you have other thoughts? > Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com> This code change looks OK to me. However: 1) I think that the hypervisor should treat an empty .livepatch.funcs section the same as it treats a non-present .livepatch.funcs section (i.e. it allows it) which would make this change unnecessary. 2) Unless I'm being stupid, I don't see how this change would work anyway. Surely this code at the start of prepare_payload() would fail if the section were missing? sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC); ASSERT(sec); if ( !section_ok(elf, sec, sizeof(*payload->funcs)) ) return -EINVAL; Regards,
> On 29. Apr 2019, at 16:37, Ross Lagerwall <ross.lagerwall@citrix.com> wrote: > > On 4/25/19 5:51 AM, Konrad Rzeszutek Wilk wrote: >> On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote: >>> When there is no changed function in the generated payload, do not >>> create an empty .livepatch.funcs section. Hypervisor code considers >>> such payloads as broken and rejects to load them. >>> >>> Such payloads without any changed functions may appear when only >>> hooks are specified. >> Ross, I am going to push this in next week unless you have other thoughts? > Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com> > > This code change looks OK to me. However: > Thank you for reviewing. > 1) I think that the hypervisor should treat an empty .livepatch.funcs section the same as it treats a non-present .livepatch.funcs section (i.e. it allows it) which would make this change unnecessary. > I do not have a strong opinion here, but it felt unnecessary (and a bit confusing) to generate an empty section. Also I did not want to touch hypervisor code for this. > 2) Unless I'm being stupid, I don't see how this change would work anyway. Surely this code at the start of prepare_payload() would fail if the section were missing? > > sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC); > ASSERT(sec); > if ( !section_ok(elf, sec, sizeof(*payload->funcs)) ) > return -EINVAL; > In my soon-to-be-upstreamed backlog I have the following commit: livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence Where I actually make use of this new functionality. The main idea behind this change, was to enable generation of hooks-only hotpatch modules (i.e. modules that does not patch anything, just trigger actions). > Regards, > -- > Ross Lagerwall Best Regards, Pawel Wieczorkiewicz Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich Ust-ID: DE 289 237 879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
diff --git a/create-diff-object.c b/create-diff-object.c index 82f777e..af2245c 100644 --- a/create-diff-object.c +++ b/create-diff-object.c @@ -1744,6 +1744,11 @@ static void livepatch_create_patches_sections(struct kpatch_elf *kelf, if (sym->type == STT_FUNC && sym->status == CHANGED) nr++; + if (nr == 0) { + log_debug("No changed functions found. Skipping .livepatch.funcs section creation\n"); + return; + } + /* create text/rela section pair */ sec = create_section_pair(kelf, ".livepatch.funcs", sizeof(*funcs), nr); relasec = sec->rela;