Re: [Xen-devel] Possible bug in tools/libxl/libxl.c -- Variable passed by reference not set in one possible case

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Oct 10, 2014 at 07:43:10AM -0500, ayush ruia wrote:
> There is one issue which I could not understand -- notably do I want the
> caller's of these functions to be one single transactions. 

Is this a question? It doesn't end with question mark but start with
"do". :-)

Currently they are using different transactions. I think it's incorrect,
and using one transaction is better.

> As it works now,
> in most of the cases the caller first tries to read the value  from the
> xenstore, if it is not present then it will call the function
> libxl__fill_dom0_memory. After the call is successful, the caller function
> will again try to read the value from xenstore . Would it be an issue if
> someone went inbetween and modified the value of target, static_max or
> freemem_slack and there are different values in target during the
> invocation of the function libxl__fill_dom0_memory and when re-reading the
> value from xenstore? I am assuming that is not a problem.
> 

It's similar to the race Ian described. It is a problem, but probably a
benign one, less harmful than the one Ian spotted. But it still needs to
get fixed.

> There are mainly three callers of the function libxl__fill_dom0_memory_info
> in the code.
> 
> 1. libxl__get_free_memory_slack -- Here we read the value from the xenstore
> again after the function successfully returns. This might be a little
> redundant, because we are reading the value again from xenstore, which we
> have already read once and "ideally" should have the value stored in the
> pointer.

Given the current implementation (that libxl__fill_dom0_memory_info has
its own transaction, by the time it returns the value is guaranteed to
be present in xenstore), it's a bit odd libxl__fill_dom0_memory_info
doesn't just return that value in out parameter and the caller just uses
it.

But if you want to make libxl__fill_dom0_memory_info share the same
transaction with its caller this pattern might be still valid.

> 2. libxl_set_memory_target -- Pretty much the same as above. Here
> re-reading the value from the xenstore might make sense as then it starts
> the transaction afresh from reading the value from xenstore again. This
> comes back to my question above -- it seems that the function possibly
> wants to complete the the whole set of calculations and set the
> memory_target in just one transaction, from when it read the value of
> target from xenstore.
> 
> 3. libxl__get_memory_target -- This is the only function that actually uses
> the values of the variables passed by reference, and does not re-read it
> from xenstore again.
> 
> 4593 target = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
> 4594                     "%s/memory/target", dompath));
> 4595     static_max = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
> 4596                     "%s/memory/static-max", dompath));
> 4597
> 4598     rc = ERROR_FAIL;
> 4599     if ((!target || !static_max) && !domid) {
> 4600         rc = libxl__fill_dom0_memory_info(gc, &target_memkb,
> 4601                                           &max_memkb);
> 4602         if (rc < 0)
> 4603             goto out;
> This seems to be plagued with the issue Ian described above. Someone could
> update the value of target or static_max inbetween line 4593 and line 4599.
> This could cause invalid values to be present in the pointers target_memkb
> and max_memkb.
> 
> 
> Out of the three callers of the function libxl__fill_dom0_memory_info(),
> two re-read the value again from the xenstore, and only one uses the
> variables passes by reference-- which might have a race condition.
> 

This is bad.  The way I see to fix this:

1. Refacotr libxl__fill_dom0_memory_info so that it returns
   freemem_slack in an out parameter.
2. Make libxl__fill_dom0_memory_info share the same transaction with its
   caller.
3. Fix all callers to adapt to the new libxl__fill_dom0_memory_info,
   which includes but not limits to change that "goto" style loop to a
   proper loop.

caller ()
{
   for (;;) {
     start transaction
     if domid == 0 {
       if those values don't exist in xenstore {
          fill in dom0 info
       }
     }
     domU path
     commit transaction
     break if success or fatal error
   }
}

It's going to be a small patch series. If you're up for writing any
code, feel free to ask more questions before actually committing serious
efforts to tackle the problem.

Wei.

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