| Message ID | 20240925084239.85649-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/livepatch: improvements to loading | expand |
On 25.09.2024 10:42, Roger Pau Monne wrote: > The current check for duplicated sections in a payload is not effective. Such > check is done inside a loop that iterates over the sections names, it's > logically impossible for the bitmap to be set more than once. > > The usage of a bitmap in check_patching_sections() has been replaced with a > boolean, since the function just cares that at least one of the special > sections is present. > > No functional change intended, as the check was useless. > > Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.') > Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Just to clarify for my eventual picking up for backporting: Despite the Fixes: tags there's no actual bug being fixed; it's merely code simplification. So no need to backport. Jan
On Wed, Sep 25, 2024 at 10:52:06AM +0200, Jan Beulich wrote: > On 25.09.2024 10:42, Roger Pau Monne wrote: > > The current check for duplicated sections in a payload is not effective. Such > > check is done inside a loop that iterates over the sections names, it's > > logically impossible for the bitmap to be set more than once. > > > > The usage of a bitmap in check_patching_sections() has been replaced with a > > boolean, since the function just cares that at least one of the special > > sections is present. > > > > No functional change intended, as the check was useless. > > > > Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.') > > Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Just to clarify for my eventual picking up for backporting: Despite > the Fixes: tags there's no actual bug being fixed; it's merely code > simplification. So no need to backport. Indeed, no strict bug, just useless code (unless my analysis is wrong). Thanks, Roger.
On 25/09/2024 9:42 am, Roger Pau Monne wrote: > The current check for duplicated sections in a payload is not effective. Such > check is done inside a loop that iterates over the sections names, it's > logically impossible for the bitmap to be set more than once. > > The usage of a bitmap in check_patching_sections() has been replaced with a > boolean, since the function just cares that at least one of the special > sections is present. > > No functional change intended, as the check was useless. > > Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.') > Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Yes I agree. This is useless logic. The only time we could spot such a case is when matching the section table with the string table. For this logic, we always only get whichever answer livepatch_elf_sec_by_name() decides. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and I'm very happy to see some atomics disappear.
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index d93a556bcda2..df41dcce970a 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -473,7 +473,6 @@ static int check_special_sections(const struct livepatch_elf *elf) static const char *const names[] = { ELF_LIVEPATCH_DEPENDS, ELF_LIVEPATCH_XEN_DEPENDS, ELF_BUILD_ID_NOTE}; - DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 }; for ( i = 0; i < ARRAY_SIZE(names); i++ ) { @@ -493,13 +492,6 @@ static int check_special_sections(const struct livepatch_elf *elf) elf->name, names[i]); return -EINVAL; } - - if ( test_and_set_bit(i, found) ) - { - printk(XENLOG_ERR LIVEPATCH "%s: %s was seen more than once\n", - elf->name, names[i]); - return -EINVAL; - } } return 0; @@ -517,7 +509,7 @@ static int check_patching_sections(const struct livepatch_elf *elf) ELF_LIVEPATCH_PREREVERT_HOOK, ELF_LIVEPATCH_REVERT_HOOK, ELF_LIVEPATCH_POSTREVERT_HOOK}; - DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 }; + bool found = false; /* * The patching sections are optional, but at least one @@ -544,16 +536,11 @@ static int check_patching_sections(const struct livepatch_elf *elf) return -EINVAL; } - if ( test_and_set_bit(i, found) ) - { - printk(XENLOG_ERR LIVEPATCH "%s: %s was seen more than once\n", - elf->name, names[i]); - return -EINVAL; - } + found = true; } /* Checking if at least one section is present. */ - if ( bitmap_empty(found, ARRAY_SIZE(names)) ) + if ( !found ) { printk(XENLOG_ERR LIVEPATCH "%s: Nothing to patch. Aborting...\n", elf->name);
The current check for duplicated sections in a payload is not effective. Such check is done inside a loop that iterates over the sections names, it's logically impossible for the bitmap to be set more than once. The usage of a bitmap in check_patching_sections() has been replaced with a boolean, since the function just cares that at least one of the special sections is present. No functional change intended, as the check was useless. Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.') Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/common/livepatch.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-)