Message-ID: <993ba3eb-bf79-6512-2c0e-f959298b8601@amazon.com>
Date:   Fri, 24 Jul 2020 12:06:24 +0300
From:   "Paraschiv, Andra-Irina" <andraprs@...zon.com>
To:     Alexander Graf <graf@...zon.de>,
        Greg KH <gregkh@...uxfoundation.org>
CC:     <linux-kernel@...r.kernel.org>,
        Anthony Liguori <aliguori@...zon.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Colm MacCarthaigh <colmmacc@...zon.com>,
        David Duncan <davdunc@...zon.com>,
        Bjoern Doebel <doebel@...zon.de>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Frank van der Linden <fllinden@...zon.com>,
        Karen Noel <knoel@...hat.com>,
        Martin Pohlack <mpohlack@...zon.de>,
        Matt Wilson <msw@...zon.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Balbir Singh <sblbir@...zon.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Stewart Smith <trawets@...zon.com>,
        Uwe Dannowski <uwed@...zon.de>,
        Vitaly Kuznetsov <vkuznets@...hat.com>, <kvm@...r.kernel.org>,
        <ne-devel-upstream@...zon.com>, Alexander Graf <graf@...zon.com>
Subject: Re: [PATCH v5 01/18] nitro_enclaves: Add ioctl interface definition



On 24/07/2020 02:04, Alexander Graf wrote:
>
>
> On 23.07.20 20:21, Paraschiv, Andra-Irina wrote:
>>
>>
>> On 23/07/2020 13:54, Greg KH wrote:
>>> On Thu, Jul 23, 2020 at 12:23:56PM +0300, Paraschiv, Andra-Irina wrote:
>>>>
>>>> On 22/07/2020 12:57, Greg KH wrote:
>>>>> On Wed, Jul 22, 2020 at 11:27:29AM +0300, Paraschiv, Andra-Irina 
>>>>> wrote:
>>>>>>>> +#ifndef _UAPI_LINUX_NITRO_ENCLAVES_H_
>>>>>>>> +#define _UAPI_LINUX_NITRO_ENCLAVES_H_
>>>>>>>> +
>>>>>>>> +#include <linux/types.h>
>>>>>>>> +
>>>>>>>> +/* Nitro Enclaves (NE) Kernel Driver Interface */
>>>>>>>> +
>>>>>>>> +#define NE_API_VERSION (1)
>>>>>>> Why do you need this version?  It shouldn't be needed, right?
>>>>>> The version is used as a way for the user space tooling to sync 
>>>>>> on the
>>>>>> features set provided by the driver e.g. in case an older version 
>>>>>> of the
>>>>>> driver is available on the system and the user space tooling 
>>>>>> expects a set
>>>>>> of features that is not included in that driver version.
>>>>> That is guaranteed to get out of sync instantly with different distro
>>>>> kernels backporting random things, combined with stable kernel patch
>>>>> updates and the like.
>>>>>
>>>>> Just use the normal api interfaces instead, don't try to "version"
>>>>> anything, it will not work, trust us :)
>>>>>
>>>>> If an ioctl returns -ENOTTY then hey, it's not present and your
>>>>> userspace code can handle it that way.
>>>> Correct, there could be a variety of kernel versions and user space 
>>>> tooling
>>>> either in the original form, customized or written from scratch. 
>>>> And ENOTTY
>>>> signals an ioctl not available or e.g. EINVAL (or custom error) if the
>>>> parameter field value is not valid within a certain version. We 
>>>> have these
>>>> in place, that's good. :)
>>>>
>>>> However, I was thinking, for example, of an ioctl flow usage where 
>>>> a certain
>>>> order needs to be followed e.g. create a VM, add resources to a VM, 
>>>> start a
>>>> VM.
>>>>
>>>> Let's say, for an use case wrt new features, ioctl A (create a VM) 
>>>> succeeds,
>>>> ioctl B (add memory to the VM) succeeds, ioctl C (add CPU to the VM)
>>>> succeeds and ioctl D (add any other type of resource before 
>>>> starting the VM)
>>>> fails because it is not supported.
>>>>
>>>> Would not need to call ioctl A to C and go through their underneath 
>>>> logic to
>>>> realize ioctl D support is not there and rollback all the changes 
>>>> done till
>>>> then within ioctl A to C logic. Of course, there could be ioctl A 
>>>> followed
>>>> by ioctl D, and would need to rollback ioctl A changes, but I 
>>>> shared a more
>>>> lengthy call chain that can be an option as well.
>>> I think you are overthinking this.
>>>
>>> If your interface is this complex, you have much larger issues as you
>>> ALWAYS have to be able to handle error conditions properly, even if the
>>> API is "supported".
>>
>> True, the error paths need to handled correctly on the kernel driver 
>> and on the user space logic side, independent of supported features 
>> or not. Cannot assume that all ioctl callers are behaving correctly 
>> or there are no errors in the system.
>>
>> What I wanted to cover with that example is more towards the user 
>> space logic using new features, either early exiting before even 
>> trying the ioctl call flow path or going through part of the flow 
>> till getting the error e.g. ENOTTY for one of the ioctl calls.
>
> If we need an API to query for new features, we can add it once we add 
> new features, no? The absence of the query API will indicate that no 
> additional features are available.
>
> So yes, sorry, oversight on my side :(. I agree with Greg: there 
> really is no need for a version query API as of today.

No problem. I can remove the versioning logic for now, although I think 
it would have been fine to have it from the beginning if we want to move 
further with a version query API in the end.

The overall discussion here was more towards having the versioning logic 
or not at all, either within the current code base or while getting to 
new features.

>
>>
>>>
>>> Perhaps your API is showing to be too complex?
>>>
>>> Also, where is the userspace code for all of this?  Did I miss a 
>>> link to
>>> it in the patches somewhere?
>>
>> Nope, you didn't miss any references to it. The codebase for the user 
>> space code is not publicly available for now, but it will be 
>> available on GitHub once the whole project is GA. And I'll include 
>> the refs, once available, in the NE kernel driver documentation.
>
> Patch 16/18 contains an example user space to drive the ioctl interface.

Yup, and the flow mentioned in the previous mail is included in the 
ioctl usage sample.

Thanks,
Andra

>
> The code base Andra is referring to above is going to be a more 
> complete framework to drive Nitro Enclaves that also consumes this 
> kernel API.
>
>
> Alex




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

Powered by blists - more mailing lists