Re: [Xen-devel] [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 22 Apr 2015, Jan Beulich wrote:
> >>> On 22.04.15 at 15:57, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > From the description of the problem above, we have two issues:
> > 
> > 1) we don't detect that maxmem is already UINT_MAX*4, so we shouldn't try
> >    to increase it
> > 
> > 2) unsigned int / uint64_t mismatch
> > 
> > 
> > 1) is pretty easy and might just come down to one more if statement in
> > libxl_set_memory_target. Something like:
> > 
> > 
> > #define MAXMEM_MAX_KB ((uint64_t)UINT32_MAX * 4)
> > if ((new_target_memkb - current_target_memkb) > 0 && 
> >     (ptr.max_memkb - new_target_memkb + current_target_memkb) < 
> > ptr.max_memkb)
> > {
> >     /* avoid overflow */
> >    rc = xc_domain_setmaxmem(ctx->xch, domid, MAXMEM_MAX_KB);
> > } else {
> >    rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
> > }
> 
> Which would build into the tool stack an assumption that the
> hypervisor representation is a 32-bit value worth of pages. Not
> really desirable imo. If anything we'd need to do actual overflow
> checking (and saturation, similar to what the hypervisor patch
> does) here too.

Sure, as long as Xen is able to handle maxmem arguments > UINT32_MAX *
4, that it doesn't look like is the case at the moment.



> > 2) is difficult as we have unsigned int in many many places.  Not only
> > we need to fix the libxc interface, but as far as I can tell we also
> > need to fix at least libxl_set_memory_target,
> > libxl__fill_dom0_memory_info, libxl__get_memory_target, all the callers.
> > This is the minimum set of changes I think are required:
> 
> I'll leave that to the tool stack maintainers to decide upon. All I
> really need immediately is that "xl mem-set" works again.

What I am saying is that, as you probably know, this patch is not enough
to get it right.

I have now seen your hypervisor side patch (it would have been better
to have them in a series so that it would have been obvious that they
need each other).

Even after your hypervisor patch and this small libxl change, xl mem-set
might work in your scenario, but libxl still doesn't have the right
behaviour: libxl is unintentionally causing overflows.
xc_domain_setmaxmem should rightfully return errors in those cases, and
libxl would abort the operation. This is not what we want, right?

I think that at the very least you need to add overflow checking in
libxl_set_memory_target.


The other changes I suggested come from:

memorykb = ptr.max_memkb - current_target_memkb + new_target_memkb;

memorykb and ptr.max_memkb might both be uint64_t with your change, but
current_target_memkb and new_target_memkb are not. There could still be
overflow there. However I do understand that they actually refer to the
memory target, not maxmem, so they could be fixed separately. If you
don't want to handle that in your series, that's fine by me.

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