Static local reworks by julien-thierry · Pull Request #1054 · dynup/kpatch

julien-thierry · 2019-11-01T07:53:51Z

This fixes the error on RHEL8.0 test gcc-static-local-var-5:

ERROR: audit.o: reference to static local variable lock.65414 in audit_log_end was removed
/root/kpatch/kpatch-build/create-diff-object: unreconcilable difference

Mentioned in PR #993 .

jpoimboe · 2019-11-01T14:24:23Z

 }
 #endif

+static bool kpatch_changed_child_needs_parent_profiling(struct symbol *sym)


It might be helpful to add a comment somewhere explaining that .cold children don't have fentry but that .part children often do.

BTW, the terms 'parent' and 'child' in this context have always bothered me because they're so ambigious. What do you think? Any ideas for better naming? Superfunction/subfunction?

I'll add the comment.

Superfunction sounds a bit odd. But subfunction definitely sounds better than child.
Maybe Top function?
Otherwise I actually like the "part" terminology (without being a direct references to the .part. in the name of the symbol). So I would suggest e.g. "function_parts" and "part_of" .

I'm not sure about function_parts/part_of. Maybe parent/child is ok.

jpoimboe · 2019-11-01T15:08:55Z

A unit test (or tests) for this PR would be good too.

@message

This is a test for dynup/kpatch#1054. The following patch was used with a RHEL-8.0 kernel: diff --git a/kernel/audit.c b/kernel/audit.c index e7478cb..ed2546a 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -325,6 +325,12 @@ void audit_panic(const char *message) } } +void kpatch_audit_foo(void) +{ + if (!jiffies) + printk("kpatch audit foo\n"); +} + static inline int audit_rate_check(void) { static unsigned long last_check = 0; @@ -335,6 +341,7 @@ static inline int audit_rate_check(void) unsigned long elapsed; int retval = 0; + kpatch_audit_foo(); if (!audit_rate_limit) return 1; spin_lock_irqsave(&lock, flags); @@ -354,6 +361,11 @@ static inline int audit_rate_check(void) return retval; } +noinline void kpatch_audit_check(void) +{ + audit_rate_check(); +} + /** * audit_log_lost - conditionally log lost audit message event * @message: the message stating reason for lost audit message @@ -400,6 +412,8 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old, struct audit_buffer *ab; int rc = 0; + kpatch_audit_check(); + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); if (unlikely(!ab)) return rc; Signed-off-by: Julien Thierry <jthierry@redhat.com>

@message

This is a test for dynup/kpatch#1054. The following patch was used with a RHEL-8.0 kernel: diff --git a/kernel/audit.c b/kernel/audit.c index e7478cb..ed2546a 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -325,6 +325,12 @@ void audit_panic(const char *message) } } +void kpatch_audit_foo(void) +{ + if (!jiffies) + printk("kpatch audit foo\n"); +} + static inline int audit_rate_check(void) { static unsigned long last_check = 0; @@ -335,6 +341,7 @@ static inline int audit_rate_check(void) unsigned long elapsed; int retval = 0; + kpatch_audit_foo(); if (!audit_rate_limit) return 1; spin_lock_irqsave(&lock, flags); @@ -354,6 +361,11 @@ static inline int audit_rate_check(void) return retval; } +noinline void kpatch_audit_check(void) +{ + audit_rate_check(); +} + /** * audit_log_lost - conditionally log lost audit message event * @message: the message stating reason for lost audit message @@ -400,6 +412,8 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old, struct audit_buffer *ab; int rc = 0; + kpatch_audit_check(); + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); if (unlikely(!ab)) return rc; Signed-off-by: Julien Thierry <jthierry@redhat.com>

This is supposedly fixed by dynup#1054 and should be reinabled once that is merged. Signed-off-by: Artem Savkov <asavkov@redhat.com>

joe-lawrence · 2020-02-17T16:27:25Z

/bump. According to the review comment conversation, @julien-thierry is waiting on @jpoimboe for a few follow ups to his suggestions? And that a unit test has been merged in the meantime.

sm00th

This also needs a commit re-enabling gcc-static-local-var-5 tests across all distros.

Otherwise looks ok.

sm00th · 2020-03-27T09:53:10Z


+/*
+ * Child functions with "*.cold" names don't have _fentry_ calls, but "*.part",
+ * often don't. In the later case, it is not necessary to include the parent


Suggested change

* often don't. In the later case, it is not necessary to include the parent

* often do. In the later case, it is not necessary to include the parent

When a child symbol has changed, the parent symbol is only needed in the output object if the child symbol is unpatchable on its own. This is the case when the child symbol does not have its own profiling call. Only include unchanged parent symbols if their child has changed and the child does not have a function profiling call. Signed-off-by: Julien Thierry <jthierry@redhat.com>

A symbol associated to a function can be split into multiple sub-functions. Currently, kpatch only supports one child per function. Extend this to support an arbitrary number of sub-function per function. Signed-off-by: Julien Thierry <jthierry@redhat.com>

Consider symbols containing .part. in their names as sub-function of the symbols they are derived from (if such symbol still exists in the object file). Signed-off-by: Julien Thierry <jthierry@redhat.com>

A symbol in the original object might get split in several sub-functions in the patched object, which can themselves be bundled (and use a separate rela section). References to local static variables from the original function, might have been moved in one of the sub-functions in the patched object. Look for references to local static variables in the rela section of child symbols in the patched object. Signed-off-by: Julien Thierry <jthierry@redhat.com>

Signed-off-by: Julien Thierry <jthierry@redhat.com>

Reenable previously failing test gcc-static-local-var-5. Signed-off-by: Julien Thierry <jthierry@redhat.com>

julien-thierry · 2020-03-30T14:17:39Z

I've added the enablement of gcc-static-local-var-5.

yhcote · 2020-04-02T19:22:21Z

Taking a look as well at the moment. Will merge later today if it's OK :-)

julien-thierry mentioned this pull request Nov 1, 2019

test/integration: Rebase to RHEL 8.0 #993

Merged

jpoimboe reviewed Nov 1, 2019

View reviewed changes

This was referenced Nov 4, 2019

Add test for static local variables being moved to/from subfunctions dynup/kpatch-unit-test-objs#21

Merged

ERROR: panic.o: reference to static local variable spin_counter.34770 in do_oops_enter_exit.part.0 was removed #1035

Closed

sm00th added a commit to sm00th/kpatch that referenced this pull request Jan 20, 2020

integration: disable rhel8/gcc-static-local-var-5

0444306

This is supposedly fixed by dynup#1054 and should be reinabled once that is merged. Signed-off-by: Artem Savkov <asavkov@redhat.com>

sm00th added a commit to sm00th/kpatch that referenced this pull request Jan 23, 2020

integration: disable rhel8/gcc-static-local-var-5

e9a99a7

This is supposedly fixed by dynup#1054 and should be reinabled once that is merged. Signed-off-by: Artem Savkov <asavkov@redhat.com>

sm00th added a commit to sm00th/kpatch that referenced this pull request Jan 24, 2020

integration: disable rhel8/gcc-static-local-var-5

6f57005

This is supposedly fixed by dynup#1054 and should be reinabled once that is merged. Signed-off-by: Artem Savkov <asavkov@redhat.com>

julien-thierry mentioned this pull request Mar 6, 2020

how about add -fno-reorder-functions in kpatch-build to deal with .text.unlikely.xxx sections? #1080

Closed

julien-thierry force-pushed the static-local-reworks branch from ab34c8a to 396f7e5 Compare March 9, 2020 15:44

sm00th reviewed Mar 27, 2020

View reviewed changes

Julien Thierry added 6 commits March 30, 2020 14:14

kpatch-build: Include .part. symbols as child function

42128ff

Consider symbols containing .part. in their names as sub-function of the symbols they are derived from (if such symbol still exists in the object file). Signed-off-by: Julien Thierry <jthierry@redhat.com>

test/unit: Add unit test for static local moving sections

2265ce6

Signed-off-by: Julien Thierry <jthierry@redhat.com>

test/integration: Reenable test

24fc731

Reenable previously failing test gcc-static-local-var-5. Signed-off-by: Julien Thierry <jthierry@redhat.com>

julien-thierry force-pushed the static-local-reworks branch from 396f7e5 to 24fc731 Compare March 30, 2020 14:13

sm00th approved these changes Mar 30, 2020

View reviewed changes

jpoimboe approved these changes Apr 2, 2020

View reviewed changes

yhcote merged commit e2c66d7 into dynup:master Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Static local reworks#1054

Static local reworks#1054
yhcote merged 6 commits into
dynup:masterfrom
julien-thierry:static-local-reworks

julien-thierry commented Nov 1, 2019

Uh oh!

Uh oh!

jpoimboe Nov 1, 2019

Uh oh!

julien-thierry Nov 4, 2019

Uh oh!

jpoimboe Feb 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpoimboe commented Nov 1, 2019

Uh oh!

joe-lawrence commented Feb 17, 2020

Uh oh!

sm00th left a comment

Uh oh!

sm00th Mar 27, 2020

Uh oh!

julien-thierry commented Mar 30, 2020

Uh oh!

yhcote commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	* often don't. In the later case, it is not necessary to include the parent
	* often do. In the later case, it is not necessary to include the parent

Uh oh!

Conversation

julien-thierry commented Nov 1, 2019

Uh oh!

Uh oh!

jpoimboe Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

julien-thierry Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

jpoimboe Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpoimboe commented Nov 1, 2019

Uh oh!

joe-lawrence commented Feb 17, 2020

Uh oh!

sm00th left a comment

Choose a reason for hiding this comment

Uh oh!

sm00th Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

julien-thierry commented Mar 30, 2020

Uh oh!

yhcote commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants