Re: [RFC Patch v4 3/9] pass correct file to qemu if we use blktap2

[George Dunlap <george.dunlap@eu.citrix.com>]

On 12/15/2014 10:18 AM, Ian Campbell wrote:
> On Sat, 2014-12-13 at 17:06 +0000, Wei Liu wrote:
>> On Thu, Dec 11, 2014 at 04:45:09PM +0000, George Dunlap wrote:
>>> On Mon, Sep 22, 2014 at 6:59 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
>>>> If we use blktap2, the correct file should be blktap device
>>>> not the pdev_path.
>>>>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> If I'm reading this correctly, this is actually a fairly serious bug
>>> in libxl -- it means that when using backend=tap with HVM domains,
>>> qemu will actually *bypass entirely* the tapdisk process and access
>>> the underlying file directly.
>>>
>>
>> Is it because qemu will corrupt the underlying file so it's very
>> serious? 
> 
> I think it ends up being reasonably safe due to the unplug stuff, in
> general only one of the two paths should be active at any given time and
> there is appropriate flushing/syncing on the unplug etc.
> 
> In fact, I'm not 100% sure this wasn't a deliberate design decision at
> one point to avoid the overhead of doing both qdisk and tapdisk. (I'm
> not going to argue that this still makes sense though).
> 
> If qemu is corrupting files then that's a different matter, which would
> indeed be serious, and which this change might paper over/avoid.

Yes, I think when I wrote that I had forgotten that even with just PV /
emulated there's usually a duplicate datapath that we need to sort out,
and that we already sort that out with the device unplug protocol.

Since half the point of tapdisk was to be able to do things like
snapshotting / other fancy tricks as colo does, not just implement
specific image formats, then if it was deliberate it was kind of dumb.
I'd rather just assume that it was overlooked. :-)

In any case, as it's already fixed in 4.5, I don't think the exact
degree of seriousness matters too much: it's above the threshold for
"should be backported" IMHO, and well below the threshold of "needs the
security response process".  So it should just be backported to 4.4,
4.3, and 4.2  (or some subset thereof, if we're designating stable trees).

 -George