| Message ID | 20210825235234.153013-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] x86/sgx: Add the missing ifdef for sgx_set_attribute() | expand |
On Thu, Aug 26, 2021 at 02:52:32AM +0300, Jarkko Sakkinen wrote: > Similarly as sgx_virt_*, decorate sgx_set_attribute() with ifdef, so that > calling it without appropraite config flags, will cause a compilation > error, and not a linking error. Please explain what exactly is this fixing. IOW, how can I reproduce the failure?
On Thu, 2021-08-26 at 11:58 +0200, Borislav Petkov wrote: > On Thu, Aug 26, 2021 at 02:52:32AM +0300, Jarkko Sakkinen wrote: > > Similarly as sgx_virt_*, decorate sgx_set_attribute() with ifdef, so that > > calling it without appropraite config flags, will cause a compilation > > error, and not a linking error. > > Please explain what exactly is this fixing. IOW, how can I reproduce the > failure? You're right, fixes tag is not necessary. I made this change because I'm including the header to set_memory.c, and IMHO it is better to make sure when possible that we get compilation errors than linker errors, if for some reason kernel did not have SGX support. It's also incoherent that KVM specific functions are compilation flagged but sgx_set_attribute() is not. /Jarkko
On Thu, Aug 26, 2021 at 07:08:07PM +0300, Jarkko Sakkinen wrote: > I made this change because I'm including the header to set_memory.c, and This is something you're doing locally, I presume. If so, you can keep this patch local too. > It's also incoherent that KVM specific functions are compilation flagged They don't really have to be - they're just declarations.
On Thu, 2021-08-26 at 18:35 +0200, Borislav Petkov wrote: > On Thu, Aug 26, 2021 at 07:08:07PM +0300, Jarkko Sakkinen wrote: > > I made this change because I'm including the header to set_memory.c, and > > This is something you're doing locally, I presume. If so, you can keep > this patch local too. > > > It's also incoherent that KVM specific functions are compilation flagged > > They don't really have to be - they're just declarations. Let me check. Is your preference is to have in set_memory.c: #ifdef CONFIG_X86_SGX #include <asm/sgx.h> #endif instead of doing this in uapi/asm/sgx.h? /Jarkko
On Thu, Aug 26, 2021 at 08:11:27PM +0300, Jarkko Sakkinen wrote:
> Is your preference is to have in set_memory.c:
My preference is to take fixes only for actual problems which can be
triggered with some config - not something you're doing locally.
On Thu, 2021-08-26 at 19:24 +0200, Borislav Petkov wrote: > On Thu, Aug 26, 2021 at 08:11:27PM +0300, Jarkko Sakkinen wrote: > > Is your preference is to have in set_memory.c: > > My preference is to take fixes only for actual problems which can be > triggered with some config - not something you're doing locally. The actual problem is to use it in set_memory.c. This the unsplit version: https://lore.kernel.org/linux-sgx/20210818132509.545997-1-jarkko@kernel.org/ Should I just squash them again into one patch? I did the split because of earlier review comments. /Jarkko
On Thu, Aug 26, 2021 at 08:31:13PM +0300, Jarkko Sakkinen wrote: > The actual problem is to use it in set_memory.c. So I asked you in the first mail: "Please explain what exactly is this fixing." > This the unsplit version: > > https://lore.kernel.org/linux-sgx/20210818132509.545997-1-jarkko@kernel.org/ But you're still feeding me some pieces of the puzzle piecemeal. > Should I just squash them again into one patch? You should explain *why* you're fixing whatever you're fixing and your commit message should explain exactly how the bug is triggered so that the reader of that commit message can reproduce it on their end. Otherwise it'll get ignored until you do it right.
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index 05f3e21f01a7..996e56590a10 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -365,6 +365,11 @@ struct sgx_sigstruct { * comment! */ +#ifdef CONFIG_X86_SGX + +int sgx_set_attribute(unsigned long *allowed_attributes, + unsigned int attribute_fd); + #ifdef CONFIG_X86_SGX_KVM int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs, int *trapnr); @@ -372,7 +377,6 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token, void __user *secs, u64 *lepubkeyhash, int *trapnr); #endif -int sgx_set_attribute(unsigned long *allowed_attributes, - unsigned int attribute_fd); +#endif /* CONFIG_X86_SGX */ #endif /* _ASM_X86_SGX_H */
Similarly as sgx_virt_*, decorate sgx_set_attribute() with ifdef, so that calling it without appropraite config flags, will cause a compilation error, and not a linking error. Fixes: b3754e5d3da3 ("x86/sgx: Move provisioning device creation out of SGX driver") Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v3: * Since CONFIG_X86_SGX_KVM depends on CONFIG_X86_SGX surround everything with CONFIG_X86_SGX config flag. Link: https://lore.kernel.org/linux-sgx/YR6Bs2twT4EK%2FjUK@google.com/ --- arch/x86/include/asm/sgx.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)