On Fri, Jul 16, 2010 at 1:55 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Thu, Jul 15, 2010 at 5:20 PM, Anthony Liguori <anth...@codemonkey.ws>
> wrote:
>> On 07/15/2010 10:19 AM, Markus Armbruster wrote:
>>>
>>> Anthony Liguori<anth...@codemonkey.ws> writes:
>>>
>>>
>>>>
>>>> On 07/14/2010 01:43 PM, Christoph Hellwig wrote:
>>>>
>>>>>
>>>>> Err, strong NACK. Please don't start messing with the contents of the
>>>>> data plane, we're getting into real trouble there. It's perfectly
>>>>> valid for a guest to create an image inside an image, and with hardware
>>>>> support for nested virtualization I guess this use case will become
>>>>> rather common, just as it already is on S/390 with VM.
>>>>>
>>>>>
>>>>
>>>> Then we have to remove block format probing.
>>>>
>>>> The two things are fundamentally incompatible.
>>>>
>>>
>>> I agree with Christoph: changing guest writes is a big no-no, and
>>> changing them silently is even worse.
>>>
>>
>> I do sympathize. The problem is we're already doing this. This patch
>> simply changes the behavior to not be a security problem. I've committed it
>> to attempt to resolve that security problem. However, we still have a
>> problem and I don't consider the issue closed.
>>
>>> I could perhaps accept EIO. Elsewhere in this thread you wrote that you
>>> rejected that approach because "it would trigger the stop-on-error
>>> behavior and the result would be far too difficult for a management
>>> tool/person to deal with." I think that would be *far* superior in
>>> fact: it fails spectacularly, immediately and safely instead of silently
>>> corrupting disk contents.
>>>
>>
>> There's really nothing wrong with this type of write, so EIO doesn't solve
>> the problem. While we can argue whether writing zeros or EIO is a "better
>> bad" solution, let's try to figure out a good solution.
>>
>>> The real problem in need of fixing is the unsafe default. You wrote
>>> that "most users want block probing". I disagree. Users want to set up
>>> drives with as little hassle as possible. If format is optional, and
>>> appears to work, why bother specifying it?
>>
>> I really think specifying the format is a burden that is nice to avoid.
>>
>> I have another idea that I hope will solve the problem in a more complete
>> way. The fundamental issue is that it's impossible to probe raw images
>> reliably. We can probe qcow2, vmdk, etc but not raw.
>>
>> So, let's do the following: have raw_probe() always fail. Probing shouldn't
>> be a heuristic, it should be an absolute. We can't prove it's a raw image,
>> so we should always fail.
>>
>> To accomodate current use-cases with raw, let's introduce a new format
>> called "probed_raw". probed_raw's semantics will be the following:
>>
>> The signature of a probed_raw will be ~{'QFI\xfb', 'VMDK', 'COWD', 'OOOM',
>> ...}. If the signature is 'QRAW', then instead of reading the first sector
>> at offset 0, we read the first sector at offset LENGTH. If the signature is
>> 'QRAW', LENGTH is computed by calculating FILE_SIZE - 512.
>>
>> For probed_raw, write requests to sector 0 are checked. If the first four
>> bytes is an invalid probed_raw signature or QRAW, we write a QRAW signature
>> to file offset 0 and copy the first sector to the end of the file
>> redirecting reads and writes to the end of file.
>>
>> An approach like this has the following properties:
>>
>> 1) We can make the bdrv_probe check 100% reliable and return a boolean.
>> 2) In the cases where we known format=raw, none of this code is ever
>> invoked.
>> 3) probed_raw images usually look exactly like raw images in most cases
>> 4) In the degenerate cases, probe_raw images are still mountable in the
>> normal way.
>> 5) Even after the QRAW signature is applied, if the guest writes a valid
>> signature, we can truncate the file and make it appear as a normal raw
>> image.
>>
>> Christoph/Markus/Stefan, does this seem like a more reasonable approach?
>
> It took me a little while to figure out how your scheme works. I like
> that the check for sector 0 writes is moved out of the generic I/O
> code path and into its own module. The probed_raw format could be
> easily dropped later if the decision is made to stop probing
> altogether.
>
> It would be simpler to avoid the QRAW signature and sector 0
> redirection by simply failing dodgy writes to sector 0 with EIO. You
> said that would be confusing to users, but if we have no good way to
> present errors to the user, then that is a different problem that
> needs to be addressed anyway.
On second thought, we can't do EIO if there is a reasonable case where
the user may wish to write any of the blacklisted signatures to disk
:(. Perhaps the sector 0 redirection is really necessary.
I like this new approach better than the first patch.
Stefan
>
> What we're talking about here is similar to "boot sector virus
> protection" that BIOSes implement(ed?).
>
> Stefan
>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>> That they get an unsafe
>>> default that way is a big surprise to them. And I can't blame them!
>>> Users can reasonably expect programs not to trap them.
>>>
>>> If we want to let users define drives without having to specify the
>>> format, we can guess the format from the file name.
>>>
>>
>>
>>
>
