[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

On 18/07/14 17:37, Andres Lagar Cavilla wrote:
On Fri, Jul 18, 2014 at 9:53 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Tested-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Tim Deegan <tim@xxxxxxx>
CC: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
CC: Aravindh Puthiyaparambil <aravindp@xxxxxxxxx>
Reviewed-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

Couple nits below ...

Â* Newline on warning
Â* Allow for multiple pause refcounts
Âxen/arch/x86/hvm/hvm.c     Â|  Â2 +-
Âxen/arch/x86/mm/mem_event.c   |  31 +++++++++++++++++++++++++++++++
Âxen/arch/x86/mm/mem_sharing.c  |  Â4 ++--
Âxen/arch/x86/mm/p2m.c      |  Â8 ++++----
Âxen/include/asm-x86/mem_event.h | Â Â3 +++
Âxen/include/xen/sched.h     |  Â3 +++
Â6 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ef2411c..efd79b8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6113,7 +6113,7 @@ static int hvm_memory_event_traps(long p, uint32_t reason,
  Âif ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync )
- Â Â Â Âvcpu_pause_nosync(v);
+ Â Â Â Âmem_event_vcpu_pause(v);

  Âreq.gfn = value;
diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c
index 40ae841..ba7e71e 100644
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -663,6 +663,37 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
  Âreturn rc;

+void mem_event_vcpu_pause(struct vcpu *v)
+ Â ÂASSERT(v == current);
+ Â Âatomic_inc(&v->mem_event_pause_count);
Nit #1: A buggy toolstack going into an infinite loop could overflow this.

No it can't (or rather, I am fairly certain it can't). This is unconditionally in the context of a running vcpu, given the assertion above. This cannot increment the refcount more than number of mem_events triggered from a single exit into Xen.

+ Â Âvcpu_pause_nosync(v);
+void mem_event_vcpu_unpause(struct vcpu *v)
+ Â Âint old, new, prev = v->mem_event_pause_count.counter;
Nit #2: not fresh in my mind whether atomic is supposed to be signed or unsigned. The flawless comparison below is to take these all as unsigned and check for new > old.


The inards of an atomic_t is signed.

Ideally I would make use of atomic_dec_bounded() from an early version of my pausedomain patch, but Jan requested that I drop that part for easier backport, given only a single consumer.

Given new consumers, that decision might be up for reconsideration.

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.