Re: [Ping^3] Re: [PATCH] sg_io: allow UNMAP and WRITE SAME without CAP_SYS_RAWIO

[Ric Wheeler <ricwheeler@gmail.com>]

On 09/06/2012 07:49 AM, Paolo Bonzini wrote:
> Il 06/09/2012 13:31, Ric Wheeler ha scritto:
>>>> Both of these commands are destructive. WRITE_SAME (if done without the
>>>> discard bits set) can also take a very long time to be destructive and
>>>> tie up the storage.
>>> FORMAT_UNIT has the same characteristics and yet it is allowed (btw, I
>>> don't think WRITE SAME slowness is limited to the case where a real
>>> write is requested; discarding can be just as slow).
>>>
>>> Also, the two new commands are anyway restricted to programs that have
>>> write access to the disk.  If you have read-only access, you won't be
>>> able to issue any destructive command (there is one exception, START
>>> STOP UNIT is allowed even with read-only capability and is somewhat
>>> destructive).
>>>
>>> Honestly, the only reason why these two commands weren't included, is
>>> that the current whitelist is heavily tailored towards CD/DVD burning.
>> I assume that FORMAT_UNIT is for CD/DVD needs - not sure what a S-ATA
>> disk would do with that.
> According to the standard, the translation layer can write a
> user-provided pattern to every sector in the disk.  It's an optional
> feature and libata doesn't do that, but it is still possible.

It is not possible today with our stack though, any patch that would change that 
would also need to be vetted.

>
>> If it is destructive, we should probably think
>> about how to make it more secure and see how many applications we would
>> break.
> We have filesystem permissions to make it secure.  They already do.
>
>>>> I think that restricting them to CAP_SYS_RAWIO seems reasonable - better
>>>> to vet and give the appropriate apps the needed capability than to
>>>> widely open up the safety check?
>>> CAP_SYS_RAWIO is so wide in its scope, that anything that requires it is
>>> insecure.
>> I don't see allowing anyone who can open the device to zero the data as
>> better though :)
> Note: anyone who can open it for writing!  And they can just as well
> issue WRITE, it just takes a little more effort than with WRITE SAME. :)
>   If you only have read access, you cannot issue WRITE or FORMAT UNIT,
> and with this patch you will not be able to issue WRITE SAME.
>
> I'm all for providing more versatile filters---which can be both
> stricter and looser depending on the configuration than the default.
> For example
> http://www.redhat.com/archives/libvir-list/2012-June/msg00505.html is a
> possible spec for BPF-based filtering of CDBs.
>
> However, the default whitelist (which is all we have for now) should
> provide a reasonable default for a user that already has been granted
> access to the device by the normal access control mechanisms.  I believe
> WRITE SAME and UNMAP fit the definition of a reasonable default.
>
> Paolo

This just seems like an argument over whether or not capabilities make sense. In 
general, anything as destructive as a single CDB that can kill all of your data 
should be tightly controlled.

Pushing more code in the data path is not where we are going - we routinely need 
to disable IO scheduling for example when driving IO to high speed/low latency 
devices and are actively looking at how to tackle other performance bottlenecks 
in the stack.

I don't see a strong reason that our existing scheme (root or CAP_SYS_RAWIO 
access) prevents you from doing what you need to do.

Ric