* 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 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-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 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
* 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
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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.