Re: [PATCH v3 27/32] nvdimm: support DSM_CMD_IMPLEMENTED function

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 10/15/2015 11:07 PM, Stefan Hajnoczi wrote:
> On Wed, Oct 14, 2015 at 10:50:40PM +0800, Xiao Guangrong wrote:
>> On 10/14/2015 05:40 PM, Stefan Hajnoczi wrote:
>>> On Sun, Oct 11, 2015 at 11:52:59AM +0800, Xiao Guangrong wrote:
>>>> +    out = (dsm_out *)in;
>>>> +
>>>> +    revision = in->arg1;
>>>> +    function = in->arg2;
>>>> +    handle = in->handle;
>>>> +    le32_to_cpus(&revision);
>>>> +    le32_to_cpus(&function);
>>>> +    le32_to_cpus(&handle);
>>>> +
>>>> +    nvdebug("UUID " UUID_FMT ".\n", in->arg0[0], in->arg0[1], in->arg0[2],
>>>> +            in->arg0[3], in->arg0[4], in->arg0[5], in->arg0[6],
>>>> +            in->arg0[7], in->arg0[8], in->arg0[9], in->arg0[10],
>>>> +            in->arg0[11], in->arg0[12], in->arg0[13], in->arg0[14],
>>>> +            in->arg0[15]);
>>>> +    nvdebug("Revision %#x Function %#x Handler %#x.\n", revision, function,
>>>> +            handle);
>>>> +
>>>> +    if (revision != DSM_REVISION) {
>>>> +        nvdebug("Revision %#x is not supported, expect %#x.\n",
>>>> +                revision, DSM_REVISION);
>>>> +        goto exit;
>>>> +    }
>>>> +
>>>> +    if (!handle) {
>>>> +        if (!dsm_is_root_uuid(in->arg0)) {
>>>
>>> Please don't dereference 'in' or pass it to other functions.  Avoid race
>>> conditions with guest vcpus by coping in the entire dsm_in struct.
>>>
>>> This is like a system call - the kernel cannot trust userspace memory
>>> and must copy in before accessing data.  The same rules apply.
>>>
>>
>> It's little different for QEMU:
>> - the memory address is always valid to QEMU, it's not always true for Kernel
>>    due to context-switch
>>
>> - we have checked the header before use it's data, for example, when we get
>>    data from GET_NAMESPACE_DATA, we have got the @offset and @length from the
>>    memory, then copy memory based on these values, that means the userspace
>>    has no chance to cause buffer overflow by increasing these values at runtime.
>>
>>    The scenario for our case is simple but Kernel is difficult to do
>>    check_all_before_use as many paths may be involved.
>>
>> - guest changes some data is okay, the worst case is that the label data is
>>    corrupted. This is caused by guest itself. Kernel also supports this kind
>>    of behaviour, e,g. network TX zero copy, the userspace page is being
>>    transferred while userspace can still access it.
>>
>> - it's 4K size on x86, full copy wastes CPU time too much.
>
> This isn't performance-critical code and I don't want to review it
> keeping the race conditions in mind the whole time.  Also, if the code
> is modified in the future, the chance of introducing a race is high.
>
> I see this as premature optimization, please just copy in data.


No strong opinion on it... will do it following your idea.