Patching kthread functions

Patching kthread functions

[Evgenii Shatokhin]

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

Re: Patching kthread functions

[Miroslav Benes]

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

Re: Patching kthread functions

[Nicolai Stange]

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

Re: Patching kthread functions

[Evgenii Shatokhin]

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

Re: Patching kthread functions

[Evgenii Shatokhin]

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
> .
> 

Re: Patching kthread functions

[Miroslav Benes]

> >> 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

Re: Patching kthread functions

[Evgenii Shatokhin]

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

Re: Patching kthread functions

[Miroslav Benes]

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

Re: Patching kthread functions

[Petr Mladek]

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

Re: Patching kthread functions

[Evgenii Shatokhin]

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