On Wed, Jan 20, 2021, Tianjia Zhang wrote: > In function sgx_encl_create(), the logic of directly assigning > value to attributes_mask determines that the call to > SGX_IOC_ENCLAVE_PROVISION must be after the command of > SGX_IOC_ENCLAVE_CREATE. If move this assignment statement to > function sgx_open(), the PROVISION command can be executed > earlier and more flexibly. Use imperative mood to describe what you're doing, e.g. the "if" leaves the reader wonder whether it's a suggestion or what the patch is actually doing. E.g. something like Move the initialization of an enclave's allowed attributes_mask to sgx_open() to allow the ENCLAVE_PROVISION ioctl() to be invoked before ENCLAVE_CREATE. > Reported-by: Jia Zhang <zhang.jia@xxxxxxxxxxxxxxxxx> > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx> > Signed-off-by: Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx> > --- > arch/x86/kernel/cpu/sgx/driver.c | 3 +++ > arch/x86/kernel/cpu/sgx/ioctl.c | 1 - > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c > index f2eac41bb4ff..8766580194ae 100644 > --- a/arch/x86/kernel/cpu/sgx/driver.c > +++ b/arch/x86/kernel/cpu/sgx/driver.c > @@ -36,6 +36,9 @@ static int sgx_open(struct inode *inode, struct file *file) > return ret; > } > > + encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > + SGX_ATTR_KSS; Probably don't need a newline before SGX_ATTR_KSS. In fact, I wouldn't be surprised if Boris explicitly requested letting the original code go past 80 chars to improve readability. Either way, with a cleaned up changelog: Reviewed-by: Sean Christopherson <seanjc@xxxxxxxxxx> > + > file->private_data = encl; > > return 0; > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 90a5caf76939..1c6ecf9fbeff 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -109,7 +109,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) > encl->base = secs->base; > encl->size = secs->size; > encl->attributes = secs->attributes; > - encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | SGX_ATTR_KSS; > > /* Set only after completion, as encl->lock has not been taken. */ > set_bit(SGX_ENCL_CREATED, &encl->flags); > -- > 2.19.1.3.ge56e4f7 >
