| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <11ef0019edd240b6a218b50d766f62ec@huawei.com>
Date: Wed, 3 Nov 2021 13:54:18 +0000
From: "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
<longpeng2@...wei.com>
To: "Paraschiv, Andra-Irina" <andraprs@...zon.com>
CC: "Gonglei (Arei)" <arei.gonglei@...wei.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"kamal@...onical.com" <kamal@...onical.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"sgarzare@...hat.com" <sgarzare@...hat.com>,
"stefanha@...hat.com" <stefanha@...hat.com>,
"vkuznets@...hat.com" <vkuznets@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"ne-devel-upstream@...zon.com" <ne-devel-upstream@...zon.com>,
"lexnv@...zon.com" <lexnv@...zon.com>,
"alcioa@...zon.com" <alcioa@...zon.com>
Subject: RE: [PATCH v3 1/4] nitro_enclaves: Merge contiguous physical memory
regions
Hi Andra,
Sorry for the long delay.
I've read all your suggestions in each patch, there's no objection. I'll
send v4 later, please review when you free, thanks.
> -----Original Message-----
> From: Paraschiv, Andra-Irina [mailto:andraprs@...zon.com]
> Sent: Friday, October 15, 2021 9:34 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@...wei.com>
> Cc: Gonglei (Arei) <arei.gonglei@...wei.com>; gregkh@...uxfoundation.org;
> kamal@...onical.com; pbonzini@...hat.com; sgarzare@...hat.com;
> stefanha@...hat.com; vkuznets@...hat.com; linux-kernel@...r.kernel.org;
> ne-devel-upstream@...zon.com; lexnv@...zon.com; alcioa@...zon.com
> Subject: Re: [PATCH v3 1/4] nitro_enclaves: Merge contiguous physical memory
> regions
>
>
>
> On 09/10/2021 04:32, Longpeng(Mike) wrote:
> >
> > From: Longpeng <longpeng2@...wei.com>
> >
> > There can be cases when there are more memory regions that need to be
> > set for an enclave than the maximum supported number of memory regions
> > per enclave. One example can be when the memory regions are backed by 2
> > MiB hugepages (the minimum supported hugepage size).
> >
> > Let's merge the adjacent regions if they are physical contiguous. This
>
> physical contiguous => physically contiguous
>
> > way the final number of memory regions is less than before merging and
> > could potentially avoid reaching maximum.
> >
> > Signed-off-by: Longpeng <longpeng2@...wei.com>
> > ---
> > Changes v2 -> v3:
> > - update the commit title and commit message. [Andra]
> > - use 'struct range' to instead of 'struct phys_mem_region'. [Andra, Greg
> KH]
> > - add comments before the function definition. [Andra]
> > - rename several variables, parameters and function. [Andra]
> > ---
> > drivers/virt/nitro_enclaves/ne_misc_dev.c | 79
> +++++++++++++++++++++----------
> > 1 file changed, 55 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > index e21e1e8..eea53e9 100644
> > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > @@ -26,6 +26,7 @@
> > #include <linux/poll.h>
> > #include <linux/slab.h>
> > #include <linux/types.h>
> > +#include <linux/range.h>
>
> Please keep the alphabetical order and move this before "#include
> <linux/slab.h>."
>
> > #include <uapi/linux/vm_sockets.h>
> >
> > #include "ne_misc_dev.h"
> > @@ -126,6 +127,16 @@ struct ne_cpu_pool {
> > static struct ne_cpu_pool ne_cpu_pool;
> >
> > /**
> > + * struct phys_contig_mem_regions - Physical contiguous memory regions
>
> Physical contiguous memory regions => Contiguous physical memory regions
>
> > + * @num: The number of regions that currently has.
> > + * @region: The array of physical memory regions.
> > + */
> > +struct phys_contig_mem_regions {
> > + unsigned long num;
> > + struct range region[0];
>
> Should use "struct range *regions" instead, to keep a similar style with
> regard to arrays throughout the code.
>
> Please align the fields name (e.g. [1]) so that it's easier to
> distinguish between fields type and name.
>
> Let's also prefix with "ne" the data structure naming e.g.
> ne_phys_contig_mem_regions. So that is similar to other data structures
> defined in the NE kernel driver codebase.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri
> vers/virt/nitro_enclaves/ne_misc_dev.c#n118
>
> > +};
> > +
> > +/**
> > * ne_check_enclaves_created() - Verify if at least one enclave has been
> created.
> > * @void: No parameters provided.
> > *
> > @@ -825,6 +836,33 @@ static int ne_sanity_check_user_mem_region_page(struct
> ne_enclave *ne_enclave,
> > }
> >
> > /**
> > + * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the
> adjacent
> > + * regions if they are physical
> contiguous.
>
> physical contiguous => physically contiguous
>
> > + * @regions : Private data associated with the physical contiguous memory
> regions.
>
> physical contiguous memory regions => contiguous physical memory regions
>
> @regions => @phys_contig_regions
>
> > + * @page_paddr: Physical start address of the region to be added.
> > + * @page_size : Length of the region to be added.
> > + *
> > + * Return:
> > + * * No return value.
>
> Please add "Context: Process context. This function is called with the
> ne_enclave mutex held." before "Return", similar to other defined functions.
>
> Can remove these lines, as for now there is no return value:
>
> * Return:
> * * No return value.
>
> And then can add this in the second patch in the series:
>
> * Context: Process context. This function is called with the ne_enclave
> mutex held. (note: already available in the first patch)
> * Return: (note: added in the second patch)
> * * 0 ...
>
> > + */
> > +static void
> > +ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions
> *regions,
> > + u64 page_paddr, u64 page_size)
>
> phys_contig_mem_regions => ne_phys_contig_mem_regions
> *regions => *phys_contig_regions
>
> Can define something like this:
>
> unsigned long num = phys_contig_regions->num;
>
> and then use it inside the function, except the last line that
> increments the num.
>
> > +{
> > + /* Physical contiguous, just merge */
>
> Physical contiguous => Physically contiguous
>
> > + if (regions->num &&
> > + (regions->region[regions->num - 1].end + 1) == page_paddr) {
>
> e.g. phys_contig_regions->regions[num - 1]
>
> > + regions->region[regions->num - 1].end += page_size;
> > +
> > + return;
> > + }
> > +
> > + regions->region[regions->num].start = page_paddr;
> > + regions->region[regions->num].end = page_paddr + page_size - 1;
> > + regions->num++;
> > +}
> > +
> > +/**
> > * ne_set_user_memory_region_ioctl() - Add user space memory region to the
> slot
> > * associated with the current enclave.
> > * @ne_enclave : Private data associated with the current enclave.
> > @@ -843,9 +881,9 @@ static int ne_set_user_memory_region_ioctl(struct
> ne_enclave *ne_enclave,
> > unsigned long max_nr_pages = 0;
> > unsigned long memory_size = 0;
> > struct ne_mem_region *ne_mem_region = NULL;
> > - unsigned long nr_phys_contig_mem_regions = 0;
> > struct pci_dev *pdev = ne_devs.ne_pci_dev->pdev;
> > - struct page **phys_contig_mem_regions = NULL;
> > + struct phys_contig_mem_regions *phys_contig_mem_regions = NULL;
>
> struct phys_contig_mem_regions *phys_contig_mem_regions = NULL; =>
> struct ne_phys_contig_mem_regions phys_contig_mem_regions = {};
>
> > + size_t size_to_alloc = 0;
> > int rc = -EINVAL;
> >
> > rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
> > @@ -866,8 +904,9 @@ static int ne_set_user_memory_region_ioctl(struct
> ne_enclave *ne_enclave,
> > goto free_mem_region;
> > }
> >
> > - phys_contig_mem_regions = kcalloc(max_nr_pages,
> sizeof(*phys_contig_mem_regions),
> > - GFP_KERNEL);
> > + size_to_alloc = sizeof(*phys_contig_mem_regions) +
> > + max_nr_pages * sizeof(struct range);
> > + phys_contig_mem_regions = kzalloc(size_to_alloc, GFP_KERNEL);
>
> Then can alloc memory for the regions field.
>
> phys_contig_mem_regions.regions = kcalloc(max_nr_pages,
> sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
>
> The "size_of_alloc" will not be needed in this case.
>
> > if (!phys_contig_mem_regions) {
> > rc = -ENOMEM;
> >
> > @@ -901,26 +940,16 @@ static int ne_set_user_memory_region_ioctl(struct
> ne_enclave *ne_enclave,
> > if (rc < 0)
> > goto put_pages;
> >
> > - /*
> > - * TODO: Update once handled non-contiguous memory regions
> > - * received from user space or contiguous physical memory regions
> > - * larger than 2 MiB e.g. 8 MiB.
> > - */
> > - phys_contig_mem_regions[i] = ne_mem_region->pages[i];
> > +
> ne_merge_phys_contig_memory_regions(phys_contig_mem_regions,
>
> And can pass it here like this: &phys_contig_mem_regions.
>
> > +
> page_to_phys(ne_mem_region->pages[i]),
> > +
> page_size(ne_mem_region->pages[i]));
> >
> > memory_size += page_size(ne_mem_region->pages[i]);
> >
> > ne_mem_region->nr_pages++;
> > } while (memory_size < mem_region.memory_size);
> >
> > - /*
> > - * TODO: Update once handled non-contiguous memory regions received
> > - * from user space or contiguous physical memory regions larger than
> > - * 2 MiB e.g. 8 MiB.
> > - */
> > - nr_phys_contig_mem_regions = ne_mem_region->nr_pages;
> > -
> > - if ((ne_enclave->nr_mem_regions + nr_phys_contig_mem_regions) >
> > + if ((ne_enclave->nr_mem_regions + phys_contig_mem_regions->num) >
> > ne_enclave->max_mem_regions) {
> > dev_err_ratelimited(ne_misc_dev.this_device,
> > "Reached max memory regions %lld\n",
> > @@ -931,9 +960,10 @@ static int ne_set_user_memory_region_ioctl(struct
> ne_enclave *ne_enclave,
> > goto put_pages;
> > }
> >
> > - for (i = 0; i < nr_phys_contig_mem_regions; i++) {
> > - u64 phys_region_addr =
> page_to_phys(phys_contig_mem_regions[i]);
> > - u64 phys_region_size = page_size(phys_contig_mem_regions[i]);
> > + for (i = 0; i < phys_contig_mem_regions->num; i++) {
> > + struct range *range = phys_contig_mem_regions->region + i;
> > + u64 phys_region_addr = range->start;
> > + u64 phys_region_size = range_len(range);
>
> With the updated data structure field, could be:
>
> u64 phys_region_addr = phys_contig_mem_regions.regions[i].start;
> u64 phys_region_size = range_len(&phys_contig_mem_regions.regions[i]);
>
> >
> > if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
> > dev_err_ratelimited(ne_misc_dev.this_device,
> > @@ -959,13 +989,14 @@ static int ne_set_user_memory_region_ioctl(struct
> ne_enclave *ne_enclave,
> >
> > list_add(&ne_mem_region->mem_region_list_entry,
> &ne_enclave->mem_regions_list);
> >
> > - for (i = 0; i < nr_phys_contig_mem_regions; i++) {
> > + for (i = 0; i < phys_contig_mem_regions->num; i++) {
> > struct ne_pci_dev_cmd_reply cmd_reply = {};
> > struct slot_add_mem_req slot_add_mem_req = {};
> > + struct range *range = phys_contig_mem_regions->region + i;
> >
> > slot_add_mem_req.slot_uid = ne_enclave->slot_uid;
> > - slot_add_mem_req.paddr =
> page_to_phys(phys_contig_mem_regions[i]);
> > - slot_add_mem_req.size =
> page_size(phys_contig_mem_regions[i]);
> > + slot_add_mem_req.paddr = range->start;
> > + slot_add_mem_req.size = range_len(range);
>
> Similarly here:
>
> slot_add_mem_req.paddr = phys_contig_mem_regions.regions[i].start;
> slot_add_mem_req.size = range_len(&phys_contig_mem_regions.regions[i]);
>
> Then remain the kfree paths for "phys_contig_mem_regions.regions"
> instead of "phys_contig_mem_regions".
>
> Thanks,
> Andra
>
> >
> > rc = ne_do_request(pdev, SLOT_ADD_MEM,
> > &slot_add_mem_req,
> sizeof(slot_add_mem_req),
> > --
> > 1.8.3.1
> >
>
>
>
> 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