Re: [Xen-devel] A bug in Xenbus driver

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

 On 08/25/2010 11:57 AM, Jun Zhu (Intern) wrote:
> Hi all,
>
> I think this is a serious bug, existing in the pvops 2.6.31.13 (also existing 
> in linux 2.6.35);
>
> In the xenbus driver (drivers/xen/xenfs/xenbus.c), the function of 
> xenbus_file_read has a section of source code like this:
>               if (ret != sz) {
>                   if (i == 0)
>                                 i = -EFAULT;
>                         goto out;
>                 }
>               /* Clear out buffer if it has been consumed */
>               if (rb->cons == rb->len) {
>                       list_del(&rb->list);
>                       kfree(rb);
>                       if (list_empty(&u->read_buffers))
>                               break;
>                       rb = list_entry(u->read_buffers.next,
>                                       struct read_buffer, list);
>               }
> It should be like this:
> //              if (ret != sz) {
>                 if (ret != 0) {
>                         if (i == 0)
>                                 i = -EFAULT;
>                         goto out;
>                 }
> This bug makes the read_buffer not be cleared most of the time. If the 
> xenstore client uses PTHREAD to create a thread to receive reply message, the 
> problem will incur. The new thread can not read what it wants to read, since 
> the list is not empty.
>
> I found this problem from the xenstore client xs_watch function. xs_watch 
> creates the new thread on demand. So I recommend that in the function of 
> read_message(xen/tools/xenstore/xs.c), if using thread to receive message, in 
> the case of read fault, it should signal to the listener and print out the 
> error. 

I don't follow your description of the problem.  The full context of the
code in question is this:

                ret = copy_to_user(ubuf + i, &rb->msg[rb->cons], sz);

                i += sz - ret;
                rb->cons += sz - ret;

                if (ret != sz) {
                        if (i == 0)
                                i = -EFAULT;
                        goto out;
                }


copy_to_user returns the number of uncopied bytes, so if ret != sz, then
some part of the data was not copied due to a memory fault problem.

Changing the test to "ret != 0" means that it will return a partial
result (at the end of a page, for example) without reporting an error,
and discarding the uncopied data.

If you're hitting the "ret != sz" case often, then I think there's some
problem with the memory you're passing into read() (either the buffer
itself, or the size argument).

Thanks,
    J

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel