[livepatch-build-tools,part3,v2,2/3] create-diff-object: Extend patchability verification: STN_UNDEF

Commit Message

Wieczorkiewicz, Pawel Aug. 8, 2019, 12:48 p.m. UTC

During verification check if all sections do not contain any entries
with undefined symbols (STN_UNDEF). This situation can happen when a
section is copied over from its original object to a patched object,
but various symbols related to the section are not copied along.
This scenario happens typically during stacked hotpatches creation
(between 2 different hotpatch modules).

Signed-off-by: Pawel Wieczorkiewicz <[e-mail]>
Reviewed-by: Martin Pohlack <[e-mail]>
Reviewed-by: Bjoern Doebel <[e-mail]>
Reviewed-by: Norbert Manthey <[e-mail]>
Reviewed-by: Andra-Irina Paraschiv <[e-mail]>
---
v2:
* Refactored code by creating a new function kpatch_section_has_undef_symbols()
  for the complicated multi-loop code of kpatch_verify_patchability().
  Hopefully this makes code more readable and easier to maintain.
* Kept lines limits to 80 chars (whereever possible)
* Detection of an undefined symbol counts as a single error
---
 create-diff-object.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Comments

Ross Lagerwall Aug. 16, 2019, 3:16 p.m. UTC | #1

On 8/8/19 1:48 PM, Pawel Wieczorkiewicz wrote:
> During verification check if all sections do not contain any entries
> with undefined symbols (STN_UNDEF). This situation can happen when a
> section is copied over from its original object to a patched object,
> but various symbols related to the section are not copied along.
> This scenario happens typically during stacked hotpatches creation
> (between 2 different hotpatch modules).
> 
> Signed-off-by: Pawel Wieczorkiewicz <[e-mail]>
> Reviewed-by: Martin Pohlack <[e-mail]>
> Reviewed-by: Bjoern Doebel <[e-mail]>
> Reviewed-by: Norbert Manthey <[e-mail]>
> Reviewed-by: Andra-Irina Paraschiv <[e-mail]>
... snip
> +static int kpatch_section_has_undef_symbols(struct kpatch_elf *kelf,
> +					    const struct section *sec)
> +{
> +	int offset, entry_size;
> +	struct rela *rela;
> +	size_t d_size;
> +
> +	entry_size = get_section_entry_size(sec, kelf);
> +	if (entry_size == 0)
> +		return false;
> +
> +	d_size = sec->base->data->d_size;
> +	for ( offset = 0; offset < d_size; offset += entry_size ) {

The coding style doesn't use spaces inside the for loop parentheses.

> +		list_for_each_entry(rela, &sec->relas, list) {
> +			if (rela->offset < offset ||
> +			    rela->offset >= offset + entry_size) {
> +				continue;
> +			}
> +
> +			if ((GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
> +			    (!rela->sym->include && rela->sym->status == SAME)) {
> +				log_normal("section %s has an entry with a STN_UNDEF symbol: %s\n",
> +					   sec->name, rela->sym->name ?: "none");

Perhaps this log message is a bit misleading if only the second 
condition is true? Maybe something slightly more general like "has an 
entry with an undefined symbol".

Otherwise looks OK to me.

Thanks,

Patch

diff --git a/create-diff-object.c b/create-diff-object.c
index 41adb09..e905131 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1531,6 +1531,61 @@  static void kpatch_print_changes(struct kpatch_elf *kelf)
 	}
 }
 
+static inline int get_section_entry_size(const struct section *sec,
+					 struct kpatch_elf *kelf)
+{
+	int entry_size;
+
+	/*
+	 * Base sections typically do not define fixed size elements.
+	 * Detect section's element size in case it's a special section.
+	 * Otherwise, skip it due to an unknown sh_entsize.
+	 */
+	entry_size = sec->sh.sh_entsize;
+	if (entry_size == 0) {
+		struct special_section *special;
+
+		/* Find special section group_size. */
+		for (special = special_sections; special->name; special++) {
+			if (!strcmp(sec->name, special->name))
+				return special->group_size(kelf, 0);
+		}
+        }
+
+	return entry_size;
+}
+
+static int kpatch_section_has_undef_symbols(struct kpatch_elf *kelf,
+					    const struct section *sec)
+{
+	int offset, entry_size;
+	struct rela *rela;
+	size_t d_size;
+
+	entry_size = get_section_entry_size(sec, kelf);
+	if (entry_size == 0)
+		return false;
+
+	d_size = sec->base->data->d_size;
+	for ( offset = 0; offset < d_size; offset += entry_size ) {
+		list_for_each_entry(rela, &sec->relas, list) {
+			if (rela->offset < offset ||
+			    rela->offset >= offset + entry_size) {
+				continue;
+			}
+
+			if ((GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
+			    (!rela->sym->include && rela->sym->status == SAME)) {
+				log_normal("section %s has an entry with a STN_UNDEF symbol: %s\n",
+					   sec->name, rela->sym->name ?: "none");
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
 static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 {
 	struct section *sec;
@@ -1563,6 +1618,17 @@  static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 					errs++;
 				}
 			}
+
+			/* Check if a RELA section does not contain any entries with
+			 * undefined symbols (STN_UNDEF). This situation can happen
+			 * when a section is copied over from its original object to
+			 * a patched object, but various symbols related to the section
+			 * are not copied along.
+			 */
+			if (is_rela_section(sec)) {
+				if (kpatch_section_has_undef_symbols(kelf, sec))
+					errs++;
+			}
 		}
 
 		/*