Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On 01.06.20 05:04, Benjamin Herrenschmidt wrote: On Thu, 2020-05-28 at 15:12 +0200, Greg KH wrote: So at runtime, after all is booted and up and going, you just ripped cores out from under someone's feet? :) And the code really handles writing to that value while the module is already loaded and up and running? At a quick glance, it didn't seem like it would handle that very well as it only is checked at ne_init() time. Or am I missing something? Anyway, yes, if you can dynamically do this at runtime, that's great, but it feels ackward to me to rely on one configuration thing as a module parameter, and everything else through the ioctl interface. Unification would seem to be a good thing, right? I personally still prefer a sysfs file :) I really don't like module parameters as a way to do such things. I think we're going in circles :). A module parameter initialized with module_param_cb gives us a sysfs file that can also have a default parameter set through easily available tooling. The ioctl has two downsides: 1) It relies on an external application 2) The permission check would be strictly limited to CAP_ADMIN, sysfs files can have different permissions So I fail to see how a module parameter is *not* giving both of you and me what we want? Of course only if it implements the callback. It was missing that and apologize for that oversight. Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Mon, Jun 01, 2020 at 12:47:12PM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2020-05-26 at 08:51 +0200, Greg KH wrote: > > > > And get them to sign off on it too, showing they agree with the design > > decisions here :) > > Isn't it generally frowned upon to publish a patch with internal sign- > off's on it already ? Not at all. > Or do you mean for us to publicly sign off once we have reviewed ? Either is fine, as long as you do the public one "quickly" and don't rely on others to do the review first :) thanks, greg k-h
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Thu, 2020-05-28 at 15:12 +0200, Greg KH wrote: > So at runtime, after all is booted and up and going, you just ripped > cores out from under someone's feet? :) > > And the code really handles writing to that value while the module is > already loaded and up and running? At a quick glance, it didn't seem > like it would handle that very well as it only is checked at ne_init() > time. > > Or am I missing something? > > Anyway, yes, if you can dynamically do this at runtime, that's great, > but it feels ackward to me to rely on one configuration thing as a > module parameter, and everything else through the ioctl interface. > Unification would seem to be a good thing, right? I personally still prefer a sysfs file :) I really don't like module parameters as a way to do such things. Cheers, Ben.
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Wed, 2020-05-27 at 00:24 +0200, Greg KH wrote: > > Would you want random users to get the ability to hot unplug CPUs from your > > system? At unlimited quantity? I don't :). > > A random user, no, but one with admin rights, why not? They can do that > already today on your system, this isn't new. So I agree with you that a module parameter sucks. I disagree on the ioctl :) This is the kind of setup task that will probably end up being done by some shell script at boot time based on some config file. Being able to echo something in a sysfs file which will parse the standard-format CPU list using the existing kernel functions to turn that into a cpu_mask makes a lot more sense than having a binary interface via an ioctl which will require an extra userspace program for use by the admin for that one single task. So sysfs is my preference here. Another approach would be configfs, which would provide a more flexible interface to potentially create multiple "CPU sets" that could be given to different users or classes of users etc... but that might be pushing it, I don't know. Cheers, Ben.
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Tue, 2020-05-26 at 14:44 +0200, Alexander Graf wrote: > So I really don't think an ioctl would be a great user experience. Same > for a sysfs file - although that's probably slightly better than the ioctl. What would be wrong with a sysfs file ? Another way to approach that makes sense from a kernel perspective is to have the user first offline the CPUs, then "donate" them to the driver via a sysfs file. > Other options I can think of: > >* sysctl (for modules?) Why would that be any good ? If anything sysctl's are even more awkward in my book :) >* module parameter (as implemented here) >* proc file (deprecated FWIW) Yeah no. > The key is the tenant split: Admin sets the pool up, user consumes. This > setup should happen (early) on boot, so that system services can spawn > enclaves. Right and you can have some init script or udev rule that sets that up from a sys admin produced config file at boot upon detection of the enclave PCI device for example. > > module parameters are a major pain, you know this :) > > I think in this case it's the least painful option ;). But I'm really > happy to hear about an actually good alternative to it. Right now, I > just can't think of any. Cheers, Ben.
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Tue, 2020-05-26 at 08:51 +0200, Greg KH wrote: > > And get them to sign off on it too, showing they agree with the design > decisions here :) Isn't it generally frowned upon to publish a patch with internal sign- off's on it already ? Or do you mean for us to publicly sign off once we have reviewed ? Cheers, Ben.
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On 28/05/2020 16:12, Greg KH wrote: On Thu, May 28, 2020 at 03:01:36PM +0200, Alexander Graf wrote: On 27.05.20 00:24, Greg KH wrote: On Tue, May 26, 2020 at 03:44:30PM +0200, Alexander Graf wrote: On 26.05.20 15:17, Greg KH wrote: On Tue, May 26, 2020 at 02:44:18PM +0200, Alexander Graf wrote: On 26.05.20 14:33, Greg KH wrote: On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: On 26.05.20 08:51, Greg KH wrote: On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: +#define NE "nitro_enclaves: " Again, no need for this. +#define NE_DEV_NAME "nitro_enclaves" KBUILD_MODNAME? +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) + +static char *ne_cpus; +module_param(ne_cpus, charp, 0644); +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro Enclaves"); Again, please do not do this. I actually asked her to put this one in specifically. The concept of this parameter is very similar to isolcpus= and maxcpus= in that it takes CPUs away from Linux and instead donates them to the underlying hypervisor, so that it can spawn enclaves using them. From an admin's point of view, this is a setting I would like to keep persisted across reboots. How would this work with sysfs? How about just as the "initial" ioctl command to set things up? Don't grab any cpu pools until asked to. Otherwise, what happens when you load this module on a system that can't support it? That would give any user with access to the enclave device the ability to remove CPUs from the system. That's clearly a CAP_ADMIN task in my book. Ok, what's wrong with that? Would you want random users to get the ability to hot unplug CPUs from your system? At unlimited quantity? I don't :). A random user, no, but one with admin rights, why not? They can do that already today on your system, this isn't new. Hence this whole split: The admin defines the CPU Pool, users can safely consume this pool to spawn enclaves from it. But having the admin define that at module load / boot time, is a major pain. What tools do they have that allow them to do that easily? The normal toolbox: editing /etc/default/grub, adding an /etc/modprobe.d/ file. Editing grub files is horrid, come on... It's not editing grub files, it's editing template config files that then are used as input for grub config file generation :). When but at module load / boot time would you define it? I really don't want to have a device node that in theory "the world" can use which then allows any user on the system to hot unplug every CPU but 0 from my system. But you have that already when the PCI device is found, right? What is the initial interface to the driver? What's wrong with using that? Or am I really missing something as to how this all fits together with the different pieces? Seeing the patches as-is doesn't really provide a good overview, sorry. Ok, let me walk you through the core donation process. Imagine you have a parent VM with 8 cores. Every one of those virtual cores is 1:1 mapped to a physical core. You enumerate the PCI device, you start working with it. None of that changes your topology. You now create an enclave spanning 2 cores. The hypervisor will remove the 1:1 map for those 2 cores and instead mark them as "free floating" on the remaining 6 cores. It then uses the 2 freed up cores and creates a 1:1 map for the enclave's 2 cores To ensure that we still see a realistic mapping of core topology, we need to remove those 2 cores from the parent VM's scope of execution. The way this is done today is by going through CPU offlining. The first and obvious option would be to offline all respective CPUs when an enclave gets created. But unprivileged users should be able to spawn enclaves. So how do I ensure that my unprivileged user doesn't just offline all of my parent VM's CPUs? The option implemented here is that we fold this into a two-stage approach. The admin reserves a "pool" of cores for enclaves to use. Unprivileged users can then consume cores from that pool, but not more than those. That way, unprivileged users have no influence over whether a core is enabled or not. They can only consume the pool of cores that was dedicated for enclave use. It also has the big advantage that you don't have dynamically changing CPU topology in your system. Long living processes that adjust their environment to the topology can still do so, without cores getting pulled out under their feet. Ok, that makes more sense, but: So I really don't think an ioctl would be a great user experience. Same for a sysfs file - although that's probably slightly better than the ioctl. You already are using ioctls to control this thing, right? What's wrong with "one more"? :) So what we *could* do is add an ioctl to set the pool size which then does a CAP_ADMIN check. That however means you now are in priority hell: A user that wants to spawn an enclave as part of an nginx service would need to create another service to s
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Thu, May 28, 2020 at 03:01:36PM +0200, Alexander Graf wrote: > > > On 27.05.20 00:24, Greg KH wrote: > > > > On Tue, May 26, 2020 at 03:44:30PM +0200, Alexander Graf wrote: > > > > > > > > > On 26.05.20 15:17, Greg KH wrote: > > > > > > > > On Tue, May 26, 2020 at 02:44:18PM +0200, Alexander Graf wrote: > > > > > > > > > > > > > > > On 26.05.20 14:33, Greg KH wrote: > > > > > > > > > > > > On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: > > > > > > > > > > > > > > > > > > > > > On 26.05.20 08:51, Greg KH wrote: > > > > > > > > > > > > > > > > On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: > > > > > > > > > +#define NE "nitro_enclaves: " > > > > > > > > > > > > > > > > Again, no need for this. > > > > > > > > > > > > > > > > > +#define NE_DEV_NAME "nitro_enclaves" > > > > > > > > > > > > > > > > KBUILD_MODNAME? > > > > > > > > > > > > > > > > > +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) > > > > > > > > > + > > > > > > > > > +static char *ne_cpus; > > > > > > > > > +module_param(ne_cpus, charp, 0644); > > > > > > > > > +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for > > > > > > > > > Nitro Enclaves"); > > > > > > > > > > > > > > > > Again, please do not do this. > > > > > > > > > > > > > > I actually asked her to put this one in specifically. > > > > > > > > > > > > > > The concept of this parameter is very similar to isolcpus= and > > > > > > > maxcpus= in > > > > > > > that it takes CPUs away from Linux and instead donates them to the > > > > > > > underlying hypervisor, so that it can spawn enclaves using them. > > > > > > > > > > > > > >From an admin's point of view, this is a setting I would like > > > > > > > to keep > > > > > > > persisted across reboots. How would this work with sysfs? > > > > > > > > > > > > How about just as the "initial" ioctl command to set things up? > > > > > > Don't > > > > > > grab any cpu pools until asked to. Otherwise, what happens when you > > > > > > load this module on a system that can't support it? > > > > > > > > > > That would give any user with access to the enclave device the > > > > > ability to > > > > > remove CPUs from the system. That's clearly a CAP_ADMIN task in my > > > > > book. > > > > > > > > Ok, what's wrong with that? > > > > > > Would you want random users to get the ability to hot unplug CPUs from > > > your > > > system? At unlimited quantity? I don't :). > > > > A random user, no, but one with admin rights, why not? They can do that > > already today on your system, this isn't new. > > > > > > > Hence this whole split: The admin defines the CPU Pool, users can > > > > > safely > > > > > consume this pool to spawn enclaves from it. > > > > > > > > But having the admin define that at module load / boot time, is a major > > > > pain. What tools do they have that allow them to do that easily? > > > > > > The normal toolbox: editing /etc/default/grub, adding an /etc/modprobe.d/ > > > file. > > > > Editing grub files is horrid, come on... > > It's not editing grub files, it's editing template config files that then > are used as input for grub config file generation :). > > > > When but at module load / boot time would you define it? I really don't > > > want > > > to have a device node that in theory "the world" can use which then allows > > > any user on the system to hot unplug every CPU but 0 from my system. > > > > But you have that already when the PCI device is found, right? What is > > the initial interface to the driver? What's wrong with using that? > > > > Or am I really missing something as to how this all fits together with > > the different pieces? Seeing the patches as-is doesn't really provide a > > good overview, sorry. > > Ok, let me walk you through the core donation process. > > Imagine you have a parent VM with 8 cores. Every one of those virtual cores > is 1:1 mapped to a physical core. > > You enumerate the PCI device, you start working with it. None of that > changes your topology. > > You now create an enclave spanning 2 cores. The hypervisor will remove the > 1:1 map for those 2 cores and instead mark them as "free floating" on the > remaining 6 cores. It then uses the 2 freed up cores and creates a 1:1 map > for the enclave's 2 cores > > To ensure that we still see a realistic mapping of core topology, we need to > remove those 2 cores from the parent VM's scope of execution. The way this > is done today is by going through CPU offlining. > > The first and obvious option would be to offline all respective CPUs when an > enclave gets created. But unprivileged users should be able to spawn > enclaves. So how do I ensure that my unprivileged user doesn't just offline > all of my parent VM's CPUs? > > The option implemented here is that we fold this into a two-stage approach. > The admin reserves a "pool" of cores for enclaves to use. Unprivileged users > can then consume cores from that pool, but not more than those. > > Th
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On 27.05.20 00:24, Greg KH wrote: On Tue, May 26, 2020 at 03:44:30PM +0200, Alexander Graf wrote: On 26.05.20 15:17, Greg KH wrote: On Tue, May 26, 2020 at 02:44:18PM +0200, Alexander Graf wrote: On 26.05.20 14:33, Greg KH wrote: On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: On 26.05.20 08:51, Greg KH wrote: On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: +#define NE "nitro_enclaves: " Again, no need for this. +#define NE_DEV_NAME "nitro_enclaves" KBUILD_MODNAME? +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) + +static char *ne_cpus; +module_param(ne_cpus, charp, 0644); +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro Enclaves"); Again, please do not do this. I actually asked her to put this one in specifically. The concept of this parameter is very similar to isolcpus= and maxcpus= in that it takes CPUs away from Linux and instead donates them to the underlying hypervisor, so that it can spawn enclaves using them. From an admin's point of view, this is a setting I would like to keep persisted across reboots. How would this work with sysfs? How about just as the "initial" ioctl command to set things up? Don't grab any cpu pools until asked to. Otherwise, what happens when you load this module on a system that can't support it? That would give any user with access to the enclave device the ability to remove CPUs from the system. That's clearly a CAP_ADMIN task in my book. Ok, what's wrong with that? Would you want random users to get the ability to hot unplug CPUs from your system? At unlimited quantity? I don't :). A random user, no, but one with admin rights, why not? They can do that already today on your system, this isn't new. Hence this whole split: The admin defines the CPU Pool, users can safely consume this pool to spawn enclaves from it. But having the admin define that at module load / boot time, is a major pain. What tools do they have that allow them to do that easily? The normal toolbox: editing /etc/default/grub, adding an /etc/modprobe.d/ file. Editing grub files is horrid, come on... It's not editing grub files, it's editing template config files that then are used as input for grub config file generation :). When but at module load / boot time would you define it? I really don't want to have a device node that in theory "the world" can use which then allows any user on the system to hot unplug every CPU but 0 from my system. But you have that already when the PCI device is found, right? What is the initial interface to the driver? What's wrong with using that? Or am I really missing something as to how this all fits together with the different pieces? Seeing the patches as-is doesn't really provide a good overview, sorry. Ok, let me walk you through the core donation process. Imagine you have a parent VM with 8 cores. Every one of those virtual cores is 1:1 mapped to a physical core. You enumerate the PCI device, you start working with it. None of that changes your topology. You now create an enclave spanning 2 cores. The hypervisor will remove the 1:1 map for those 2 cores and instead mark them as "free floating" on the remaining 6 cores. It then uses the 2 freed up cores and creates a 1:1 map for the enclave's 2 cores To ensure that we still see a realistic mapping of core topology, we need to remove those 2 cores from the parent VM's scope of execution. The way this is done today is by going through CPU offlining. The first and obvious option would be to offline all respective CPUs when an enclave gets created. But unprivileged users should be able to spawn enclaves. So how do I ensure that my unprivileged user doesn't just offline all of my parent VM's CPUs? The option implemented here is that we fold this into a two-stage approach. The admin reserves a "pool" of cores for enclaves to use. Unprivileged users can then consume cores from that pool, but not more than those. That way, unprivileged users have no influence over whether a core is enabled or not. They can only consume the pool of cores that was dedicated for enclave use. It also has the big advantage that you don't have dynamically changing CPU topology in your system. Long living processes that adjust their environment to the topology can still do so, without cores getting pulled out under their feet. So I really don't think an ioctl would be a great user experience. Same for a sysfs file - although that's probably slightly better than the ioctl. You already are using ioctls to control this thing, right? What's wrong with "one more"? :) So what we *could* do is add an ioctl to set the pool size which then does a CAP_ADMIN check. That however means you now are in priority hell: A user that wants to spawn an enclave as part of an nginx service would need to create another service to set the pool size and indicate the dependency in systemd control files. Is that rea
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Tue, May 26, 2020 at 03:44:30PM +0200, Alexander Graf wrote: > > > On 26.05.20 15:17, Greg KH wrote: > > > > On Tue, May 26, 2020 at 02:44:18PM +0200, Alexander Graf wrote: > > > > > > > > > On 26.05.20 14:33, Greg KH wrote: > > > > > > > > On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: > > > > > > > > > > > > > > > On 26.05.20 08:51, Greg KH wrote: > > > > > > > > > > > > On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: > > > > > > > +#define NE "nitro_enclaves: " > > > > > > > > > > > > Again, no need for this. > > > > > > > > > > > > > +#define NE_DEV_NAME "nitro_enclaves" > > > > > > > > > > > > KBUILD_MODNAME? > > > > > > > > > > > > > +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) > > > > > > > + > > > > > > > +static char *ne_cpus; > > > > > > > +module_param(ne_cpus, charp, 0644); > > > > > > > +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro > > > > > > > Enclaves"); > > > > > > > > > > > > Again, please do not do this. > > > > > > > > > > I actually asked her to put this one in specifically. > > > > > > > > > > The concept of this parameter is very similar to isolcpus= and > > > > > maxcpus= in > > > > > that it takes CPUs away from Linux and instead donates them to the > > > > > underlying hypervisor, so that it can spawn enclaves using them. > > > > > > > > > > From an admin's point of view, this is a setting I would like to > > > > > keep > > > > > persisted across reboots. How would this work with sysfs? > > > > > > > > How about just as the "initial" ioctl command to set things up? Don't > > > > grab any cpu pools until asked to. Otherwise, what happens when you > > > > load this module on a system that can't support it? > > > > > > That would give any user with access to the enclave device the ability to > > > remove CPUs from the system. That's clearly a CAP_ADMIN task in my book. > > > > Ok, what's wrong with that? > > Would you want random users to get the ability to hot unplug CPUs from your > system? At unlimited quantity? I don't :). A random user, no, but one with admin rights, why not? They can do that already today on your system, this isn't new. > > > Hence this whole split: The admin defines the CPU Pool, users can safely > > > consume this pool to spawn enclaves from it. > > > > But having the admin define that at module load / boot time, is a major > > pain. What tools do they have that allow them to do that easily? > > The normal toolbox: editing /etc/default/grub, adding an /etc/modprobe.d/ > file. Editing grub files is horrid, come on... > When but at module load / boot time would you define it? I really don't want > to have a device node that in theory "the world" can use which then allows > any user on the system to hot unplug every CPU but 0 from my system. But you have that already when the PCI device is found, right? What is the initial interface to the driver? What's wrong with using that? Or am I really missing something as to how this all fits together with the different pieces? Seeing the patches as-is doesn't really provide a good overview, sorry. > > > So I really don't think an ioctl would be a great user experience. Same > > > for > > > a sysfs file - although that's probably slightly better than the ioctl. > > > > You already are using ioctls to control this thing, right? What's wrong > > with "one more"? :) > > So what we *could* do is add an ioctl to set the pool size which then does a > CAP_ADMIN check. That however means you now are in priority hell: > > A user that wants to spawn an enclave as part of an nginx service would need > to create another service to set the pool size and indicate the dependency > in systemd control files. > > Is that really better than a module parameter? module parameters are hard to change, and manage control over who really is changing them. thanks, greg k-h
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On 26.05.20 15:17, Greg KH wrote: On Tue, May 26, 2020 at 02:44:18PM +0200, Alexander Graf wrote: On 26.05.20 14:33, Greg KH wrote: On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: On 26.05.20 08:51, Greg KH wrote: On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: +#define NE "nitro_enclaves: " Again, no need for this. +#define NE_DEV_NAME "nitro_enclaves" KBUILD_MODNAME? +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) + +static char *ne_cpus; +module_param(ne_cpus, charp, 0644); +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro Enclaves"); Again, please do not do this. I actually asked her to put this one in specifically. The concept of this parameter is very similar to isolcpus= and maxcpus= in that it takes CPUs away from Linux and instead donates them to the underlying hypervisor, so that it can spawn enclaves using them. From an admin's point of view, this is a setting I would like to keep persisted across reboots. How would this work with sysfs? How about just as the "initial" ioctl command to set things up? Don't grab any cpu pools until asked to. Otherwise, what happens when you load this module on a system that can't support it? That would give any user with access to the enclave device the ability to remove CPUs from the system. That's clearly a CAP_ADMIN task in my book. Ok, what's wrong with that? Would you want random users to get the ability to hot unplug CPUs from your system? At unlimited quantity? I don't :). Hence this whole split: The admin defines the CPU Pool, users can safely consume this pool to spawn enclaves from it. But having the admin define that at module load / boot time, is a major pain. What tools do they have that allow them to do that easily? The normal toolbox: editing /etc/default/grub, adding an /etc/modprobe.d/ file. When but at module load / boot time would you define it? I really don't want to have a device node that in theory "the world" can use which then allows any user on the system to hot unplug every CPU but 0 from my system. So I really don't think an ioctl would be a great user experience. Same for a sysfs file - although that's probably slightly better than the ioctl. You already are using ioctls to control this thing, right? What's wrong with "one more"? :) So what we *could* do is add an ioctl to set the pool size which then does a CAP_ADMIN check. That however means you now are in priority hell: A user that wants to spawn an enclave as part of an nginx service would need to create another service to set the pool size and indicate the dependency in systemd control files. Is that really better than a module parameter? Other options I can think of: * sysctl (for modules?) Ick. * module parameter (as implemented here) Ick. * proc file (deprecated FWIW) Ick. The key is the tenant split: Admin sets the pool up, user consumes. This setup should happen (early) on boot, so that system services can spawn enclaves. But it takes more than jus this initial "split up" to set the pool up, I don't quite follow. The initial "split up" is all it takes. From the hypervisor's point of view, the physical underlying cores will not be used to schedule the parent as soon as an enclave is running on them. Which CPUs are available for enclaves however is purely a parent OS construct, hence the module parameter. right? Why not make this part of that initial process? What makes this so special you have to do this at module load time only? What is the "initial process"? It's really 2 stages: One stage to create a pool (CAP_ADMIN) which makes sure that some cores become invisible to the Linux scheduler and one stage to spawn an enclave (normal user permission) on those pool's CPUs. Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On 26/05/2020 15:33, Greg KH wrote: On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: On 26.05.20 08:51, Greg KH wrote: On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: +#define NE "nitro_enclaves: " Again, no need for this. +#define NE_DEV_NAME "nitro_enclaves" KBUILD_MODNAME? +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) + +static char *ne_cpus; +module_param(ne_cpus, charp, 0644); +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro Enclaves"); Again, please do not do this. I actually asked her to put this one in specifically. The concept of this parameter is very similar to isolcpus= and maxcpus= in that it takes CPUs away from Linux and instead donates them to the underlying hypervisor, so that it can spawn enclaves using them. From an admin's point of view, this is a setting I would like to keep persisted across reboots. How would this work with sysfs? How about just as the "initial" ioctl command to set things up? Don't grab any cpu pools until asked to. Otherwise, what happens when you load this module on a system that can't support it? module parameters are a major pain, you know this :) So yes, let's give everyone in CC the change to review v3 properly first before v4 goes out. And get them to sign off on it too, showing they agree with the design decisions here :) I would expect a Reviewed-by tag as a result from the above would satisfy this? :) That would be most appreciated. With regarding to reviewing, yes, the patch series went through several rounds before submitting it upstream. I posted v3 shortly after v2 to have more visibility into the changelog for each patch in addition to the cover letter changelog. But no major design changes in there. :) Thank you. Andra 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.
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Tue, May 26, 2020 at 02:44:18PM +0200, Alexander Graf wrote: > > > On 26.05.20 14:33, Greg KH wrote: > > > > On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: > > > > > > > > > On 26.05.20 08:51, Greg KH wrote: > > > > > > > > On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: > > > > > +#define NE "nitro_enclaves: " > > > > > > > > Again, no need for this. > > > > > > > > > +#define NE_DEV_NAME "nitro_enclaves" > > > > > > > > KBUILD_MODNAME? > > > > > > > > > +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) > > > > > + > > > > > +static char *ne_cpus; > > > > > +module_param(ne_cpus, charp, 0644); > > > > > +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro > > > > > Enclaves"); > > > > > > > > Again, please do not do this. > > > > > > I actually asked her to put this one in specifically. > > > > > > The concept of this parameter is very similar to isolcpus= and maxcpus= in > > > that it takes CPUs away from Linux and instead donates them to the > > > underlying hypervisor, so that it can spawn enclaves using them. > > > > > > From an admin's point of view, this is a setting I would like to keep > > > persisted across reboots. How would this work with sysfs? > > > > How about just as the "initial" ioctl command to set things up? Don't > > grab any cpu pools until asked to. Otherwise, what happens when you > > load this module on a system that can't support it? > > That would give any user with access to the enclave device the ability to > remove CPUs from the system. That's clearly a CAP_ADMIN task in my book. Ok, what's wrong with that? > Hence this whole split: The admin defines the CPU Pool, users can safely > consume this pool to spawn enclaves from it. But having the admin define that at module load / boot time, is a major pain. What tools do they have that allow them to do that easily? > So I really don't think an ioctl would be a great user experience. Same for > a sysfs file - although that's probably slightly better than the ioctl. You already are using ioctls to control this thing, right? What's wrong with "one more"? :) > Other options I can think of: > > * sysctl (for modules?) Ick. > * module parameter (as implemented here) Ick. > * proc file (deprecated FWIW) Ick. > The key is the tenant split: Admin sets the pool up, user consumes. This > setup should happen (early) on boot, so that system services can spawn > enclaves. But it takes more than jus this initial "split up" to set the pool up, right? Why not make this part of that initial process? What makes this so special you have to do this at module load time only? thanks, greg k-h
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On 26.05.20 14:33, Greg KH wrote: On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: On 26.05.20 08:51, Greg KH wrote: On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: +#define NE "nitro_enclaves: " Again, no need for this. +#define NE_DEV_NAME "nitro_enclaves" KBUILD_MODNAME? +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) + +static char *ne_cpus; +module_param(ne_cpus, charp, 0644); +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro Enclaves"); Again, please do not do this. I actually asked her to put this one in specifically. The concept of this parameter is very similar to isolcpus= and maxcpus= in that it takes CPUs away from Linux and instead donates them to the underlying hypervisor, so that it can spawn enclaves using them. From an admin's point of view, this is a setting I would like to keep persisted across reboots. How would this work with sysfs? How about just as the "initial" ioctl command to set things up? Don't grab any cpu pools until asked to. Otherwise, what happens when you load this module on a system that can't support it? That would give any user with access to the enclave device the ability to remove CPUs from the system. That's clearly a CAP_ADMIN task in my book. Hence this whole split: The admin defines the CPU Pool, users can safely consume this pool to spawn enclaves from it. So I really don't think an ioctl would be a great user experience. Same for a sysfs file - although that's probably slightly better than the ioctl. Other options I can think of: * sysctl (for modules?) * module parameter (as implemented here) * proc file (deprecated FWIW) The key is the tenant split: Admin sets the pool up, user consumes. This setup should happen (early) on boot, so that system services can spawn enclaves. module parameters are a major pain, you know this :) I think in this case it's the least painful option ;). But I'm really happy to hear about an actually good alternative to it. Right now, I just can't think of any. Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Tue, May 26, 2020 at 01:42:41PM +0200, Alexander Graf wrote: > > > On 26.05.20 08:51, Greg KH wrote: > > > > On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: > > > +#define NE "nitro_enclaves: " > > > > Again, no need for this. > > > > > +#define NE_DEV_NAME "nitro_enclaves" > > > > KBUILD_MODNAME? > > > > > +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) > > > + > > > +static char *ne_cpus; > > > +module_param(ne_cpus, charp, 0644); > > > +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro > > > Enclaves"); > > > > Again, please do not do this. > > I actually asked her to put this one in specifically. > > The concept of this parameter is very similar to isolcpus= and maxcpus= in > that it takes CPUs away from Linux and instead donates them to the > underlying hypervisor, so that it can spawn enclaves using them. > > From an admin's point of view, this is a setting I would like to keep > persisted across reboots. How would this work with sysfs? How about just as the "initial" ioctl command to set things up? Don't grab any cpu pools until asked to. Otherwise, what happens when you load this module on a system that can't support it? module parameters are a major pain, you know this :) > So yes, let's give everyone in CC the change to review v3 properly first > before v4 goes out. > > > And get them to sign off on it too, showing they agree with the design > > decisions here :) > > I would expect a Reviewed-by tag as a result from the above would satisfy > this? :) That would be most appreciated. thanks, greg k-h
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On 26.05.20 08:51, Greg KH wrote: On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: +#define NE "nitro_enclaves: " Again, no need for this. +#define NE_DEV_NAME "nitro_enclaves" KBUILD_MODNAME? +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) + +static char *ne_cpus; +module_param(ne_cpus, charp, 0644); +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro Enclaves"); Again, please do not do this. I actually asked her to put this one in specifically. The concept of this parameter is very similar to isolcpus= and maxcpus= in that it takes CPUs away from Linux and instead donates them to the underlying hypervisor, so that it can spawn enclaves using them. From an admin's point of view, this is a setting I would like to keep persisted across reboots. How would this work with sysfs? Can you get the other amazon.com developers on the cc: list to review this before you send it out again? I feel like I am doing basic review of things that should be easily caught by them before you ask the community to review your code. Again, my fault :). We did a good number of internal review rounds, but I guess I didn't catch the bits you pointed out. So yes, let's give everyone in CC the change to review v3 properly first before v4 goes out. And get them to sign off on it too, showing they agree with the design decisions here :) I would expect a Reviewed-by tag as a result from the above would satisfy this? :) Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Re: [PATCH v3 07/18] nitro_enclaves: Init misc device providing the ioctl interface
On Tue, May 26, 2020 at 01:13:23AM +0300, Andra Paraschiv wrote: > +#define NE "nitro_enclaves: " Again, no need for this. > +#define NE_DEV_NAME "nitro_enclaves" KBUILD_MODNAME? > +#define NE_IMAGE_LOAD_OFFSET (8 * 1024UL * 1024UL) > + > +static char *ne_cpus; > +module_param(ne_cpus, charp, 0644); > +MODULE_PARM_DESC(ne_cpus, " - CPU pool used for Nitro Enclaves"); Again, please do not do this. Can you get the other amazon.com developers on the cc: list to review this before you send it out again? I feel like I am doing basic review of things that should be easily caught by them before you ask the community to review your code. And get them to sign off on it too, showing they agree with the design decisions here :) thanks, greg k-h
