patch-author-guide: more on locks in callbacks#824
Conversation
doc/patch-author-guide.md
Outdated
| Don't forget to protect access to the data as needed. Please note that | ||
| spinlocks and mutexes / sleeping locks can't be used from stop_machine | ||
| context. Also note the pre-patch callback return code will be ignored by the | ||
| kernel's module notifier, so it does not effect the target module or livepatch |
doc/patch-author-guide.md
Outdated
|
|
||
| Similarly, if all patch targets will be unloaded before the kpatch patch | ||
| module is removed or disabled, all unpatch callbacks will execute in process | ||
| context. |
There was a problem hiding this comment.
Unlike 2, I'm not sure 3 is very feasible. It seems unlikely and difficult to enforce with tooling.
There was a problem hiding this comment.
This section was only a listing of possibilities, I never said this one was reasonable :) I agree that it is probably not a useful suggestion, so we could drop it if you like.
There was a problem hiding this comment.
The document is meant to guide, I'm not sure we want to guide people in that direction :-)
doc/patch-author-guide.md
Outdated
| unpatching cannot be stopped at this point and the callbacks cannot spin or | ||
| sleep. | ||
|
|
||
| It may be easiest to omit any unpatching code at this point. |
There was a problem hiding this comment.
I'm not sure what this means. Does it mean to leave the unpatch callbacks empty?
There was a problem hiding this comment.
Yes. My take on it is that undo-ing the patch is probably going to be more trouble than its worth.
There was a problem hiding this comment.
Agreed, but that wasn't clear to me so this statement should be clarified.
doc/patch-author-guide.md
Outdated
|
|
||
| If unpatching and mutual exclusion is required, a separate mechanism, such as | ||
| a sysfs file, should be employed to provide process context to safely | ||
| spin/sleep to take a lock/mutex and unpatch. |
There was a problem hiding this comment.
Interesting idea, but I wonder if this is really feasible (and worth documenting)? It seems like a tricky feature to implement and it would require tooling to support it.
There was a problem hiding this comment.
I think @sm00th was tinkering with this idea for one of our kpatches, so I thought I'd mention it. I agree that kpatch tooling doesn't help us here.
There was a problem hiding this comment.
I'd say let's leave such ideas out of the guide unless it's something we'd be willing to do ourselves (and recommend).
|
|
||
| 2. If it can be assured that all patch targets will be loaded before the | ||
| kpatch patch module, pre-patch callbacks may return -EBUSY if the lock/mutex | ||
| is held to block the patching. |
There was a problem hiding this comment.
side note: I think I'll post a livepatch RFC to get rid of patching modules before they're loaded, by creating patch module depencies on the target objects. It will make so many of these things easier... If it were accepted upstream (probably unlikely) then we could propose something similar for kpatch.ko.
There was a problem hiding this comment.
It would cut down the support/testing matrix... maybe saleable if it really simplifies the code.
One thing to consider -- modules can to fail their init (see #816 for example) if hardware is not in an expected state. This might be a very unlikely scenario, but the module dependencies would require all of those modules to successfully load before the kpatch module. Maybe not worrying about.
The weirdest thing I found about this feature (in livepatch context) was that a failing pre-patch callback had two different outcomes based on how it was called: If the kpatch module is loading, its failure would prevent the kpatch module from loading. If kpatch was already loaded and a module is loading, it would prevent the module from loading. Not completely crazy, but on the other hand, forces the patch writer to understand from which any particular callback will being made.
There was a problem hiding this comment.
Good point about modules failing their init due to missing HW. We should see how common that is, and figure out what to do about it. I get the sense that the vast majority of modules don't do that.
|
Do we want to use this opportunity to brainstorm better solutions and/or kpatch enhancements to support such ideas? I'm just trying to better frame this PR and the recommended text. |
|
Sure, what ideas did you have? I thought the text was headed in the right direction, except for the bits that were more brainstorming and less practical. |
|
I'll have to think on it a little bit, though I'm kinda hesitant to expend a lot of time and energy when livepatch is going to be the future. We went down a lot of roads with last week's kpatch, but ended up concluding that most ideas solved 90% of the problem, "except for this one thing." Maybe if kpatch can be enhanced a little, some of those ideas would have been closer to 100%. |
|
Agreed, livepatch is the future, so we don't need to spend much more time on improving kpatch.ko (other than possibly any changes to make it more livepatch-esque). |
Elaborate on the difficulties in using locks/mutexes from the kpatch callback routines and suggest a few workarounds. Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
joe-lawrence
force-pushed
the
author-guide-callback-locks
branch
from
fd0f4ff to
fb06282
Compare
|
v2:
|
3 participants
Elaborate on the difficulties in using locks/mutexes from the kpatch
callback routines and suggest a few workarounds.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com