Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: x86@kernel.org, linux-sgx@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Matthew Wilcox <willy@infradead.org>,
	Jethro Beekman <jethro@fortanix.com>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Chunyang Hui <sanqian.hcy@antfin.com>,
	Jordan Hand <jorhand@linux.microsoft.com>,
	Nathaniel McCallum <npmccallum@redhat.com>,
	Seth Moore <sethmo@google.com>,
	Darren Kenny <darren.kenny@oracle.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Suresh Siddha <suresh.b.siddha@intel.com>,
	andriy.shevchenko@linux.intel.com, asapek@google.com,
	bp@alien8.de, cedric.xing@intel.com, chenalexchen@google.com,
	conradparker@google.com, cyhanish@google.com,
	dave.hansen@intel.com, haitao.huang@intel.com,
	kai.huang@intel.com, kai.svahn@intel.com, kmoy@google.com,
	ludloff@google.com, luto@kernel.org, nhorman@redhat.com,
	puiterwijk@redhat.com, rientjes@google.com, tglx@linutronix.de,
	yaozhangx@google.com, mikko.ylinen@intel.com
Subject: Re: [PATCH v39 11/24] x86/sgx: Add SGX enclave driver
Date: Mon, 5 Oct 2020 15:42:21 +0300
Message-ID: <20201005124221.GA191854@linux.intel.com> (raw)
In-Reply-To: <20201005094246.GB151835@kroah.com>

On Mon, Oct 05, 2020 at 11:42:46AM +0200, Greg KH wrote:
> > > You use gpl-only header files in this file, so how in the world can it
> > > be bsd-3 licensed?
> > > 
> > > Please get your legal department to agree with this, after you explain
> > > to them how you are mixing gpl2-only code in with this file.
> > 
> > I'll do what I already stated that I will do. Should I do something
> > more?
> 
> This was written before your previous response.

OK, that is weird, I got this one some time later.

> > > > +	mutex_lock(&encl->lock);
> > > > +	atomic_or(SGX_ENCL_DEAD, &encl->flags);
> > > 
> > > So you set a flag that this is dead, and then instantly delete it?  Why
> > > does that matter?  I see you check for this flag elsewhere, but as you
> > > are just about to delete this structure, how can this be an issue?
> > 
> > It matters because ksgxswapd (sgx_reclaimer_*) might be processing it.
> 
> I don't see that happening in this patch, did I miss it?

It's implemented in 16/24:

https://lore.kernel.org/linux-sgx/20201004223921.GA48517@linux.intel.com/T/#u

> > It will use the flag to skip the operations that it would do to a victim
> > page, when the enclave is still alive.
> 
> Again, why are you adding flags when the patch does not use them?
> Please put new functionality in the specific patch that uses it.
> 
> And can you really rely on this?  How did sgx_reclaimer_* (whatever that
> is), get the reference on this object in the first place?  Again, I
> don't see that happening at all in here, and at a quick glance in the
> other patches I don't see it there either.  What am I missing?

I went through the patch, and yes, they can be migrated to 16/24.
I agree with this, no excuses.

In 16/24 pages are added to sgx_active_page_list from which they are
swapped by the reclaimer to the main memory when Enclave Page Cache
(EPC), the memory where enclave pages reside, gets full.

When a reclaimer thread takes a victim page from that list, it will also
get a kref to the enclave so that struct sgx_encl instance does not
get wiped while it's doing its job.

> > Because ksgxswapd needs the alive enclave instance while it is in the
> > process of swapping a victim page. The reason for this is the
> > hierarchical nature of the enclave pages.
> > 
> > As an example, a write operation to main memory, EWB (SDM vol 3D 40-79)
> 
> What is that referencing?

https://software.intel.com/content/dam/develop/public/us/en/documents/332831-sdm-vol-3d.pdf

> > needs to access SGX Enclave Control Structure (SECS) page, which is
> > contains global data for an enclave, like the unswapped child count.
> 
> Ok, but how did it get access to this structure in the first place, like
> I ask above?

I guess I answered that, and I also fully agree with your suggestions.

It used to be many iterations ago that enclaves were not file based but
just memory mappings (long story short: was not great way to make them
multiprocess, that's why file centered now), and then refcount played a
bigger role. Having those "extras" in this patch is by no means
intentional but more like cruft of many iterations of refactoring.

Sometimes when you work long with this kind of pile of code, which has
converged through many iterations, you really need someone else to point
some of the simple and obvious things out.

> > There is a patch that adds "sgx/provision".
> 
> What number in this series?

It's 15/24.

> 
> > Either works for me. Should I flatten them to "sgx_enclave" and
> > "sgx_provision", or keep them as they are?
> 
> Having 2 char nodes in a subdir is better than one, I will give you
> that.  But none is even better, don't you think?

I think that having just "sgx_enclave" and "sgx_provision" would be
better.

I've been thinking about this for a while but at the same time try not
to be too proactive without feedback. One reason would be that "enclave"
and "provision" without the subdir are not good identifiers.

I also recalled this discussion:

https://lkml.org/lkml/2019/12/23/158

and was wondering how that subdir would even play with /sys/class/misc,
if we decide to add attributes? Not enough knowledge to answer this.

Anyway, I'll put a note to my backlog on this, and also to move the
previously discussed cruft to the correct patch.

> thanks,
> 
> greg k-h

Thank you.

/Jarkko

  reply index

Thread overview: 117+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-03  4:50 [PATCH v39 00/24] Intel SGX foundations Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 01/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-10-19 14:10   ` Dave Hansen
2020-10-19 17:49     ` Sean Christopherson
2020-10-03  4:50 ` [PATCH v39 02/24] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 03/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 04/24] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 05/24] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2020-10-19 14:30   ` Dave Hansen
2020-10-19 17:38     ` Sean Christopherson
2020-10-19 17:48       ` Dave Hansen
2020-10-19 17:53         ` Sean Christopherson
2020-10-19 17:58           ` Dave Hansen
2020-10-03  4:50 ` [PATCH v39 06/24] x86/cpu/intel: Detect SGX support Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 07/24] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 08/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen
2020-10-19  8:45   ` Jarkko Sakkinen
2020-10-19 12:39     ` Borislav Petkov
2020-10-23  9:01       ` Jarkko Sakkinen
2020-10-19 13:40     ` Dave Hansen
2020-10-23  9:03       ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 09/24] x86/sgx: Add __sgx_alloc_epc_page() and sgx_free_epc_page() Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 11/24] x86/sgx: Add SGX enclave driver Jarkko Sakkinen
2020-10-03 14:39   ` Greg KH
2020-10-04 14:32     ` Jarkko Sakkinen
2020-10-04 15:01       ` Jarkko Sakkinen
2020-10-05  9:42       ` Greg KH
2020-10-05 12:42         ` Jarkko Sakkinen [this message]
2020-10-07 18:09           ` Haitao Huang
2020-10-07 19:26             ` Greg KH
2020-10-09  6:44               ` Jarkko Sakkinen
2020-10-14 20:16                 ` Dave Hansen
2020-10-05  8:45     ` Christoph Hellwig
2020-10-05 11:42       ` Jarkko Sakkinen
2020-10-05 11:50         ` Greg KH
2020-10-05 14:23           ` Jarkko Sakkinen
2020-10-05 15:02             ` Greg KH
2020-10-05 16:40               ` Dave Hansen
2020-10-05 20:02                 ` Jarkko Sakkinen
2020-10-09  7:10     ` Pavel Machek
2020-10-09  7:21       ` Greg KH
2020-10-09  8:21         ` Pavel Machek
2020-10-03 19:54   ` Matthew Wilcox
2020-10-04 21:50     ` Jarkko Sakkinen
2020-10-04 22:02       ` Jarkko Sakkinen
2020-10-04 22:27       ` Matthew Wilcox
2020-10-04 23:41         ` Jarkko Sakkinen
2020-10-05  1:30           ` Matthew Wilcox
2020-10-05  3:06             ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Jarkko Sakkinen
2020-10-16 17:07   ` Dave Hansen
2020-10-18  4:26     ` Jarkko Sakkinen
2020-10-19 20:21       ` Dave Hansen
2020-10-19 20:48         ` Sean Christopherson
2020-10-03  4:50 ` [PATCH v39 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-10-16 21:25   ` Dave Hansen
2020-10-18  5:03     ` Jarkko Sakkinen
2020-10-19  7:03       ` Jarkko Sakkinen
2020-10-19 20:48       ` Dave Hansen
2020-10-19 21:15         ` Sean Christopherson
2020-10-19 21:44           ` Dave Hansen
2020-10-23 10:11             ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION Jarkko Sakkinen
2020-10-20 15:48   ` Dave Hansen
2020-10-23 10:14     ` Jarkko Sakkinen
2020-10-20 21:19   ` Dave Hansen
2020-10-23 10:17     ` Jarkko Sakkinen
2020-10-23 14:19       ` Dave Hansen
2020-10-24 11:34         ` Jarkko Sakkinen
2020-10-24 15:47           ` Andy Lutomirski
2020-10-24 20:23             ` Jarkko Sakkinen
2020-10-27 10:38               ` Dr. Greg
2020-10-23 14:23       ` Jethro Beekman
2020-10-24 11:40         ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 16/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-10-03  5:22   ` Haitao Huang
2020-10-03 13:32     ` Jarkko Sakkinen
2020-10-03 18:23       ` Haitao Huang
2020-10-04 22:39         ` Jarkko Sakkinen
2020-10-07 17:25           ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 17/24] x86/sgx: Add ptrace() support for the SGX driver Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 18/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 19/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 20/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-10-06  2:57   ` Sean Christopherson
2020-10-06  8:30     ` Jethro Beekman
2020-10-06 15:15       ` Sean Christopherson
2020-10-06 17:28         ` Jarkko Sakkinen
2020-10-06 23:21           ` Sean Christopherson
2020-10-07  0:22             ` Jarkko Sakkinen
2020-10-07  1:17               ` Sean Christopherson
2020-10-07  3:14                 ` Jarkko Sakkinen
2020-10-07  4:34                   ` Sean Christopherson
2020-10-07  7:39                     ` Jarkko Sakkinen
2020-10-07  8:04                       ` Jarkko Sakkinen
2020-10-07 15:25                       ` Sean Christopherson
2020-10-07 17:08                         ` Jarkko Sakkinen
2020-10-07 17:13                           ` Jarkko Sakkinen
2020-10-06 15:49       ` Jarkko Sakkinen
2020-10-06 15:36     ` Jarkko Sakkinen
2020-10-06 21:39     ` Jarkko Sakkinen
2020-10-07  0:23       ` Jarkko Sakkinen
2020-10-17  1:48   ` Andy Lutomirski
2020-10-17 21:02     ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 22/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-10-12 16:50   ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 23/24] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 24/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-10-16 21:04   ` Dave Hansen
2020-10-18  4:27     ` Jarkko Sakkinen
2020-10-03 14:32 ` [PATCH v39 00/24] Intel SGX foundations Greg KH
2020-10-03 14:53   ` Jarkko Sakkinen
2020-10-15 19:06 ` Dave Hansen
2020-10-17 20:43   ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201005124221.GA191854@linux.intel.com \
    --to=jarkko.sakkinen@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=asapek@google.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=chenalexchen@google.com \
    --cc=conradparker@google.com \
    --cc=cyhanish@google.com \
    --cc=darren.kenny@oracle.com \
    --cc=dave.hansen@intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=haitao.huang@intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=jorhand@linux.microsoft.com \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=kmoy@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=ludloff@google.com \
    --cc=luto@kernel.org \
    --cc=mikko.ylinen@intel.com \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sanqian.hcy@antfin.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=sethmo@google.com \
    --cc=suresh.b.siddha@intel.com \
    --cc=tglx@linutronix.de \
    --cc=willy@infradead.org \
    --cc=x86@kernel.org \
    --cc=yaozhangx@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org
	public-inbox-index linux-sgx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git