* Patching kthread functions
@ 2020-09-30 15:44 Evgenii Shatokhin
2020-10-01 11:13 ` Miroslav Benes
0 siblings, 1 reply; 10+ messages in thread
From: Evgenii Shatokhin @ 2020-09-30 15:44 UTC (permalink / raw)
To: live-patching
Hi,
I wonder, can livepatch from the current mainline kernel patch the main
functions of kthreads, which are running or sleeping constantly? Are
there any best practices here?
I mean, suppose we have a function which runs in a kthread (passed to
kthread_create()) and is organized like this:
while (!kthread_should_stop()) {
...
DEFINE_WAIT(_wait);
for (;;) {
prepare_to_wait(waitq, &_wait, TASK_INTERRUPTIBLE);
if (we_have_requests_to_process || kthread_should_stop())
break;
schedule();
}
finish_wait(waitq, &_wait);
...
if (we_have_requests_to_process)
process_one_request();
...
}
(The question appeared when I was looking at the following code:
https://src.openvz.org/projects/OVZ/repos/vzkernel/browse/drivers/block/ploop/io_kaio.c?at=refs%2Ftags%2Frh7-3.10.0-1127.8.2.vz7.151.14#478)
The kthread is always running and never exits the kernel.
I could rewrite the function to add klp_update_patch_state() somewhere,
but would it help?
No locks are held right before and after "schedule()", and the thread is
not processing any requests at that point. But even if I place
klp_update_patch_state(), say, just before schedule(), it would just
switch task->patch_state for that kthread. The old function will
continue running, right?
Looks like we can only switch to the patched code of the function at the
beginning, via Ftrace hook. So, if the function is constantly running or
sleeping, it seems, it cannot be live-patched. Is that so? Are there any
workarounds?
Thanks in advance.
Regards,
Evgenii
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-09-30 15:44 Patching kthread functions Evgenii Shatokhin
@ 2020-10-01 11:13 ` Miroslav Benes
2020-10-01 12:43 ` Nicolai Stange
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Miroslav Benes @ 2020-10-01 11:13 UTC (permalink / raw)
To: Evgenii Shatokhin; +Cc: live-patching, pmladek, nstange
On Wed, 30 Sep 2020, Evgenii Shatokhin wrote:
> Hi,
>
> I wonder, can livepatch from the current mainline kernel patch the main
> functions of kthreads, which are running or sleeping constantly? Are there any
> best practices here?
No. It is a "known" limitation, "" because we discussed it a couple of
times (at least with Petr), but it is not documented :(
I wonder if it is really an issue practically. I haven't met a case
yet when we wanted to patch such thing. But yes, you're correct, it is not
possible.
> I mean, suppose we have a function which runs in a kthread (passed to
> kthread_create()) and is organized like this:
>
> while (!kthread_should_stop()) {
> ...
> DEFINE_WAIT(_wait);
> for (;;) {
> prepare_to_wait(waitq, &_wait, TASK_INTERRUPTIBLE);
> if (we_have_requests_to_process || kthread_should_stop())
> break;
> schedule();
> }
> finish_wait(waitq, &_wait);
> ...
> if (we_have_requests_to_process)
> process_one_request();
> ...
> }
>
> (The question appeared when I was looking at the following code:
> https://src.openvz.org/projects/OVZ/repos/vzkernel/browse/drivers/block/ploop/io_kaio.c?at=refs%2Ftags%2Frh7-3.10.0-1127.8.2.vz7.151.14#478)
>
> The kthread is always running and never exits the kernel.
>
> I could rewrite the function to add klp_update_patch_state() somewhere, but
> would it help?
In fact, we used exactly this approach in, now obsolete, kGraft. All
kthreads had to be manually annotated somewhere safe, where safe meant
that the thread could be switched to a new universe without the problem
wrt to calling old/new functions in the loop...
> No locks are held right before and after "schedule()", and the thread is not
> processing any requests at that point.
... like this.
> But even if I place
> klp_update_patch_state(), say, just before schedule(), it would just switch
> task->patch_state for that kthread.
Correct.
> The old function will continue running, right?
Correct. It will, however, call new functions.
> Looks like we can only switch to the patched code of the function at the
> beginning, via Ftrace hook. So, if the function is constantly running or
> sleeping, it seems, it cannot be live-patched.
Yes and no. Normally, a task cannot run indefinitely and if it sleeps, we
can deal with that (thanks to stack checking or signaling/kicking the
task), but kthreads' main loops are special.
> Is that so? Are there any workarounds?
Petr, do you remember the crazy workarounds we talked about? My head is
empty now. And I am sure, Nicolai could come up with something.
Thanks
Miroslav
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-01 11:13 ` Miroslav Benes
@ 2020-10-01 12:43 ` Nicolai Stange
2020-10-01 13:18 ` Evgenii Shatokhin
2020-10-01 13:12 ` Evgenii Shatokhin
2020-10-01 14:46 ` Petr Mladek
2 siblings, 1 reply; 10+ messages in thread
From: Nicolai Stange @ 2020-10-01 12:43 UTC (permalink / raw)
To: Evgenii Shatokhin; +Cc: Miroslav Benes, live-patching, pmladek, nstange
Miroslav Benes <mbenes@suse.cz> writes:
> On Wed, 30 Sep 2020, Evgenii Shatokhin wrote:
>> Is that so? Are there any workarounds?
>
> Petr, do you remember the crazy workarounds we talked about? My head is
> empty now. And I am sure, Nicolai could come up with something.
There might be some clever tricks ycou could play, but that depends on
the diff you want to turn into a livepatch. For example, sometimes it's
possible to livepatch a callee and make it trick the unpatchable caller
into the desired behaviour.
However, in your case it might be easier to simply kill the running
kthread and start it again, e.g. from a ->post_patch() callback. Note
that KLP's callbacks are a bit subtle though, at a minimum you'd
probably also want to implement ->pre_unpatch() to roll everything back
and perhaps also disable (->replace) downgrades by means of the klp_state API.
You can find some good docs on callbacks and klp_state at
Documentation/livepatch/.
Thanks,
Nicolai
--
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Felix Imendörffer
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-01 11:13 ` Miroslav Benes
2020-10-01 12:43 ` Nicolai Stange
@ 2020-10-01 13:12 ` Evgenii Shatokhin
2020-10-02 11:53 ` Miroslav Benes
2020-10-01 14:46 ` Petr Mladek
2 siblings, 1 reply; 10+ messages in thread
From: Evgenii Shatokhin @ 2020-10-01 13:12 UTC (permalink / raw)
To: Miroslav Benes; +Cc: live-patching, pmladek, nstange
On 01.10.2020 14:13, Miroslav Benes wrote:
> On Wed, 30 Sep 2020, Evgenii Shatokhin wrote:
>
>> Hi,
>>
>> I wonder, can livepatch from the current mainline kernel patch the main
>> functions of kthreads, which are running or sleeping constantly? Are there any
>> best practices here?
>
> No. It is a "known" limitation, "" because we discussed it a couple of
> times (at least with Petr), but it is not documented :(
>
> I wonder if it is really an issue practically. I haven't met a case
> yet when we wanted to patch such thing. But yes, you're correct, it is not
> possible.
Well, I have recently encountered such case, with kaio_fsync_thread()
function from our custom kernel, the code at the URL below. Our
customers were interested in a particular bug fix there: there was a
race, potentially leading to data corruption.
We still use the old-style kpatch.ko-based patches for that kernel
version, so we definitely cannot deliver the fix via a live kernel
patch, only a regular kernel update will do. That made me wonder if the
modern livepatch could handle it.
>
>> I mean, suppose we have a function which runs in a kthread (passed to
>> kthread_create()) and is organized like this:
>>
>> while (!kthread_should_stop()) {
>> ...
>> DEFINE_WAIT(_wait);
>> for (;;) {
>> prepare_to_wait(waitq, &_wait, TASK_INTERRUPTIBLE);
>> if (we_have_requests_to_process || kthread_should_stop())
>> break;
>> schedule();
>> }
>> finish_wait(waitq, &_wait);
>> ...
>> if (we_have_requests_to_process)
>> process_one_request();
>> ...
>> }
>>
>> (The question appeared when I was looking at the following code:
>> https://src.openvz.org/projects/OVZ/repos/vzkernel/browse/drivers/block/ploop/io_kaio.c?at=refs%2Ftags%2Frh7-3.10.0-1127.8.2.vz7.151.14#478)
>>
>> The kthread is always running and never exits the kernel.
>>
>> I could rewrite the function to add klp_update_patch_state() somewhere, but
>> would it help?
>
> In fact, we used exactly this approach in, now obsolete, kGraft. All
> kthreads had to be manually annotated somewhere safe, where safe meant
> that the thread could be switched to a new universe without the problem
> wrt to calling old/new functions in the loop...
>
>> No locks are held right before and after "schedule()", and the thread is not
>> processing any requests at that point.
>
> ... like this.
>
>> But even if I place
>> klp_update_patch_state(), say, just before schedule(), it would just switch
>> task->patch_state for that kthread.
>
> Correct.
>
>> The old function will continue running, right?
>
> Correct. It will, however, call new functions.
Ah, I see.
So, I guess, our best bet would be to rewrite the thread function so
that it contains just the event loop and calls other non-inline
functions to actually process the requests. And, perhaps, - place
klp_update_patch_state() before schedule().
This will not help with this particular kernel version but could make it
possible to live-patch the request-processing functions in the future
kernel versions. The main thread function will remain unpatchable but it
will call the patched functions once we switch the patch_state for the
thread.
>
>> Looks like we can only switch to the patched code of the function at the
>> beginning, via Ftrace hook. So, if the function is constantly running or
>> sleeping, it seems, it cannot be live-patched.
>
> Yes and no. Normally, a task cannot run indefinitely and if it sleeps, we
> can deal with that (thanks to stack checking or signaling/kicking the
> task), but kthreads' main loops are special.
>
>> Is that so? Are there any workarounds?
>
> Petr, do you remember the crazy workarounds we talked about? My head is
> empty now. And I am sure, Nicolai could come up with something.
Interesting. I am all ears.
Thanks!
Evgenii
>
> Thanks
> Miroslav
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-01 12:43 ` Nicolai Stange
@ 2020-10-01 13:18 ` Evgenii Shatokhin
0 siblings, 0 replies; 10+ messages in thread
From: Evgenii Shatokhin @ 2020-10-01 13:18 UTC (permalink / raw)
To: Nicolai Stange; +Cc: Miroslav Benes, live-patching, pmladek
Hi,
On 01.10.2020 15:43, Nicolai Stange wrote:
> Miroslav Benes <mbenes@suse.cz> writes:
>
>> On Wed, 30 Sep 2020, Evgenii Shatokhin wrote:
>
>>> Is that so? Are there any workarounds?
>>
>> Petr, do you remember the crazy workarounds we talked about? My head is
>> empty now. And I am sure, Nicolai could come up with something.
>
> There might be some clever tricks ycou could play, but that depends on
> the diff you want to turn into a livepatch. For example, sometimes it's
> possible to livepatch a callee and make it trick the unpatchable caller
> into the desired behaviour.
In this particular case, we needed to add a kind of lock/unlock pair
around the request processing part of the function. Unfortunately, there
is no suitable callee there.
>
> However, in your case it might be easier to simply kill the running
> kthread and start it again, e.g. from a ->post_patch() callback. Note
> that KLP's callbacks are a bit subtle though, at a minimum you'd
> probably also want to implement ->pre_unpatch() to roll everything back
> and perhaps also disable (->replace) downgrades by means of the klp_state API.
Interesting.
Something like: block submitting of new requests to the thread, wait for
the old requests to be processed, kill the thread, then start it again,
unblock the requests.
Thanks for the idea!
>
> You can find some good docs on callbacks and klp_state at
> Documentation/livepatch/.
>
> Thanks,
>
> Nicolai
>
Evgenii
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-01 11:13 ` Miroslav Benes
2020-10-01 12:43 ` Nicolai Stange
2020-10-01 13:12 ` Evgenii Shatokhin
@ 2020-10-01 14:46 ` Petr Mladek
2020-10-01 16:34 ` Evgenii Shatokhin
2 siblings, 1 reply; 10+ messages in thread
From: Petr Mladek @ 2020-10-01 14:46 UTC (permalink / raw)
To: Miroslav Benes; +Cc: Evgenii Shatokhin, live-patching, nstange
On Thu 2020-10-01 13:13:07, Miroslav Benes wrote:
> On Wed, 30 Sep 2020, Evgenii Shatokhin wrote:
>
> > Hi,
> >
> > I wonder, can livepatch from the current mainline kernel patch the main
> > functions of kthreads, which are running or sleeping constantly? Are there any
> > best practices here?
>
> No. It is a "known" limitation, "" because we discussed it a couple of
> times (at least with Petr), but it is not documented :(
>
> I wonder if it is really an issue practically. I haven't met a case
> yet when we wanted to patch such thing. But yes, you're correct, it is not
> possible.
>
> > I mean, suppose we have a function which runs in a kthread (passed to
> > kthread_create()) and is organized like this:
> >
> > while (!kthread_should_stop()) {
> > ...
> > DEFINE_WAIT(_wait);
> > for (;;) {
> > prepare_to_wait(waitq, &_wait, TASK_INTERRUPTIBLE);
> > if (we_have_requests_to_process || kthread_should_stop())
> > break;
> > schedule();
> > }
> > finish_wait(waitq, &_wait);
> > ...
> > if (we_have_requests_to_process)
> > process_one_request();
> > ...
> > }
Crazy hack would be to patch only process_one_request() the following way:
1. Put the fixed main loop into the new process_one_request() function.
2. Put the original process_one_request() code into another function,
e.g. do_process_one_request_for_real() and call it from the
fixed loop.
Does it make any sense or should I provide some code?
Be aware that such patch could not get reverted because it would never
leave the new main loop.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-01 14:46 ` Petr Mladek
@ 2020-10-01 16:34 ` Evgenii Shatokhin
0 siblings, 0 replies; 10+ messages in thread
From: Evgenii Shatokhin @ 2020-10-01 16:34 UTC (permalink / raw)
To: Petr Mladek; +Cc: Miroslav Benes, live-patching, nstange
On 01.10.2020 17:46, Petr Mladek wrote:
> On Thu 2020-10-01 13:13:07, Miroslav Benes wrote:
>> On Wed, 30 Sep 2020, Evgenii Shatokhin wrote:
>>
>>> Hi,
>>>
>>> I wonder, can livepatch from the current mainline kernel patch the main
>>> functions of kthreads, which are running or sleeping constantly? Are there any
>>> best practices here?
>>
>> No. It is a "known" limitation, "" because we discussed it a couple of
>> times (at least with Petr), but it is not documented :(
>>
>> I wonder if it is really an issue practically. I haven't met a case
>> yet when we wanted to patch such thing. But yes, you're correct, it is not
>> possible.
>>
>>> I mean, suppose we have a function which runs in a kthread (passed to
>>> kthread_create()) and is organized like this:
>>>
>>> while (!kthread_should_stop()) {
>>> ...
>>> DEFINE_WAIT(_wait);
>>> for (;;) {
>>> prepare_to_wait(waitq, &_wait, TASK_INTERRUPTIBLE);
>>> if (we_have_requests_to_process || kthread_should_stop())
>>> break;
>>> schedule();
>>> }
>>> finish_wait(waitq, &_wait);
>>> ...
>>> if (we_have_requests_to_process)
>>> process_one_request();
>>> ...
>>> }
>
> Crazy hack would be to patch only process_one_request() the following way:
>
> 1. Put the fixed main loop into the new process_one_request() function.
>
> 2. Put the original process_one_request() code into another function,
> e.g. do_process_one_request_for_real() and call it from the
> fixed loop.
>
> Does it make any sense or should I provide some code?
I think, I get the idea, thanks!
So, the thread would loop in the new process_one_request() and, with
necessary care taken, would exit both loops correctly when needed.
In our case (kaio_fsync_thread() at the mentioned URL)
process_one_request() is actually not a separate function but rather
part of the same thread function. So, in this particular case it would
not help. If we refactor the function in some future versions, this
could be an option.
But, yes, such patch would need to remain applied forever, no updates
and no unloads, which is a downside.
>
> Be aware that such patch could not get reverted because it would never
> leave the new main loop.
>
> Best Regards,
> Petr
> .
>
Regards,
Evgenii
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-01 13:12 ` Evgenii Shatokhin
@ 2020-10-02 11:53 ` Miroslav Benes
2020-10-02 12:52 ` Evgenii Shatokhin
0 siblings, 1 reply; 10+ messages in thread
From: Miroslav Benes @ 2020-10-02 11:53 UTC (permalink / raw)
To: Evgenii Shatokhin; +Cc: live-patching, pmladek, nstange
> >> The old function will continue running, right?
> >
> > Correct. It will, however, call new functions.
>
> Ah, I see.
>
> So, I guess, our best bet would be to rewrite the thread function so that it
> contains just the event loop and calls other non-inline functions to actually
> process the requests. And, perhaps, - place klp_update_patch_state() before
> schedule().
Yes, that might be the way. klp_update_patch_state() might not be even
needed. If the callees are live patched, the kthread would be migrated
thanks to stack checking once a task leaves the callee.
> This will not help with this particular kernel version but could make it
> possible to live-patch the request-processing functions in the future kernel
> versions. The main thread function will remain unpatchable but it will call
> the patched functions once we switch the patch_state for the thread.
Yes. The only issue is if the intended fix changes the semantics which is
incompatible between old and new functions (livepatch consistency model is
LEAVE_PATCHED_SET, SWITCH_THREAD, see
https://lore.kernel.org/lkml/20141107140458.GA21774@suse.cz/ for the
explanation if interested).
Regards
Miroslav
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-02 11:53 ` Miroslav Benes
@ 2020-10-02 12:52 ` Evgenii Shatokhin
2020-10-02 13:06 ` Miroslav Benes
0 siblings, 1 reply; 10+ messages in thread
From: Evgenii Shatokhin @ 2020-10-02 12:52 UTC (permalink / raw)
To: Miroslav Benes; +Cc: live-patching, pmladek, nstange
On 02.10.2020 14:53, Miroslav Benes wrote:
>
>>>> The old function will continue running, right?
>>>
>>> Correct. It will, however, call new functions.
>>
>> Ah, I see.
>>
>> So, I guess, our best bet would be to rewrite the thread function so that it
>> contains just the event loop and calls other non-inline functions to actually
>> process the requests. And, perhaps, - place klp_update_patch_state() before
>> schedule().
>
> Yes, that might be the way. klp_update_patch_state() might not be even
> needed. If the callees are live patched, the kthread would be migrated
> thanks to stack checking once a task leaves the callee.
You mean, the task runs the callee, then goes to schedule(), then, while
it waits, livepatch core checks its stack, sees no target functions
there and switches patch_state?
>
>> This will not help with this particular kernel version but could make it
>> possible to live-patch the request-processing functions in the future kernel
>> versions. The main thread function will remain unpatchable but it will call
>> the patched functions once we switch the patch_state for the thread.
>
> Yes. The only issue is if the intended fix changes the semantics which is
> incompatible between old and new functions (livepatch consistency model is
> LEAVE_PATCHED_SET, SWITCH_THREAD, see
> https://lore.kernel.org/lkml/20141107140458.GA21774@suse.cz/ for the
> explanation if interested).
Yes, I have read that.
In our case, the fix only adds a kind of lock/unlock around the part of
the function processing actual requests. The implementation is more
complex, but, essentially, it is lock + unlock. The code not touched by
the patch already handles such locking OK, so it can work both with old
and the new versions of patched functions. And - even if some threads
use the old functions and some - the new ones. So, I guess, it should be
fine.
>
> Regards
> Miroslav
> .
>
Thanks!
Evgenii
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patching kthread functions
2020-10-02 12:52 ` Evgenii Shatokhin
@ 2020-10-02 13:06 ` Miroslav Benes
0 siblings, 0 replies; 10+ messages in thread
From: Miroslav Benes @ 2020-10-02 13:06 UTC (permalink / raw)
To: Evgenii Shatokhin; +Cc: live-patching, pmladek, nstange
On Fri, 2 Oct 2020, Evgenii Shatokhin wrote:
> On 02.10.2020 14:53, Miroslav Benes wrote:
> >
> >>>> The old function will continue running, right?
> >>>
> >>> Correct. It will, however, call new functions.
> >>
> >> Ah, I see.
> >>
> >> So, I guess, our best bet would be to rewrite the thread function so that
> >> it
> >> contains just the event loop and calls other non-inline functions to
> >> actually
> >> process the requests. And, perhaps, - place klp_update_patch_state() before
> >> schedule().
> >
> > Yes, that might be the way. klp_update_patch_state() might not be even
> > needed. If the callees are live patched, the kthread would be migrated
> > thanks to stack checking once a task leaves the callee.
>
> You mean, the task runs the callee, then goes to schedule(), then, while it
> waits, livepatch core checks its stack, sees no target functions there and
> switches patch_state?
Yes. Once the task gets out of the target function (or the set of
functions), its patch state can be changed. If it sleeps (interruptedly)
in the target function, we wake it up so it can get out
(klp_send_signals()).
> >
> >> This will not help with this particular kernel version but could make it
> >> possible to live-patch the request-processing functions in the future
> >> kernel
> >> versions. The main thread function will remain unpatchable but it will call
> >> the patched functions once we switch the patch_state for the thread.
> >
> > Yes. The only issue is if the intended fix changes the semantics which is
> > incompatible between old and new functions (livepatch consistency model is
> > LEAVE_PATCHED_SET, SWITCH_THREAD, see
> > https://lore.kernel.org/lkml/20141107140458.GA21774@suse.cz/ for the
> > explanation if interested).
>
> Yes, I have read that.
>
> In our case, the fix only adds a kind of lock/unlock around the part of the
> function processing actual requests. The implementation is more complex, but,
> essentially, it is lock + unlock. The code not touched by the patch already
> handles such locking OK, so it can work both with old and the new versions of
> patched functions. And - even if some threads use the old functions and some -
> the new ones. So, I guess, it should be fine.
Ok, that should be fine.
Miroslav
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-02 13:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 15:44 Patching kthread functions Evgenii Shatokhin
2020-10-01 11:13 ` Miroslav Benes
2020-10-01 12:43 ` Nicolai Stange
2020-10-01 13:18 ` Evgenii Shatokhin
2020-10-01 13:12 ` Evgenii Shatokhin
2020-10-02 11:53 ` Miroslav Benes
2020-10-02 12:52 ` Evgenii Shatokhin
2020-10-02 13:06 ` Miroslav Benes
2020-10-01 14:46 ` Petr Mladek
2020-10-01 16:34 ` Evgenii Shatokhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).