[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Possible bug in tools/libxl/libxl.c -- Variable passed by reference not set in one possible case
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |