Re: [Qemu-devel] [PATCH] block: Fix race in gluster_finish_aiocb

[Paolo Bonzini]

Il 22/08/2013 16:01, Bharata B Rao ha scritto:
>>>> EFAULT means the buffer address is invalid, I/O error would be EIO, but...
>>>>
>>>>> I remember this was one of the motivations to
>>>>> handle this failure.
>>>>
>>>> ... this write is on the pipe, not on a disk.
>>>
>>> Right. Failure to complete the write on the pipe means that IO done to the
>>> disk didn't complete and hence to the VM it is essentially a disk IO 
>>> failure.
>>
>> The question is, can the write to the pipe actually fail?  Not just "in
>> practice not" according to the documented errors, it seems to me that it
>> cannot.
> 
> May be I am dragging this a bit, but since we are at it, let me make one last
> observation here :)
> 
> The buffer in question here is the GlusterAIOCB pointer that gets passed
> back and forth between QEMU and gluster thro' glfs_pwritev_async and 
> associated
> callback gluster_finish_aiocb.

No, it's not:

  retval = qemu_write_full(s->fds[GLUSTER_FD_WRITE], &acb, sizeof(acb));

The pointer that is passed to qemu_write_full is on the stack; it's the
address of the acb variable.  The _content_ of the buffer (which the
kernel doesn't look at, so it's not relevant for generating EFAULT) is
the GlusterAIOCB pointer.

> Isn't there a possibility that gluster will
> not give us back the same pointer during callback due to some errors on the
> gluster side ? Unlikely but possible ?

Then we would have already crashed on the line before:

  acb->ret = ret;

which writes to the hypothetically corrupted pointer.

But we cannot protect against memory corruption, or against bugs that
violate the API contract at such a fundamental level.  QEMU will
SIGSEGV, but it wouldn't be the first time.

Paolo