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

[ayush ruia <ayushruia@xxxxxxxxx>]

That was a question. Thanks for answering it :). I think I am going to wait, if you are thinking of changing the function definition. :)

Regards,

Ayush Ruia.

On Fri, Oct 10, 2014 at 8:58 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:

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