Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Mon 2016-05-16 13:12:50, Josh Poimboeuf wrote:
> On Mon, May 09, 2016 at 02:23:03PM +0200, Petr Mladek wrote:
> > On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> > > On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > > > I have missed that the two commands are called with preemption
> > > > disabled. So, I had the following crazy scenario in mind:
> > > >
> > > >
> > > > CPU0CPU1
> > > >
> > > > klp_enable_patch()
> > > >
> > > > klp_target_state = KLP_PATCHED;
> > > >
> > > > for_each_task()
> > > > set TIF_PENDING_PATCH
> > > >
> > > > # task 123
> > > >
> > > > if (klp_patch_pending(current)
> > > > klp_patch_task(current)
> > > >
> > > > clear TIF_PENDING_PATCH
> > > >
> > > > smp_rmb();
> > > >
> > > > # switch to assembly of
> > > > # klp_patch_task()
> > > >
> > > > mov klp_target_state, %r12
> > > >
> > > > # interrupt and schedule
> > > > # another task
> > > >
> > > >
> > > > klp_reverse_transition();
> > > >
> > > > klp_target_state = KLP_UNPATCHED;
> > > >
> > > > klt_try_to_complete_transition()
> > > >
> > > > task = 123;
> > > > if (task->patch_state == klp_target_state;
> > > > return 0;
> > > >
> > > > => task 123 is in target state and does
> > > > not block conversion
> > > >
> > > > klp_complete_transition()
> > > >
> > > >
> > > > # disable previous patch on the stack
> > > > klp_disable_patch();
> > > >
> > > > klp_target_state = KLP_UNPATCHED;
> > > >
> > > >
> > > > # task 123 gets scheduled again
> > > > lea %r12, task->patch_state
> > > >
> > > > => it happily stores an outdated
> > > > state
> > > >
> > >
> > > Thanks for the clear explanation, this helps a lot.
> > >
> > > > This is why the two functions should get called with preemption
> > > > disabled. We should document it at least. I imagine that we will
> > > > use them later also in another context and nobody will remember
> > > > this crazy scenario.
> > > >
> > > > Well, even disabled preemption does not help. The process on
> > > > CPU1 might be also interrupted by an NMI and do some long
> > > > printk in it.
> > > >
> > > > IMHO, the only safe approach is to call klp_patch_task()
> > > > only for "current" on a safe place. Then this race is harmless.
> > > > The switch happen on a safe place, so that it does not matter
> > > > into which state the process is switched.
> > >
> > > I'm not sure about this solution. When klp_complete_transition() is
> > > called, we need all tasks to be patched, for good. We don't want any of
> > > them to randomly switch to the wrong state at some later time in the
> > > middle of a future patch operation. How would changing klp_patch_task()
> > > to only use "current" prevent that?
> >
> > You are right that it is pity but it really should be safe because
> > it is not entirely random.
> >
> > If the race happens and assign an outdated value, there are two
> > situations:
> >
> > 1. It is assigned when there is not transition in the progress.
> >Then it is OK because it will be ignored by the ftrace handler.
> >The right state will be set before the next transition starts.
> >
> > 2. It is assigned when some other transition is in progress.
> >Then it is OK as long as the function is called from "current".
> >The "wrong" state will be used consistently. It will switch
> >to the right state on another safe state.
>
> Maybe it would be safe, though I'm not entirely convinced. Regardless I
> think we should avoid these situations entirely because they create
> windows for future bugs and races.
Yup, I would prefer a cleaner solution as well.
> > > > By other words, the task state might be updated only
> > > >
> > > >+ by the task itself on a safe place
> > > >+ by other task when the updated on is sleeping on a safe place
> > > >
> > > > This should be well documented and the API should help to avoid
> > > > a misuse.
> > >
> > > I think we could fix it to be safe for future callers who might not have
> > > preemption disabled with a couple of changes to klp_patch_task():
> > > disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> > > before changing the patch state:
> > >
> > > void klp_patch_task(struct task_struct *task)
> > > {
> > > preempt_disable();
> > >
> > > if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> > > task->patch_state =
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Mon 2016-05-16 13:12:50, Josh Poimboeuf wrote:
> On Mon, May 09, 2016 at 02:23:03PM +0200, Petr Mladek wrote:
> > On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> > > On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > > > I have missed that the two commands are called with preemption
> > > > disabled. So, I had the following crazy scenario in mind:
> > > >
> > > >
> > > > CPU0CPU1
> > > >
> > > > klp_enable_patch()
> > > >
> > > > klp_target_state = KLP_PATCHED;
> > > >
> > > > for_each_task()
> > > > set TIF_PENDING_PATCH
> > > >
> > > > # task 123
> > > >
> > > > if (klp_patch_pending(current)
> > > > klp_patch_task(current)
> > > >
> > > > clear TIF_PENDING_PATCH
> > > >
> > > > smp_rmb();
> > > >
> > > > # switch to assembly of
> > > > # klp_patch_task()
> > > >
> > > > mov klp_target_state, %r12
> > > >
> > > > # interrupt and schedule
> > > > # another task
> > > >
> > > >
> > > > klp_reverse_transition();
> > > >
> > > > klp_target_state = KLP_UNPATCHED;
> > > >
> > > > klt_try_to_complete_transition()
> > > >
> > > > task = 123;
> > > > if (task->patch_state == klp_target_state;
> > > > return 0;
> > > >
> > > > => task 123 is in target state and does
> > > > not block conversion
> > > >
> > > > klp_complete_transition()
> > > >
> > > >
> > > > # disable previous patch on the stack
> > > > klp_disable_patch();
> > > >
> > > > klp_target_state = KLP_UNPATCHED;
> > > >
> > > >
> > > > # task 123 gets scheduled again
> > > > lea %r12, task->patch_state
> > > >
> > > > => it happily stores an outdated
> > > > state
> > > >
> > >
> > > Thanks for the clear explanation, this helps a lot.
> > >
> > > > This is why the two functions should get called with preemption
> > > > disabled. We should document it at least. I imagine that we will
> > > > use them later also in another context and nobody will remember
> > > > this crazy scenario.
> > > >
> > > > Well, even disabled preemption does not help. The process on
> > > > CPU1 might be also interrupted by an NMI and do some long
> > > > printk in it.
> > > >
> > > > IMHO, the only safe approach is to call klp_patch_task()
> > > > only for "current" on a safe place. Then this race is harmless.
> > > > The switch happen on a safe place, so that it does not matter
> > > > into which state the process is switched.
> > >
> > > I'm not sure about this solution. When klp_complete_transition() is
> > > called, we need all tasks to be patched, for good. We don't want any of
> > > them to randomly switch to the wrong state at some later time in the
> > > middle of a future patch operation. How would changing klp_patch_task()
> > > to only use "current" prevent that?
> >
> > You are right that it is pity but it really should be safe because
> > it is not entirely random.
> >
> > If the race happens and assign an outdated value, there are two
> > situations:
> >
> > 1. It is assigned when there is not transition in the progress.
> >Then it is OK because it will be ignored by the ftrace handler.
> >The right state will be set before the next transition starts.
> >
> > 2. It is assigned when some other transition is in progress.
> >Then it is OK as long as the function is called from "current".
> >The "wrong" state will be used consistently. It will switch
> >to the right state on another safe state.
>
> Maybe it would be safe, though I'm not entirely convinced. Regardless I
> think we should avoid these situations entirely because they create
> windows for future bugs and races.
Yup, I would prefer a cleaner solution as well.
> > > > By other words, the task state might be updated only
> > > >
> > > >+ by the task itself on a safe place
> > > >+ by other task when the updated on is sleeping on a safe place
> > > >
> > > > This should be well documented and the API should help to avoid
> > > > a misuse.
> > >
> > > I think we could fix it to be safe for future callers who might not have
> > > preemption disabled with a couple of changes to klp_patch_task():
> > > disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> > > before changing the patch state:
> > >
> > > void klp_patch_task(struct task_struct *task)
> > > {
> > > preempt_disable();
> > >
> > > if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> > > task->patch_state =
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Mon, May 09, 2016 at 02:23:03PM +0200, Petr Mladek wrote:
> On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> > On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > > I have missed that the two commands are called with preemption
> > > disabled. So, I had the following crazy scenario in mind:
> > >
> > >
> > > CPU0 CPU1
> > >
> > > klp_enable_patch()
> > >
> > > klp_target_state = KLP_PATCHED;
> > >
> > > for_each_task()
> > > set TIF_PENDING_PATCH
> > >
> > > # task 123
> > >
> > > if (klp_patch_pending(current)
> > > klp_patch_task(current)
> > >
> > > clear TIF_PENDING_PATCH
> > >
> > > smp_rmb();
> > >
> > > # switch to assembly of
> > > # klp_patch_task()
> > >
> > > mov klp_target_state, %r12
> > >
> > > # interrupt and schedule
> > > # another task
> > >
> > >
> > > klp_reverse_transition();
> > >
> > > klp_target_state = KLP_UNPATCHED;
> > >
> > > klt_try_to_complete_transition()
> > >
> > > task = 123;
> > > if (task->patch_state == klp_target_state;
> > > return 0;
> > >
> > > => task 123 is in target state and does
> > > not block conversion
> > >
> > > klp_complete_transition()
> > >
> > >
> > > # disable previous patch on the stack
> > > klp_disable_patch();
> > >
> > > klp_target_state = KLP_UNPATCHED;
> > >
> > >
> > > # task 123 gets scheduled again
> > > lea %r12, task->patch_state
> > >
> > > => it happily stores an outdated
> > > state
> > >
> >
> > Thanks for the clear explanation, this helps a lot.
> >
> > > This is why the two functions should get called with preemption
> > > disabled. We should document it at least. I imagine that we will
> > > use them later also in another context and nobody will remember
> > > this crazy scenario.
> > >
> > > Well, even disabled preemption does not help. The process on
> > > CPU1 might be also interrupted by an NMI and do some long
> > > printk in it.
> > >
> > > IMHO, the only safe approach is to call klp_patch_task()
> > > only for "current" on a safe place. Then this race is harmless.
> > > The switch happen on a safe place, so that it does not matter
> > > into which state the process is switched.
> >
> > I'm not sure about this solution. When klp_complete_transition() is
> > called, we need all tasks to be patched, for good. We don't want any of
> > them to randomly switch to the wrong state at some later time in the
> > middle of a future patch operation. How would changing klp_patch_task()
> > to only use "current" prevent that?
>
> You are right that it is pity but it really should be safe because
> it is not entirely random.
>
> If the race happens and assign an outdated value, there are two
> situations:
>
> 1. It is assigned when there is not transition in the progress.
>Then it is OK because it will be ignored by the ftrace handler.
>The right state will be set before the next transition starts.
>
> 2. It is assigned when some other transition is in progress.
>Then it is OK as long as the function is called from "current".
>The "wrong" state will be used consistently. It will switch
>to the right state on another safe state.
Maybe it would be safe, though I'm not entirely convinced. Regardless I
think we should avoid these situations entirely because they create
windows for future bugs and races.
> > > By other words, the task state might be updated only
> > >
> > >+ by the task itself on a safe place
> > >+ by other task when the updated on is sleeping on a safe place
> > >
> > > This should be well documented and the API should help to avoid
> > > a misuse.
> >
> > I think we could fix it to be safe for future callers who might not have
> > preemption disabled with a couple of changes to klp_patch_task():
> > disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> > before changing the patch state:
> >
> > void klp_patch_task(struct task_struct *task)
> > {
> > preempt_disable();
> >
> > if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> > task->patch_state = READ_ONCE(klp_target_state);
> >
> > preempt_enable();
> > }
>
> It reduces the race window a bit but it is still there. For example,
> NMI still might add a huge delay between reading klp_target_state
> and assigning task->patch state.
Maybe you missed this paragraph from my last email:
| We would also need a synchronize_sched() after the patching is complete,
| either at the end of klp_try_complete_transition() or in
|
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Mon, May 09, 2016 at 02:23:03PM +0200, Petr Mladek wrote:
> On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> > On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > > I have missed that the two commands are called with preemption
> > > disabled. So, I had the following crazy scenario in mind:
> > >
> > >
> > > CPU0 CPU1
> > >
> > > klp_enable_patch()
> > >
> > > klp_target_state = KLP_PATCHED;
> > >
> > > for_each_task()
> > > set TIF_PENDING_PATCH
> > >
> > > # task 123
> > >
> > > if (klp_patch_pending(current)
> > > klp_patch_task(current)
> > >
> > > clear TIF_PENDING_PATCH
> > >
> > > smp_rmb();
> > >
> > > # switch to assembly of
> > > # klp_patch_task()
> > >
> > > mov klp_target_state, %r12
> > >
> > > # interrupt and schedule
> > > # another task
> > >
> > >
> > > klp_reverse_transition();
> > >
> > > klp_target_state = KLP_UNPATCHED;
> > >
> > > klt_try_to_complete_transition()
> > >
> > > task = 123;
> > > if (task->patch_state == klp_target_state;
> > > return 0;
> > >
> > > => task 123 is in target state and does
> > > not block conversion
> > >
> > > klp_complete_transition()
> > >
> > >
> > > # disable previous patch on the stack
> > > klp_disable_patch();
> > >
> > > klp_target_state = KLP_UNPATCHED;
> > >
> > >
> > > # task 123 gets scheduled again
> > > lea %r12, task->patch_state
> > >
> > > => it happily stores an outdated
> > > state
> > >
> >
> > Thanks for the clear explanation, this helps a lot.
> >
> > > This is why the two functions should get called with preemption
> > > disabled. We should document it at least. I imagine that we will
> > > use them later also in another context and nobody will remember
> > > this crazy scenario.
> > >
> > > Well, even disabled preemption does not help. The process on
> > > CPU1 might be also interrupted by an NMI and do some long
> > > printk in it.
> > >
> > > IMHO, the only safe approach is to call klp_patch_task()
> > > only for "current" on a safe place. Then this race is harmless.
> > > The switch happen on a safe place, so that it does not matter
> > > into which state the process is switched.
> >
> > I'm not sure about this solution. When klp_complete_transition() is
> > called, we need all tasks to be patched, for good. We don't want any of
> > them to randomly switch to the wrong state at some later time in the
> > middle of a future patch operation. How would changing klp_patch_task()
> > to only use "current" prevent that?
>
> You are right that it is pity but it really should be safe because
> it is not entirely random.
>
> If the race happens and assign an outdated value, there are two
> situations:
>
> 1. It is assigned when there is not transition in the progress.
>Then it is OK because it will be ignored by the ftrace handler.
>The right state will be set before the next transition starts.
>
> 2. It is assigned when some other transition is in progress.
>Then it is OK as long as the function is called from "current".
>The "wrong" state will be used consistently. It will switch
>to the right state on another safe state.
Maybe it would be safe, though I'm not entirely convinced. Regardless I
think we should avoid these situations entirely because they create
windows for future bugs and races.
> > > By other words, the task state might be updated only
> > >
> > >+ by the task itself on a safe place
> > >+ by other task when the updated on is sleeping on a safe place
> > >
> > > This should be well documented and the API should help to avoid
> > > a misuse.
> >
> > I think we could fix it to be safe for future callers who might not have
> > preemption disabled with a couple of changes to klp_patch_task():
> > disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> > before changing the patch state:
> >
> > void klp_patch_task(struct task_struct *task)
> > {
> > preempt_disable();
> >
> > if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> > task->patch_state = READ_ONCE(klp_target_state);
> >
> > preempt_enable();
> > }
>
> It reduces the race window a bit but it is still there. For example,
> NMI still might add a huge delay between reading klp_target_state
> and assigning task->patch state.
Maybe you missed this paragraph from my last email:
| We would also need a synchronize_sched() after the patching is complete,
| either at the end of klp_try_complete_transition() or in
|
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > I have missed that the two commands are called with preemption
> > disabled. So, I had the following crazy scenario in mind:
> >
> >
> > CPU0CPU1
> >
> > klp_enable_patch()
> >
> > klp_target_state = KLP_PATCHED;
> >
> > for_each_task()
> > set TIF_PENDING_PATCH
> >
> > # task 123
> >
> > if (klp_patch_pending(current)
> > klp_patch_task(current)
> >
> > clear TIF_PENDING_PATCH
> >
> > smp_rmb();
> >
> > # switch to assembly of
> > # klp_patch_task()
> >
> > mov klp_target_state, %r12
> >
> > # interrupt and schedule
> > # another task
> >
> >
> > klp_reverse_transition();
> >
> > klp_target_state = KLP_UNPATCHED;
> >
> > klt_try_to_complete_transition()
> >
> > task = 123;
> > if (task->patch_state == klp_target_state;
> > return 0;
> >
> > => task 123 is in target state and does
> > not block conversion
> >
> > klp_complete_transition()
> >
> >
> > # disable previous patch on the stack
> > klp_disable_patch();
> >
> > klp_target_state = KLP_UNPATCHED;
> >
> >
> > # task 123 gets scheduled again
> > lea %r12, task->patch_state
> >
> > => it happily stores an outdated
> > state
> >
>
> Thanks for the clear explanation, this helps a lot.
>
> > This is why the two functions should get called with preemption
> > disabled. We should document it at least. I imagine that we will
> > use them later also in another context and nobody will remember
> > this crazy scenario.
> >
> > Well, even disabled preemption does not help. The process on
> > CPU1 might be also interrupted by an NMI and do some long
> > printk in it.
> >
> > IMHO, the only safe approach is to call klp_patch_task()
> > only for "current" on a safe place. Then this race is harmless.
> > The switch happen on a safe place, so that it does not matter
> > into which state the process is switched.
>
> I'm not sure about this solution. When klp_complete_transition() is
> called, we need all tasks to be patched, for good. We don't want any of
> them to randomly switch to the wrong state at some later time in the
> middle of a future patch operation. How would changing klp_patch_task()
> to only use "current" prevent that?
You are right that it is pity but it really should be safe because
it is not entirely random.
If the race happens and assign an outdated value, there are two
situations:
1. It is assigned when there is not transition in the progress.
Then it is OK because it will be ignored by the ftrace handler.
The right state will be set before the next transition starts.
2. It is assigned when some other transition is in progress.
Then it is OK as long as the function is called from "current".
The "wrong" state will be used consistently. It will switch
to the right state on another safe state.
> > By other words, the task state might be updated only
> >
> >+ by the task itself on a safe place
> >+ by other task when the updated on is sleeping on a safe place
> >
> > This should be well documented and the API should help to avoid
> > a misuse.
>
> I think we could fix it to be safe for future callers who might not have
> preemption disabled with a couple of changes to klp_patch_task():
> disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> before changing the patch state:
>
> void klp_patch_task(struct task_struct *task)
> {
> preempt_disable();
>
> if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> task->patch_state = READ_ONCE(klp_target_state);
>
> preempt_enable();
> }
It reduces the race window a bit but it is still there. For example,
NMI still might add a huge delay between reading klp_target_state
and assigning task->patch state.
What about the following?
/*
* This function might assign an outdated value if the transaction
`* is reverted and finalized in parallel. But it is safe. If the value
* is assigned outside of a transaction, it is ignored and the next
* transaction will set the right one. Or if it gets assigned
* inside another transaction, it will repeat the cycle and
* set the right state.
*/
void klp_update_current_patch_state()
{
while (test_and_clear_tsk_thread_flag(current, TIF_PATCH_PENDING))
current->patch_state = READ_ONCE(klp_target_state);
}
Note that the disabled preemption helped only partially,
so I think that
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Fri 2016-05-06 07:38:55, Josh Poimboeuf wrote:
> On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> > I have missed that the two commands are called with preemption
> > disabled. So, I had the following crazy scenario in mind:
> >
> >
> > CPU0CPU1
> >
> > klp_enable_patch()
> >
> > klp_target_state = KLP_PATCHED;
> >
> > for_each_task()
> > set TIF_PENDING_PATCH
> >
> > # task 123
> >
> > if (klp_patch_pending(current)
> > klp_patch_task(current)
> >
> > clear TIF_PENDING_PATCH
> >
> > smp_rmb();
> >
> > # switch to assembly of
> > # klp_patch_task()
> >
> > mov klp_target_state, %r12
> >
> > # interrupt and schedule
> > # another task
> >
> >
> > klp_reverse_transition();
> >
> > klp_target_state = KLP_UNPATCHED;
> >
> > klt_try_to_complete_transition()
> >
> > task = 123;
> > if (task->patch_state == klp_target_state;
> > return 0;
> >
> > => task 123 is in target state and does
> > not block conversion
> >
> > klp_complete_transition()
> >
> >
> > # disable previous patch on the stack
> > klp_disable_patch();
> >
> > klp_target_state = KLP_UNPATCHED;
> >
> >
> > # task 123 gets scheduled again
> > lea %r12, task->patch_state
> >
> > => it happily stores an outdated
> > state
> >
>
> Thanks for the clear explanation, this helps a lot.
>
> > This is why the two functions should get called with preemption
> > disabled. We should document it at least. I imagine that we will
> > use them later also in another context and nobody will remember
> > this crazy scenario.
> >
> > Well, even disabled preemption does not help. The process on
> > CPU1 might be also interrupted by an NMI and do some long
> > printk in it.
> >
> > IMHO, the only safe approach is to call klp_patch_task()
> > only for "current" on a safe place. Then this race is harmless.
> > The switch happen on a safe place, so that it does not matter
> > into which state the process is switched.
>
> I'm not sure about this solution. When klp_complete_transition() is
> called, we need all tasks to be patched, for good. We don't want any of
> them to randomly switch to the wrong state at some later time in the
> middle of a future patch operation. How would changing klp_patch_task()
> to only use "current" prevent that?
You are right that it is pity but it really should be safe because
it is not entirely random.
If the race happens and assign an outdated value, there are two
situations:
1. It is assigned when there is not transition in the progress.
Then it is OK because it will be ignored by the ftrace handler.
The right state will be set before the next transition starts.
2. It is assigned when some other transition is in progress.
Then it is OK as long as the function is called from "current".
The "wrong" state will be used consistently. It will switch
to the right state on another safe state.
> > By other words, the task state might be updated only
> >
> >+ by the task itself on a safe place
> >+ by other task when the updated on is sleeping on a safe place
> >
> > This should be well documented and the API should help to avoid
> > a misuse.
>
> I think we could fix it to be safe for future callers who might not have
> preemption disabled with a couple of changes to klp_patch_task():
> disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
> before changing the patch state:
>
> void klp_patch_task(struct task_struct *task)
> {
> preempt_disable();
>
> if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
> task->patch_state = READ_ONCE(klp_target_state);
>
> preempt_enable();
> }
It reduces the race window a bit but it is still there. For example,
NMI still might add a huge delay between reading klp_target_state
and assigning task->patch state.
What about the following?
/*
* This function might assign an outdated value if the transaction
`* is reverted and finalized in parallel. But it is safe. If the value
* is assigned outside of a transaction, it is ignored and the next
* transaction will set the right one. Or if it gets assigned
* inside another transaction, it will repeat the cycle and
* set the right state.
*/
void klp_update_current_patch_state()
{
while (test_and_clear_tsk_thread_flag(current, TIF_PATCH_PENDING))
current->patch_state = READ_ONCE(klp_target_state);
}
Note that the disabled preemption helped only partially,
so I think that
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> I have missed that the two commands are called with preemption
> disabled. So, I had the following crazy scenario in mind:
>
>
> CPU0 CPU1
>
> klp_enable_patch()
>
> klp_target_state = KLP_PATCHED;
>
> for_each_task()
> set TIF_PENDING_PATCH
>
> # task 123
>
> if (klp_patch_pending(current)
> klp_patch_task(current)
>
> clear TIF_PENDING_PATCH
>
> smp_rmb();
>
> # switch to assembly of
> # klp_patch_task()
>
> mov klp_target_state, %r12
>
> # interrupt and schedule
> # another task
>
>
> klp_reverse_transition();
>
> klp_target_state = KLP_UNPATCHED;
>
> klt_try_to_complete_transition()
>
> task = 123;
> if (task->patch_state == klp_target_state;
> return 0;
>
> => task 123 is in target state and does
> not block conversion
>
> klp_complete_transition()
>
>
> # disable previous patch on the stack
> klp_disable_patch();
>
> klp_target_state = KLP_UNPATCHED;
>
>
> # task 123 gets scheduled again
> lea %r12, task->patch_state
>
> => it happily stores an outdated
> state
>
Thanks for the clear explanation, this helps a lot.
> This is why the two functions should get called with preemption
> disabled. We should document it at least. I imagine that we will
> use them later also in another context and nobody will remember
> this crazy scenario.
>
> Well, even disabled preemption does not help. The process on
> CPU1 might be also interrupted by an NMI and do some long
> printk in it.
>
> IMHO, the only safe approach is to call klp_patch_task()
> only for "current" on a safe place. Then this race is harmless.
> The switch happen on a safe place, so that it does not matter
> into which state the process is switched.
I'm not sure about this solution. When klp_complete_transition() is
called, we need all tasks to be patched, for good. We don't want any of
them to randomly switch to the wrong state at some later time in the
middle of a future patch operation. How would changing klp_patch_task()
to only use "current" prevent that?
> By other words, the task state might be updated only
>
>+ by the task itself on a safe place
>+ by other task when the updated on is sleeping on a safe place
>
> This should be well documented and the API should help to avoid
> a misuse.
I think we could fix it to be safe for future callers who might not have
preemption disabled with a couple of changes to klp_patch_task():
disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
before changing the patch state:
void klp_patch_task(struct task_struct *task)
{
preempt_disable();
if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
task->patch_state = READ_ONCE(klp_target_state);
preempt_enable();
}
We would also need a synchronize_sched() after the patching is complete,
either at the end of klp_try_complete_transition() or in
klp_complete_transition(). That would make sure that all existing calls
to klp_patch_task() are done.
--
Josh
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Thu, May 05, 2016 at 01:57:01PM +0200, Petr Mladek wrote:
> I have missed that the two commands are called with preemption
> disabled. So, I had the following crazy scenario in mind:
>
>
> CPU0 CPU1
>
> klp_enable_patch()
>
> klp_target_state = KLP_PATCHED;
>
> for_each_task()
> set TIF_PENDING_PATCH
>
> # task 123
>
> if (klp_patch_pending(current)
> klp_patch_task(current)
>
> clear TIF_PENDING_PATCH
>
> smp_rmb();
>
> # switch to assembly of
> # klp_patch_task()
>
> mov klp_target_state, %r12
>
> # interrupt and schedule
> # another task
>
>
> klp_reverse_transition();
>
> klp_target_state = KLP_UNPATCHED;
>
> klt_try_to_complete_transition()
>
> task = 123;
> if (task->patch_state == klp_target_state;
> return 0;
>
> => task 123 is in target state and does
> not block conversion
>
> klp_complete_transition()
>
>
> # disable previous patch on the stack
> klp_disable_patch();
>
> klp_target_state = KLP_UNPATCHED;
>
>
> # task 123 gets scheduled again
> lea %r12, task->patch_state
>
> => it happily stores an outdated
> state
>
Thanks for the clear explanation, this helps a lot.
> This is why the two functions should get called with preemption
> disabled. We should document it at least. I imagine that we will
> use them later also in another context and nobody will remember
> this crazy scenario.
>
> Well, even disabled preemption does not help. The process on
> CPU1 might be also interrupted by an NMI and do some long
> printk in it.
>
> IMHO, the only safe approach is to call klp_patch_task()
> only for "current" on a safe place. Then this race is harmless.
> The switch happen on a safe place, so that it does not matter
> into which state the process is switched.
I'm not sure about this solution. When klp_complete_transition() is
called, we need all tasks to be patched, for good. We don't want any of
them to randomly switch to the wrong state at some later time in the
middle of a future patch operation. How would changing klp_patch_task()
to only use "current" prevent that?
> By other words, the task state might be updated only
>
>+ by the task itself on a safe place
>+ by other task when the updated on is sleeping on a safe place
>
> This should be well documented and the API should help to avoid
> a misuse.
I think we could fix it to be safe for future callers who might not have
preemption disabled with a couple of changes to klp_patch_task():
disabling preemption and testing/clearing the TIF_PATCH_PENDING flag
before changing the patch state:
void klp_patch_task(struct task_struct *task)
{
preempt_disable();
if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
task->patch_state = READ_ONCE(klp_target_state);
preempt_enable();
}
We would also need a synchronize_sched() after the patching is complete,
either at the end of klp_try_complete_transition() or in
klp_complete_transition(). That would make sure that all existing calls
to klp_patch_task() are done.
--
Josh
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Wed 2016-05-04 12:57:00, Josh Poimboeuf wrote:
> On Wed, May 04, 2016 at 04:48:54PM +0200, Petr Mladek wrote:
> > On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote:
> > > Change livepatch to use a basic per-task consistency model. This is the
> > > foundation which will eventually enable us to patch those ~10% of
> > > security patches which change function or data semantics. This is the
> > > biggest remaining piece needed to make livepatch more generally useful.
> > >
> > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > > new file mode 100644
> > > index 000..92819bb
> > > --- /dev/null
> > > +++ b/kernel/livepatch/transition.c
> > > +/*
> > > + * klp_patch_task() - change the patched state of a task
> > > + * @task:The task to change
> > > + *
> > > + * Switches the patched state of the task to the set of functions in the
> > > target
> > > + * patch state.
> > > + */
> > > +void klp_patch_task(struct task_struct *task)
> > > +{
> > > + clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> > > +
> > > + /*
> > > + * The corresponding write barriers are in klp_init_transition() and
> > > + * klp_reverse_transition(). See the comments there for an explanation.
> > > + */
> > > + smp_rmb();
> > > +
> > > + task->patch_state = klp_target_state;
> > > +}
> > > +
> > > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> > > index bd12c6c..60d633f 100644
> > > --- a/kernel/sched/idle.c
> > > +++ b/kernel/sched/idle.c
> > > @@ -9,6 +9,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > >
> > > @@ -266,6 +267,9 @@ static void cpu_idle_loop(void)
> > >
> > > sched_ttwu_pending();
> > > schedule_preempt_disabled();
> > > +
> > > + if (unlikely(klp_patch_pending(current)))
> > > + klp_patch_task(current);
> > > }
> >
> > Some more ideas from the world of crazy races. I was shaking my head
> > if this was safe or not.
> >
> > The problem might be if the task get rescheduled between the check
> > for the pending stuff or inside the klp_patch_task() function.
> > This will get even more important when we use this construct
> > on some more locations, e.g. in some kthreads.
> >
> > If the task is sleeping on this strange locations, it might assign
> > strange values on strange times.
> >
> > I think that it is safe only because it is called with the
> > 'current' parameter and on a safe locations. It means that
> > the result is always safe and consistent. Also we could assign
> > an outdated value only when sleeping between reading klp_target_state
> > and storing task->patch_state. But if anyone modified
> > klp_target_state at this point, he also set TIF_PENDING_PATCH,
> > so the change will not get lost.
> >
> > I think that we should document that klp_patch_func() must be
> > called only from a safe location from within the affected task.
> >
> > I even suggest to avoid misuse by removing the struct *task_struct
> > parameter. It should always be called with current.
>
> Would the race involve two tasks trying to call klp_patch_task() for the
> same task at the same time? If so I don't think that would be a problem
> since they would both write the same value for task->patch_state.
I have missed that the two commands are called with preemption
disabled. So, I had the following crazy scenario in mind:
CPU0CPU1
klp_enable_patch()
klp_target_state = KLP_PATCHED;
for_each_task()
set TIF_PENDING_PATCH
# task 123
if (klp_patch_pending(current)
klp_patch_task(current)
clear TIF_PENDING_PATCH
smp_rmb();
# switch to assembly of
# klp_patch_task()
mov klp_target_state, %r12
# interrupt and schedule
# another task
klp_reverse_transition();
klp_target_state = KLP_UNPATCHED;
klt_try_to_complete_transition()
task = 123;
if (task->patch_state == klp_target_state;
return 0;
=> task 123 is in target state and does
not block conversion
klp_complete_transition()
# disable previous patch on the stack
klp_disable_patch();
klp_target_state = KLP_UNPATCHED;
# task 123 gets scheduled again
lea %r12, task->patch_state
=> it happily stores an outdated
state
This is why the two functions should get called with preemption
disabled. We should document it at least. I imagine that we will
use them later also in another context and nobody will remember
this crazy scenario.
Well, even
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Wed 2016-05-04 12:57:00, Josh Poimboeuf wrote:
> On Wed, May 04, 2016 at 04:48:54PM +0200, Petr Mladek wrote:
> > On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote:
> > > Change livepatch to use a basic per-task consistency model. This is the
> > > foundation which will eventually enable us to patch those ~10% of
> > > security patches which change function or data semantics. This is the
> > > biggest remaining piece needed to make livepatch more generally useful.
> > >
> > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > > new file mode 100644
> > > index 000..92819bb
> > > --- /dev/null
> > > +++ b/kernel/livepatch/transition.c
> > > +/*
> > > + * klp_patch_task() - change the patched state of a task
> > > + * @task:The task to change
> > > + *
> > > + * Switches the patched state of the task to the set of functions in the
> > > target
> > > + * patch state.
> > > + */
> > > +void klp_patch_task(struct task_struct *task)
> > > +{
> > > + clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> > > +
> > > + /*
> > > + * The corresponding write barriers are in klp_init_transition() and
> > > + * klp_reverse_transition(). See the comments there for an explanation.
> > > + */
> > > + smp_rmb();
> > > +
> > > + task->patch_state = klp_target_state;
> > > +}
> > > +
> > > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> > > index bd12c6c..60d633f 100644
> > > --- a/kernel/sched/idle.c
> > > +++ b/kernel/sched/idle.c
> > > @@ -9,6 +9,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > >
> > > @@ -266,6 +267,9 @@ static void cpu_idle_loop(void)
> > >
> > > sched_ttwu_pending();
> > > schedule_preempt_disabled();
> > > +
> > > + if (unlikely(klp_patch_pending(current)))
> > > + klp_patch_task(current);
> > > }
> >
> > Some more ideas from the world of crazy races. I was shaking my head
> > if this was safe or not.
> >
> > The problem might be if the task get rescheduled between the check
> > for the pending stuff or inside the klp_patch_task() function.
> > This will get even more important when we use this construct
> > on some more locations, e.g. in some kthreads.
> >
> > If the task is sleeping on this strange locations, it might assign
> > strange values on strange times.
> >
> > I think that it is safe only because it is called with the
> > 'current' parameter and on a safe locations. It means that
> > the result is always safe and consistent. Also we could assign
> > an outdated value only when sleeping between reading klp_target_state
> > and storing task->patch_state. But if anyone modified
> > klp_target_state at this point, he also set TIF_PENDING_PATCH,
> > so the change will not get lost.
> >
> > I think that we should document that klp_patch_func() must be
> > called only from a safe location from within the affected task.
> >
> > I even suggest to avoid misuse by removing the struct *task_struct
> > parameter. It should always be called with current.
>
> Would the race involve two tasks trying to call klp_patch_task() for the
> same task at the same time? If so I don't think that would be a problem
> since they would both write the same value for task->patch_state.
I have missed that the two commands are called with preemption
disabled. So, I had the following crazy scenario in mind:
CPU0CPU1
klp_enable_patch()
klp_target_state = KLP_PATCHED;
for_each_task()
set TIF_PENDING_PATCH
# task 123
if (klp_patch_pending(current)
klp_patch_task(current)
clear TIF_PENDING_PATCH
smp_rmb();
# switch to assembly of
# klp_patch_task()
mov klp_target_state, %r12
# interrupt and schedule
# another task
klp_reverse_transition();
klp_target_state = KLP_UNPATCHED;
klt_try_to_complete_transition()
task = 123;
if (task->patch_state == klp_target_state;
return 0;
=> task 123 is in target state and does
not block conversion
klp_complete_transition()
# disable previous patch on the stack
klp_disable_patch();
klp_target_state = KLP_UNPATCHED;
# task 123 gets scheduled again
lea %r12, task->patch_state
=> it happily stores an outdated
state
This is why the two functions should get called with preemption
disabled. We should document it at least. I imagine that we will
use them later also in another context and nobody will remember
this crazy scenario.
Well, even
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Wed, May 04, 2016 at 04:48:54PM +0200, Petr Mladek wrote:
> On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote:
> > Change livepatch to use a basic per-task consistency model. This is the
> > foundation which will eventually enable us to patch those ~10% of
> > security patches which change function or data semantics. This is the
> > biggest remaining piece needed to make livepatch more generally useful.
> >
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > new file mode 100644
> > index 000..92819bb
> > --- /dev/null
> > +++ b/kernel/livepatch/transition.c
> > +/*
> > + * klp_patch_task() - change the patched state of a task
> > + * @task: The task to change
> > + *
> > + * Switches the patched state of the task to the set of functions in the
> > target
> > + * patch state.
> > + */
> > +void klp_patch_task(struct task_struct *task)
> > +{
> > + clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> > +
> > + /*
> > +* The corresponding write barriers are in klp_init_transition() and
> > +* klp_reverse_transition(). See the comments there for an explanation.
> > +*/
> > + smp_rmb();
> > +
> > + task->patch_state = klp_target_state;
> > +}
> > +
> > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> > index bd12c6c..60d633f 100644
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -9,6 +9,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> >
> > @@ -266,6 +267,9 @@ static void cpu_idle_loop(void)
> >
> > sched_ttwu_pending();
> > schedule_preempt_disabled();
> > +
> > + if (unlikely(klp_patch_pending(current)))
> > + klp_patch_task(current);
> > }
>
> Some more ideas from the world of crazy races. I was shaking my head
> if this was safe or not.
>
> The problem might be if the task get rescheduled between the check
> for the pending stuff or inside the klp_patch_task() function.
> This will get even more important when we use this construct
> on some more locations, e.g. in some kthreads.
>
> If the task is sleeping on this strange locations, it might assign
> strange values on strange times.
>
> I think that it is safe only because it is called with the
> 'current' parameter and on a safe locations. It means that
> the result is always safe and consistent. Also we could assign
> an outdated value only when sleeping between reading klp_target_state
> and storing task->patch_state. But if anyone modified
> klp_target_state at this point, he also set TIF_PENDING_PATCH,
> so the change will not get lost.
>
> I think that we should document that klp_patch_func() must be
> called only from a safe location from within the affected task.
>
> I even suggest to avoid misuse by removing the struct *task_struct
> parameter. It should always be called with current.
Would the race involve two tasks trying to call klp_patch_task() for the
same task at the same time? If so I don't think that would be a problem
since they would both write the same value for task->patch_state.
(Sorry if I'm being slow, I think I've managed to reach my quota of hard
thinking for the day and I don't exactly follow what the race would be.)
--
Josh
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Wed, May 04, 2016 at 04:48:54PM +0200, Petr Mladek wrote:
> On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote:
> > Change livepatch to use a basic per-task consistency model. This is the
> > foundation which will eventually enable us to patch those ~10% of
> > security patches which change function or data semantics. This is the
> > biggest remaining piece needed to make livepatch more generally useful.
> >
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > new file mode 100644
> > index 000..92819bb
> > --- /dev/null
> > +++ b/kernel/livepatch/transition.c
> > +/*
> > + * klp_patch_task() - change the patched state of a task
> > + * @task: The task to change
> > + *
> > + * Switches the patched state of the task to the set of functions in the
> > target
> > + * patch state.
> > + */
> > +void klp_patch_task(struct task_struct *task)
> > +{
> > + clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> > +
> > + /*
> > +* The corresponding write barriers are in klp_init_transition() and
> > +* klp_reverse_transition(). See the comments there for an explanation.
> > +*/
> > + smp_rmb();
> > +
> > + task->patch_state = klp_target_state;
> > +}
> > +
> > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> > index bd12c6c..60d633f 100644
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -9,6 +9,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> >
> > @@ -266,6 +267,9 @@ static void cpu_idle_loop(void)
> >
> > sched_ttwu_pending();
> > schedule_preempt_disabled();
> > +
> > + if (unlikely(klp_patch_pending(current)))
> > + klp_patch_task(current);
> > }
>
> Some more ideas from the world of crazy races. I was shaking my head
> if this was safe or not.
>
> The problem might be if the task get rescheduled between the check
> for the pending stuff or inside the klp_patch_task() function.
> This will get even more important when we use this construct
> on some more locations, e.g. in some kthreads.
>
> If the task is sleeping on this strange locations, it might assign
> strange values on strange times.
>
> I think that it is safe only because it is called with the
> 'current' parameter and on a safe locations. It means that
> the result is always safe and consistent. Also we could assign
> an outdated value only when sleeping between reading klp_target_state
> and storing task->patch_state. But if anyone modified
> klp_target_state at this point, he also set TIF_PENDING_PATCH,
> so the change will not get lost.
>
> I think that we should document that klp_patch_func() must be
> called only from a safe location from within the affected task.
>
> I even suggest to avoid misuse by removing the struct *task_struct
> parameter. It should always be called with current.
Would the race involve two tasks trying to call klp_patch_task() for the
same task at the same time? If so I don't think that would be a problem
since they would both write the same value for task->patch_state.
(Sorry if I'm being slow, I think I've managed to reach my quota of hard
thinking for the day and I don't exactly follow what the race would be.)
--
Josh
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Wed, 4 May 2016, Petr Mladek wrote: > > + > > + if (unlikely(klp_patch_pending(current))) > > + klp_patch_task(current); > > } > > Some more ideas from the world of crazy races. I was shaking my head > if this was safe or not. > > The problem might be if the task get rescheduled between the check > for the pending stuff The code in question is running with preemption disabled. > or inside the klp_patch_task() function. We must make sure that this function doesn't go to sleep. It's only used to clear the task_struct flag anyway. -- Jiri Kosina SUSE Labs
Re: klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Wed, 4 May 2016, Petr Mladek wrote: > > + > > + if (unlikely(klp_patch_pending(current))) > > + klp_patch_task(current); > > } > > Some more ideas from the world of crazy races. I was shaking my head > if this was safe or not. > > The problem might be if the task get rescheduled between the check > for the pending stuff The code in question is running with preemption disabled. > or inside the klp_patch_task() function. We must make sure that this function doesn't go to sleep. It's only used to clear the task_struct flag anyway. -- Jiri Kosina SUSE Labs
klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote:
> Change livepatch to use a basic per-task consistency model. This is the
> foundation which will eventually enable us to patch those ~10% of
> security patches which change function or data semantics. This is the
> biggest remaining piece needed to make livepatch more generally useful.
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> new file mode 100644
> index 000..92819bb
> --- /dev/null
> +++ b/kernel/livepatch/transition.c
> +/*
> + * klp_patch_task() - change the patched state of a task
> + * @task:The task to change
> + *
> + * Switches the patched state of the task to the set of functions in the
> target
> + * patch state.
> + */
> +void klp_patch_task(struct task_struct *task)
> +{
> + clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> +
> + /*
> + * The corresponding write barriers are in klp_init_transition() and
> + * klp_reverse_transition(). See the comments there for an explanation.
> + */
> + smp_rmb();
> +
> + task->patch_state = klp_target_state;
> +}
> +
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index bd12c6c..60d633f 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -266,6 +267,9 @@ static void cpu_idle_loop(void)
>
> sched_ttwu_pending();
> schedule_preempt_disabled();
> +
> + if (unlikely(klp_patch_pending(current)))
> + klp_patch_task(current);
> }
Some more ideas from the world of crazy races. I was shaking my head
if this was safe or not.
The problem might be if the task get rescheduled between the check
for the pending stuff or inside the klp_patch_task() function.
This will get even more important when we use this construct
on some more locations, e.g. in some kthreads.
If the task is sleeping on this strange locations, it might assign
strange values on strange times.
I think that it is safe only because it is called with the
'current' parameter and on a safe locations. It means that
the result is always safe and consistent. Also we could assign
an outdated value only when sleeping between reading klp_target_state
and storing task->patch_state. But if anyone modified
klp_target_state at this point, he also set TIF_PENDING_PATCH,
so the change will not get lost.
I think that we should document that klp_patch_func() must be
called only from a safe location from within the affected task.
I even suggest to avoid misuse by removing the struct *task_struct
parameter. It should always be called with current.
Best Regards,
Petr
klp_task_patch: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model
On Thu 2016-04-28 15:44:48, Josh Poimboeuf wrote:
> Change livepatch to use a basic per-task consistency model. This is the
> foundation which will eventually enable us to patch those ~10% of
> security patches which change function or data semantics. This is the
> biggest remaining piece needed to make livepatch more generally useful.
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> new file mode 100644
> index 000..92819bb
> --- /dev/null
> +++ b/kernel/livepatch/transition.c
> +/*
> + * klp_patch_task() - change the patched state of a task
> + * @task:The task to change
> + *
> + * Switches the patched state of the task to the set of functions in the
> target
> + * patch state.
> + */
> +void klp_patch_task(struct task_struct *task)
> +{
> + clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
> +
> + /*
> + * The corresponding write barriers are in klp_init_transition() and
> + * klp_reverse_transition(). See the comments there for an explanation.
> + */
> + smp_rmb();
> +
> + task->patch_state = klp_target_state;
> +}
> +
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index bd12c6c..60d633f 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -266,6 +267,9 @@ static void cpu_idle_loop(void)
>
> sched_ttwu_pending();
> schedule_preempt_disabled();
> +
> + if (unlikely(klp_patch_pending(current)))
> + klp_patch_task(current);
> }
Some more ideas from the world of crazy races. I was shaking my head
if this was safe or not.
The problem might be if the task get rescheduled between the check
for the pending stuff or inside the klp_patch_task() function.
This will get even more important when we use this construct
on some more locations, e.g. in some kthreads.
If the task is sleeping on this strange locations, it might assign
strange values on strange times.
I think that it is safe only because it is called with the
'current' parameter and on a safe locations. It means that
the result is always safe and consistent. Also we could assign
an outdated value only when sleeping between reading klp_target_state
and storing task->patch_state. But if anyone modified
klp_target_state at this point, he also set TIF_PENDING_PATCH,
so the change will not get lost.
I think that we should document that klp_patch_func() must be
called only from a safe location from within the affected task.
I even suggest to avoid misuse by removing the struct *task_struct
parameter. It should always be called with current.
Best Regards,
Petr
