On Wed, Nov 10, 2021 at 12:20:47PM +0000, David Laight wrote:
quoted
quoted
Wouldn't moving part of a function to .text.cold (or .text.unlikely)
generate the same problems with the stack backtrace code as the
.text.fixup section you are removing had??
GCC can already split a function into func and func.cold today (or
worse: func, func.isra.N, func.cold, func.isra.N.cold etc..).
I'm assuming reliable unwind and livepatch know how to deal with this.
They'll have 'proper' function labels at the top - so backtrace
stands a chance.
Indeed you (probably) want it to output "func.irsa.n.cold" rather
than just "func" to help show which copy it is in. >
I guess that livepatch will need separate patches for each
version of the function - which might be 'interesting' if
all the copies actually need patching at the same time.
You'd certainly want a warning if there seemed to be multiple
copies of the function.
Hm, I think there is actually a livepatch problem here.
If the .cold (aka "child") function actually had a fentry hook then we'd
be fine. Then we could just patch both "parent" and "child" functions
at the same time. We already have the ability to patch multiple
functions having dependent interface changes.
But there's no fentry hook in the child, so we can only patch the
parent.
If the child schedules out, and then the parent gets patched, things can
go off-script if the child later jumps back to the unpatched version of
the parent, and then for example the old parent tries to call another
patched function with a since-changed ABI.
Granted, it's like three nested edge cases, so it may not be all that
likely to happen.
Some ideas to fix:
a) Add a field to 'klp_func' which allows the patch module to specify a
function's .cold counterpart?
b) Detect such cold counterparts in klp_enable_patch()? Presumably it
would require searching kallsyms for "<func>.cold", which is somewhat
problematic as there might be duplicates.
c) Update the reliable stacktrace code to mark the stack unreliable if
it has a function with ".cold" in the name?
d) Don't patch functions with .cold counterparts? (Probably not a viable
long-term solution, there are a ton of .cold functions because calls
to printk are marked cold)
e) Disable .cold optimization?
f) Add fentry hooks to .cold functions?
I'm thinking a) seems do-able, and less disruptive / more precise than
most others, but it requires more due diligence on behalf of the patch
creation. It sounds be pretty easy for kpatch-build to handle at least.
--
Josh
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-11-12 09:34:03
On Thu, Nov 11, 2021 at 05:50:03PM -0800, Josh Poimboeuf wrote:
Hm, I think there is actually a livepatch problem here.
I suspected as much, because I couldn't find any code dealing with it
when I looked in a hurry.. :/
Some ideas to fix:
c) Update the reliable stacktrace code to mark the stack unreliable if
it has a function with ".cold" in the name?
Why not simply match func.cold as func in the transition thing? Then
func won't get patched as long as it (or it's .cold part) is in use.
This seems like the natural thing to do.
If there are enough .cold functions, always reporting stacktrace as
unreliable will make progress hard, even though it might be perfectly
safe.
e) Disable .cold optimization?
Yeah, lets not do that. That'll have me lobbying to kill KLP again
because it generates crap code.
On Fri, Nov 12, 2021 at 10:33:36AM +0100, Peter Zijlstra wrote:
On Thu, Nov 11, 2021 at 05:50:03PM -0800, Josh Poimboeuf wrote:
quoted
Hm, I think there is actually a livepatch problem here.
I suspected as much, because I couldn't find any code dealing with it
when I looked in a hurry.. :/
quoted
Some ideas to fix:
quoted
c) Update the reliable stacktrace code to mark the stack unreliable if
it has a function with ".cold" in the name?
Why not simply match func.cold as func in the transition thing? Then
func won't get patched as long as it (or it's .cold part) is in use.
This seems like the natural thing to do.
Well yes, you're basically hinting at my first two options a and b:
a) Add a field to 'klp_func' which allows the patch module to specify a
function's .cold counterpart?
b) Detect such cold counterparts in klp_enable_patch()? Presumably it
would require searching kallsyms for "<func>.cold", which is somewhat
problematic as there might be duplicates.
It's basically a two-step process: 1) match func to .cold if it exists;
2) check for both in klp_check_stack_func(). The above two options are
proposals for the 1st step. The 2nd step was implied.
I think something like that is probably the way to go, but the question
is where to match func to .cold:
- patch creation;
- klp_init_object_loaded(); or
- klp_check_stack_func().
I think the main problem with matching them in the kernel is that you
can't disambiguate duplicate ".cold" symbols without disassembling the
function. Duplicates are rare but they do exist.
Matching them at patch creation time (option a) may work best. At least
with kpatch-build, the tooling already knows about .cold functions, so
explicitly matching them in new klp_func.{cold_name,cold_sympos} fields
would be trivial.
I don't know about other patch creation tooling, but I'd imagine they
also need to know about .cold functions, unless they have that
optimization disabled. Because the func and its .cold counterpart
always need to be patched together.
--
Josh
From: Miroslav Benes <mbenes@suse.cz> Date: 2021-11-15 12:37:02
On Fri, 12 Nov 2021, Josh Poimboeuf wrote:
On Fri, Nov 12, 2021 at 10:33:36AM +0100, Peter Zijlstra wrote:
quoted
On Thu, Nov 11, 2021 at 05:50:03PM -0800, Josh Poimboeuf wrote:
quoted
Hm, I think there is actually a livepatch problem here.
I suspected as much, because I couldn't find any code dealing with it
when I looked in a hurry.. :/
quoted
Some ideas to fix:
quoted
c) Update the reliable stacktrace code to mark the stack unreliable if
it has a function with ".cold" in the name?
Why not simply match func.cold as func in the transition thing? Then
func won't get patched as long as it (or it's .cold part) is in use.
This seems like the natural thing to do.
Well yes, you're basically hinting at my first two options a and b:
a) Add a field to 'klp_func' which allows the patch module to specify a
function's .cold counterpart?
b) Detect such cold counterparts in klp_enable_patch()? Presumably it
would require searching kallsyms for "<func>.cold", which is somewhat
problematic as there might be duplicates.
It's basically a two-step process: 1) match func to .cold if it exists;
2) check for both in klp_check_stack_func(). The above two options are
proposals for the 1st step. The 2nd step was implied.
This reminded me... one of the things I have on my todo list for a long
time is to add an option for a live patch creator to specify functions
which are not contained in the live patch but their presence on stacks
should be checked for. It could save some space in the final live patch
when one would add functions (callers) just because the consistency
requires it.
I took as a convenience feature with a low priority and forgot about it.
The problem above changes it. So should we take the opportunity and
implement both in one step? I wanted to include a list of functions in
on a patch level (klp_patch structure) and klp_check_stack() would just
have more to check.
Miroslav
From: Miroslav Benes <mbenes@suse.cz> Date: 2021-11-15 12:59:56
On Fri, 12 Nov 2021, Josh Poimboeuf wrote:
If the child schedules out, and then the parent gets patched, things can
go off-script if the child later jumps back to the unpatched version of
the parent, and then for example the old parent tries to call another
patched function with a since-changed ABI.
...
I don't know about other patch creation tooling, but I'd imagine they
also need to know about .cold functions, unless they have that
optimization disabled. Because the func and its .cold counterpart
always need to be patched together.
We, at SUSE, solve the issue differently... the new patched parent would
call that another patched function with a changed ABI statically in a live
patch. So in that example, .cold child would jump back to the unpatched
parent which would then call, also, the unpatched function.
The situation would change if we ever were to have some notion of global
consistency. Then it would be really fragile, so it is worth of improving
this, I think.
Miroslav
From: Joe Lawrence <joe.lawrence@redhat.com> Date: 2021-11-15 13:02:35
On 11/15/21 7:36 AM, Miroslav Benes wrote:
On Fri, 12 Nov 2021, Josh Poimboeuf wrote:
quoted
On Fri, Nov 12, 2021 at 10:33:36AM +0100, Peter Zijlstra wrote:
quoted
On Thu, Nov 11, 2021 at 05:50:03PM -0800, Josh Poimboeuf wrote:
quoted
Hm, I think there is actually a livepatch problem here.
I suspected as much, because I couldn't find any code dealing with it
when I looked in a hurry.. :/
quoted
Some ideas to fix:
quoted
c) Update the reliable stacktrace code to mark the stack unreliable if
it has a function with ".cold" in the name?
Why not simply match func.cold as func in the transition thing? Then
func won't get patched as long as it (or it's .cold part) is in use.
This seems like the natural thing to do.
Well yes, you're basically hinting at my first two options a and b:
a) Add a field to 'klp_func' which allows the patch module to specify a
function's .cold counterpart?
b) Detect such cold counterparts in klp_enable_patch()? Presumably it
would require searching kallsyms for "<func>.cold", which is somewhat
problematic as there might be duplicates.
It's basically a two-step process: 1) match func to .cold if it exists;
2) check for both in klp_check_stack_func(). The above two options are
proposals for the 1st step. The 2nd step was implied.
This reminded me... one of the things I have on my todo list for a long
time is to add an option for a live patch creator to specify functions
which are not contained in the live patch but their presence on stacks
should be checked for. It could save some space in the final live patch
when one would add functions (callers) just because the consistency
requires it.
Yea, I've used this technique once (adding a nop to a function so
kpatch-build would detect and include in klp_funcs[]) to make a set of
changes safer with respect to the consistency model. Making it simpler
to for the livepatch author to say, "I'm not changing foo(), but I don't
want it doing anything while patching a task" sounds reasonable.
I took as a convenience feature with a low priority and forgot about it.
The problem above changes it. So should we take the opportunity and
implement both in one step? I wanted to include a list of functions in
on a patch level (klp_patch structure) and klp_check_stack() would just
have more to check.
As far as I read the original problem, I think it would solve for that,
too, so I would say go for it.
--
Joe
On Mon, Nov 15, 2021 at 08:01:13AM -0500, Joe Lawrence wrote:
quoted
This reminded me... one of the things I have on my todo list for a long
time is to add an option for a live patch creator to specify functions
which are not contained in the live patch but their presence on stacks
should be checked for. It could save some space in the final live patch
when one would add functions (callers) just because the consistency
requires it.
Yea, I've used this technique once (adding a nop to a function so
kpatch-build would detect and include in klp_funcs[]) to make a set of
changes safer with respect to the consistency model. Making it simpler
to for the livepatch author to say, "I'm not changing foo(), but I don't
want it doing anything while patching a task" sounds reasonable.
quoted
I took as a convenience feature with a low priority and forgot about it.
The problem above changes it. So should we take the opportunity and
implement both in one step? I wanted to include a list of functions in
on a patch level (klp_patch structure) and klp_check_stack() would just
have more to check.
As far as I read the original problem, I think it would solve for that,
too, so I would say go for it.
Sounds good to me.
Miroslav, do I understand correctly that you're volunteering to make
this change? ;-)
--
Josh
From: Miroslav Benes <mbenes@suse.cz> Date: 2021-11-16 07:26:10
On Mon, 15 Nov 2021, Josh Poimboeuf wrote:
On Mon, Nov 15, 2021 at 08:01:13AM -0500, Joe Lawrence wrote:
quoted
quoted
This reminded me... one of the things I have on my todo list for a long
time is to add an option for a live patch creator to specify functions
which are not contained in the live patch but their presence on stacks
should be checked for. It could save some space in the final live patch
when one would add functions (callers) just because the consistency
requires it.
Yea, I've used this technique once (adding a nop to a function so
kpatch-build would detect and include in klp_funcs[]) to make a set of
changes safer with respect to the consistency model. Making it simpler
to for the livepatch author to say, "I'm not changing foo(), but I don't
want it doing anything while patching a task" sounds reasonable.
quoted
I took as a convenience feature with a low priority and forgot about it.
The problem above changes it. So should we take the opportunity and
implement both in one step? I wanted to include a list of functions in
on a patch level (klp_patch structure) and klp_check_stack() would just
have more to check.
As far as I read the original problem, I think it would solve for that,
too, so I would say go for it.
Sounds good to me.
Miroslav, do I understand correctly that you're volunteering to make
this change? ;-)
Yes, you do. I am not superfast peterz, so it will take some time, but
I'll be happy to do it :).
Miroslav
On Mon, Nov 15, 2021 at 01:59:36PM +0100, Miroslav Benes wrote:
On Fri, 12 Nov 2021, Josh Poimboeuf wrote:
quoted
If the child schedules out, and then the parent gets patched, things can
go off-script if the child later jumps back to the unpatched version of
the parent, and then for example the old parent tries to call another
patched function with a since-changed ABI.
...
quoted
I don't know about other patch creation tooling, but I'd imagine they
also need to know about .cold functions, unless they have that
optimization disabled. Because the func and its .cold counterpart
always need to be patched together.
We, at SUSE, solve the issue differently... the new patched parent would
call that another patched function with a changed ABI statically in a live
patch. So in that example, .cold child would jump back to the unpatched
parent which would then call, also, the unpatched function.
So if I understand correctly, if a function's ABI changes then you don't
patch it directly, but only patch its callers to call the replacement?
Is there a reason for that?
--
Josh
From: Miroslav Benes <mbenes@suse.cz> Date: 2021-11-18 07:15:58
On Tue, 16 Nov 2021, Josh Poimboeuf wrote:
On Mon, Nov 15, 2021 at 01:59:36PM +0100, Miroslav Benes wrote:
quoted
On Fri, 12 Nov 2021, Josh Poimboeuf wrote:
quoted
If the child schedules out, and then the parent gets patched, things can
go off-script if the child later jumps back to the unpatched version of
the parent, and then for example the old parent tries to call another
patched function with a since-changed ABI.
...
quoted
I don't know about other patch creation tooling, but I'd imagine they
also need to know about .cold functions, unless they have that
optimization disabled. Because the func and its .cold counterpart
always need to be patched together.
We, at SUSE, solve the issue differently... the new patched parent would
call that another patched function with a changed ABI statically in a live
patch. So in that example, .cold child would jump back to the unpatched
parent which would then call, also, the unpatched function.
So if I understand correctly, if a function's ABI changes then you don't
patch it directly, but only patch its callers to call the replacement?
Yes.
Is there a reason for that?
Not really. There were a couple of cases in the past where this was safer
to implement and then it became a habbit, I guess.
[ Nicolai CCed, if he wants to add more details ]
Miroslav
From: Petr Mladek <pmladek@suse.com> Date: 2021-11-22 17:46:57
On Thu 2021-11-11 17:50:03, Josh Poimboeuf wrote:
On Wed, Nov 10, 2021 at 12:20:47PM +0000, David Laight wrote:
quoted
quoted
quoted
Wouldn't moving part of a function to .text.cold (or .text.unlikely)
generate the same problems with the stack backtrace code as the
.text.fixup section you are removing had??
GCC can already split a function into func and func.cold today (or
worse: func, func.isra.N, func.cold, func.isra.N.cold etc..).
I'm assuming reliable unwind and livepatch know how to deal with this.
They'll have 'proper' function labels at the top - so backtrace
stands a chance.
Indeed you (probably) want it to output "func.irsa.n.cold" rather
than just "func" to help show which copy it is in. >
I guess that livepatch will need separate patches for each
version of the function - which might be 'interesting' if
all the copies actually need patching at the same time.
You'd certainly want a warning if there seemed to be multiple
copies of the function.
Hm, I think there is actually a livepatch problem here.
If the .cold (aka "child") function actually had a fentry hook then we'd
be fine. Then we could just patch both "parent" and "child" functions
at the same time. We already have the ability to patch multiple
functions having dependent interface changes.
But there's no fentry hook in the child, so we can only patch the
parent.
If the child schedules out, and then the parent gets patched, things can
go off-script if the child later jumps back to the unpatched version of
the parent, and then for example the old parent tries to call another
patched function with a since-changed ABI.
This thread seems to be motivation for the patchset
https://lore.kernel.org/all/20211119090327.12811-1-mbenes@suse.cz/
I am trying to understand the problem here, first. And I am
a bit lost.
How exactly is child called in the above scenario, please?
How could parent get livepatched when child is sleeping?
I imagine it the following way:
parent_func()
fentry
/* some parent code */
jmp child
/* child code */
jmp back_to_parent
/* more parent code */
ret
In the above example, parent_func() would be on stack and could not
get livepatched even when the process is sleeping in the child code.
The livepatching is done via ftrace. Only code with fentry could be
livepatched. And code called via fentry must be visible on stack.
Anyway, this looks to me like yet another compiler optimization where
we need to livepatch the callers. The compiler might produce completely
different optimizations for the new code. I do not see a reasonable
way how to create compatible func, func.isra.N, func.cold,
func.isra.N.cold variants.
Best Regards,
Petr
On Mon, Nov 22, 2021 at 06:46:44PM +0100, Petr Mladek wrote:
On Thu 2021-11-11 17:50:03, Josh Poimboeuf wrote:
quoted
On Wed, Nov 10, 2021 at 12:20:47PM +0000, David Laight wrote:
quoted
quoted
quoted
Wouldn't moving part of a function to .text.cold (or .text.unlikely)
generate the same problems with the stack backtrace code as the
.text.fixup section you are removing had??
GCC can already split a function into func and func.cold today (or
worse: func, func.isra.N, func.cold, func.isra.N.cold etc..).
I'm assuming reliable unwind and livepatch know how to deal with this.
They'll have 'proper' function labels at the top - so backtrace
stands a chance.
Indeed you (probably) want it to output "func.irsa.n.cold" rather
than just "func" to help show which copy it is in. >
I guess that livepatch will need separate patches for each
version of the function - which might be 'interesting' if
all the copies actually need patching at the same time.
You'd certainly want a warning if there seemed to be multiple
copies of the function.
Hm, I think there is actually a livepatch problem here.
If the .cold (aka "child") function actually had a fentry hook then we'd
be fine. Then we could just patch both "parent" and "child" functions
at the same time. We already have the ability to patch multiple
functions having dependent interface changes.
But there's no fentry hook in the child, so we can only patch the
parent.
If the child schedules out, and then the parent gets patched, things can
go off-script if the child later jumps back to the unpatched version of
the parent, and then for example the old parent tries to call another
patched function with a since-changed ABI.
This thread seems to be motivation for the patchset
https://lore.kernel.org/all/20211119090327.12811-1-mbenes@suse.cz/
I am trying to understand the problem here, first. And I am
a bit lost.
How exactly is child called in the above scenario, please?
How could parent get livepatched when child is sleeping?
I imagine it the following way:
parent_func()
fentry
/* some parent code */
jmp child
/* child code */
jmp back_to_parent
/* more parent code */
ret
Right.
In the above example, parent_func() would be on stack and could not
get livepatched even when the process is sleeping in the child code.
The livepatching is done via ftrace. Only code with fentry could be
livepatched. And code called via fentry must be visible on stack.
How would parent_func() be on the stack? If it jumps to the child then
it leaves no trace on the stack.
--
Josh
From: Petr Mladek <pmladek@suse.com> Date: 2021-11-25 08:20:14
On Wed 2021-11-24 09:42:13, Josh Poimboeuf wrote:
On Mon, Nov 22, 2021 at 06:46:44PM +0100, Petr Mladek wrote:
quoted
On Thu 2021-11-11 17:50:03, Josh Poimboeuf wrote:
quoted
On Wed, Nov 10, 2021 at 12:20:47PM +0000, David Laight wrote:
quoted
quoted
quoted
Wouldn't moving part of a function to .text.cold (or .text.unlikely)
generate the same problems with the stack backtrace code as the
.text.fixup section you are removing had??
GCC can already split a function into func and func.cold today (or
worse: func, func.isra.N, func.cold, func.isra.N.cold etc..).
I'm assuming reliable unwind and livepatch know how to deal with this.
They'll have 'proper' function labels at the top - so backtrace
stands a chance.
Indeed you (probably) want it to output "func.irsa.n.cold" rather
than just "func" to help show which copy it is in. >
I guess that livepatch will need separate patches for each
version of the function - which might be 'interesting' if
all the copies actually need patching at the same time.
You'd certainly want a warning if there seemed to be multiple
copies of the function.
Hm, I think there is actually a livepatch problem here.
If the .cold (aka "child") function actually had a fentry hook then we'd
be fine. Then we could just patch both "parent" and "child" functions
at the same time. We already have the ability to patch multiple
functions having dependent interface changes.
But there's no fentry hook in the child, so we can only patch the
parent.
If the child schedules out, and then the parent gets patched, things can
go off-script if the child later jumps back to the unpatched version of
the parent, and then for example the old parent tries to call another
patched function with a since-changed ABI.
This thread seems to be motivation for the patchset
https://lore.kernel.org/all/20211119090327.12811-1-mbenes@suse.cz/
I am trying to understand the problem here, first. And I am
a bit lost.
How exactly is child called in the above scenario, please?
How could parent get livepatched when child is sleeping?
I imagine it the following way:
parent_func()
fentry
/* some parent code */
jmp child
/* child code */
jmp back_to_parent
/* more parent code */
ret
Right.
quoted
In the above example, parent_func() would be on stack and could not
get livepatched even when the process is sleeping in the child code.
The livepatching is done via ftrace. Only code with fentry could be
livepatched. And code called via fentry must be visible on stack.
How would parent_func() be on the stack? If it jumps to the child then
it leaves no trace on the stack.
Grr, sure. It was off-by-one error on my side. /o\
Thanks for explanation.
Best Regards,
Petr