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

[ayush ruia <ayushruia@xxxxxxxxx>]

There is one issue which I could not understand -- notably do I want the caller's of these functions to be one single transactions. 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.

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.

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.

Regards,

Ayush Ruia.

On Fri, Oct 10, 2014 at 6:30 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:

On Fri, 2014-10-10 at 11:54 +0100, Wei Liu wrote:
> On Fri, Oct 10, 2014 at 10:30:06AM +0100, Ian Campbell wrote:
> [...]
> > >
> > > The way the callers use it prevents the issue you described from
> > > happening -- they only call this function when they can't read those
> > > values from xenstore -- if those values are already in xenstore this
> > > function won't get called.
> >
> > At least the callers in libxl__get_free_memory_slack and
> > libxl__get_memory_target look to be racy with someone else writing these
> > nodes to me.
> >
>
>Â libxl__fill_dom0_memory_info uses transaction already, that should
>Â avoid race?

I don't think so.

The caller is:
    read target (perhaps in a transaction)
    if (!target)
        ***
        if ( call fill_dom0_memory(&target) < 0 ) /* uses a transaction */
            return
        use target
    }

If another process dives in at *** and writes target then
fill_dom0_memory will return 0 but not initialise target, but the caller
will still (potentially) use it.

Even if it somehow transpires that through some careful coding in the
caller the use target never actually happens because of some other
reason it way to opaque as it is currently written.

I think the initialisation of current_target_memkb to zero in
libxl_set_memory_target might be masking the issue here, preventing the
caller from (legitimately) complaining about a variable which is used
without initialisation

> > libxl_set_memory_target is a bit unclear but the fact that it ends the
> > transaction right before it calls libxl__fill_dom0_memory_info seems
> > suspicious.
> >
>
> To avoid having a transaction inside another transaction?

I suppose that might have been what the author was thinking, bit it
means that help if you are relying on that transaction to provide
atomicity you lose.

Ian

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