| Message ID | 1442507270-67227-1-git-send-email-dingel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/09/2015 18:27, Dominik Dingel wrote: > + preempt_disable(); > + solo = single_task_running(); > + preempt_enable(); > + > cur = ktime_get(); > - } while (single_task_running() && ktime_before(cur, stop)); That's the obvious way to fix it, but the TOCTTOU problem (which was in the buggy code too) is obvious too. :) And the only other user of single_task_running() in drivers/crypto/mcryptd.c has the same issue. In fact, because of the way the function is used ("maybe I can do a little bit of work before going to sleep") it will likely be called many times in a loop. This in turn means that: - any wrong result due to a concurrent process migration would be rectified very soon - preempt_disable()/preempt_enable() can actually be just as expensive or more expensive than single_task_running() itself. Therefore, I wonder if single_task_running() should just use raw_smp_processor_id(). At least the TOCTTOU issue can be clearly documented in the function comment, instead of being hidden behind each of the callers. Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 17 Sep 2015 18:45:00 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 17/09/2015 18:27, Dominik Dingel wrote: > > + preempt_disable(); > > + solo = single_task_running(); > > + preempt_enable(); > > + > > cur = ktime_get(); > > - } while (single_task_running() && ktime_before(cur, stop)); > > That's the obvious way to fix it, but the TOCTTOU problem (which was in > the buggy code too) is obvious too. :) And the only other user of > single_task_running() in drivers/crypto/mcryptd.c has the same issue. Right, worst thing we fly another round. I am not sure about the case for mcryptd.c. I think it might be that the worker there is bounded to one cpu and will not be migrated. I really need to look more in the details what is happening with that worker. > In fact, because of the way the function is used ("maybe I can do a > little bit of work before going to sleep") it will likely be called many > times in a loop. This in turn means that: > > - any wrong result due to a concurrent process migration would be > rectified very soon > > - preempt_disable()/preempt_enable() can actually be just as expensive > or more expensive than single_task_running() itself. > > Therefore, I wonder if single_task_running() should just use > raw_smp_processor_id(). At least the TOCTTOU issue can be clearly > documented in the function comment, instead of being hidden behind each > of the callers. Yes to be useful it should probably call raw_smp_processor_id, and as a lot of code actually already does just does that I do not really see much down sides. @Tim, would it be okay if I change single_task_running and add a specific comment on top? > Thanks, > > Paolo > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-09-17 at 19:07 +0200, Dominik Dingel wrote: > On Thu, 17 Sep 2015 18:45:00 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > On 17/09/2015 18:27, Dominik Dingel wrote: > > > + preempt_disable(); > > > + solo = single_task_running(); > > > + preempt_enable(); > > > + > > > cur = ktime_get(); > > > - } while (single_task_running() && ktime_before(cur, stop)); > > > > That's the obvious way to fix it, but the TOCTTOU problem (which was in > > the buggy code too) is obvious too. :) And the only other user of > > single_task_running() in drivers/crypto/mcryptd.c has the same issue. > > Right, worst thing we fly another round. > > I am not sure about the case for mcryptd.c. I think it might be that the worker > there is bounded to one cpu and will not be migrated. > > I really need to look more in the details what is happening with that worker. > > > In fact, because of the way the function is used ("maybe I can do a > > little bit of work before going to sleep") it will likely be called many > > times in a loop. This in turn means that: > > > > - any wrong result due to a concurrent process migration would be > > rectified very soon > > > > - preempt_disable()/preempt_enable() can actually be just as expensive > > or more expensive than single_task_running() itself. > > > > Therefore, I wonder if single_task_running() should just use > > raw_smp_processor_id(). At least the TOCTTOU issue can be clearly > > documented in the function comment, instead of being hidden behind each > > of the callers. > > Yes to be useful it should probably call raw_smp_processor_id, > and as a lot of code actually already does just does that I do not really see much > down sides. > > @Tim, would it be okay if I change single_task_running and add a specific comment on top? I have no objection to change single_task_running to use raw_smp_processor_id. The worker in mcryptd is bound to the cpu so it has no migration/preemption issue. So it shouldn't care which smp_processor_id version is being used. Yes, please add a comment to alert the user of this caveat should you change single_task_running. Thanks. Tim -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 54534de..ce67dd6 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1971,6 +1971,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu) start = cur = ktime_get(); if (vcpu->halt_poll_ns) { + bool solo; ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns); do { @@ -1982,8 +1983,13 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu) ++vcpu->stat.halt_successful_poll; goto out; } + + preempt_disable(); + solo = single_task_running(); + preempt_enable(); + cur = ktime_get(); - } while (single_task_running() && ktime_before(cur, stop)); + } while (solo && ktime_before(cur, stop)); } for (;;) {
Commit f78195129963 ("kvm: add halt_poll_ns module parameter") calls, with enabled preemption, single_task_running. When CONFIG_DEBUG_PREEMPT is enabled that will result in a debug_smp_processor_id() call. Cc: <stable@vger.kernel.org> # 4.2.x Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com> --- virt/kvm/kvm_main.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)