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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3328 bytes --]

On 18/07/14 17:37, Andres Lagar Cavilla wrote:
> On Fri, Jul 18, 2014 at 9:53 AM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com
>     <mailto:andrew.cooper3@citrix.com>>
>     Tested-by: Razvan Cojocaru <rcojocaru@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>
>     CC: Jan Beulich <JBeulich@suse.com <mailto:JBeulich@suse.com>>
>     CC: Tim Deegan <tim@xen.org <mailto:tim@xen.org>>
>     CC: Andres Lagar-Cavilla <andres@lagarcavilla.org
>     <mailto:andres@lagarcavilla.org>>
>     CC: Aravindh Puthiyaparambil <aravindp@cisco.com
>     <mailto:aravindp@cisco.com>>
>
> Reviewed-by: Andres Lagar-Cavilla <andres@lagarcavilla.org
> <mailto:andres@lagarcavilla.org>>
>
> Couple nits below ...
>
>
>     --
>     v3:
>      * Newline on warning
>     v2:
>      * 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 )
>          {
>              req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
>     -        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.
>
> Thanks
> Andres

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.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 6846 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel