| Message ID | 20171125193132.24321-10-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Repeated NAK - any interface that deals with raw file descriptor table entries has absolutely no business in a driver. Please fix your API already.
On Tue, Nov 28, 2017 at 06:35:04AM -0800, Christoph Hellwig wrote: > Repeated NAK - any interface that deals with raw file descriptor table > entries has absolutely no business in a driver. > > Please fix your API already. Does it make a differnece if the code is moved to arch/x86, which could potentially happen (see Darren's and tglx's comments on v5)? Then the need for export will be gone. /Jarkko
On Tue, Nov 28, 2017 at 10:42:20PM +0200, Jarkko Sakkinen wrote: > On Tue, Nov 28, 2017 at 06:35:04AM -0800, Christoph Hellwig wrote: > > Repeated NAK - any interface that deals with raw file descriptor table > > entries has absolutely no business in a driver. > > > > Please fix your API already. > > Does it make a differnece if the code is moved to arch/x86, which could > potentially happen (see Darren's and tglx's comments on v5)? Then the > need for export will be gone. Yes. You still shall not play nasty games with file descriptors.
On Tue, Nov 28, 2017 at 01:05:51PM -0800, Christoph Hellwig wrote: > On Tue, Nov 28, 2017 at 10:42:20PM +0200, Jarkko Sakkinen wrote: > > On Tue, Nov 28, 2017 at 06:35:04AM -0800, Christoph Hellwig wrote: > > > Repeated NAK - any interface that deals with raw file descriptor table > > > entries has absolutely no business in a driver. > > > > > > Please fix your API already. > > > > Does it make a differnece if the code is moved to arch/x86, which could > > potentially happen (see Darren's and tglx's comments on v5)? Then the > > need for export will be gone. > > Yes. You still shall not play nasty games with file descriptors. I need to put something to file descriptors in order to have a IO channels for the launch enclave hosting process. /Jarkko
On Tue, Nov 28, 2017 at 11:57:53PM +0200, Jarkko Sakkinen wrote: > > Yes. You still shall not play nasty games with file descriptors. > > I need to put something to file descriptors in order to have a IO > channels for the launch enclave hosting process. Just do it like any other program - open it from your userspace program using open() and related syscalls.
On Wed, Nov 29, 2017 at 03:13:57PM -0800, Christoph Hellwig wrote: > On Tue, Nov 28, 2017 at 11:57:53PM +0200, Jarkko Sakkinen wrote: > > > Yes. You still shall not play nasty games with file descriptors. > > > > I need to put something to file descriptors in order to have a IO > > channels for the launch enclave hosting process. > > Just do it like any other program - open it from your userspace > program using open() and related syscalls. In this case it would not work as the launch enclave is still part of the kernel and it would create a dependency how the user space defines paths. If using pipe specifically is an issue, I could easily use shmem file as a mean for communiation. The way I implemented is much like how I did arch/x86/realmode with HPA and it has kind of comparable requirements, part of the kernel but not exactly code living in the kernel namespace. /Jarkko
On Thu, 2017-11-30 at 18:43 +0200, Jarkko Sakkinen wrote: > On Wed, Nov 29, 2017 at 03:13:57PM -0800, Christoph Hellwig wrote: > > > > On Tue, Nov 28, 2017 at 11:57:53PM +0200, Jarkko Sakkinen wrote: > > > > > > > > > > > Yes. You still shall not play nasty games with file > > > > descriptors. > > > > > > I need to put something to file descriptors in order to have a IO > > > channels for the launch enclave hosting process. > > > > Just do it like any other program - open it from your userspace > > program using open() and related syscalls. > > In this case it would not work as the launch enclave is still part of > the kernel and it would create a dependency how the user space > defines paths. If using pipe specifically is an issue, I could easily > use shmem file as a mean for communiation. Can't you simply use sys_pipe2() sys_close() sys_dup2() To achieve the same effect as replace_fd()/create_pipe_files()? The point Christoph is making is that you can call sys_ interfaces from within the kernel (carefully) and have them operate like direct invocations. Look at main.c:kernel_init_freeable() it's doing something similar to what you want, except with the console, not a pipe and it begins with the file table empty. James
On Thu, Nov 30, 2017 at 10:38:30AM -0800, James Bottomley wrote: > On Thu, 2017-11-30 at 18:43 +0200, Jarkko Sakkinen wrote: > > On Wed, Nov 29, 2017 at 03:13:57PM -0800, Christoph Hellwig wrote: > > > > > > On Tue, Nov 28, 2017 at 11:57:53PM +0200, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > Yes. You still shall not play nasty games with file > > > > > descriptors. > > > > > > > > I need to put something to file descriptors in order to have a IO > > > > channels for the launch enclave hosting process. > > > > > > Just do it like any other program - open it from your userspace > > > program using open() and related syscalls. > > > > In this case it would not work as the launch enclave is still part of > > the kernel and it would create a dependency how the user space > > defines paths. If using pipe specifically is an issue, I could easily > > use shmem file as a mean for communiation. > > Can't you simply use > > sys_pipe2() > sys_close() > sys_dup2() > > To achieve the same effect as replace_fd()/create_pipe_files()? > > The point Christoph is making is that you can call sys_ interfaces from > within the kernel (carefully) and have them operate like direct > invocations. Look at main.c:kernel_init_freeable() it's doing > something similar to what you want, except with the console, not a pipe > and it begins with the file table empty. Thank you. I'll take a peek. /Jarkko
On Mon, Dec 04, 2017 at 11:00:59AM +0200, Jarkko Sakkinen wrote: > On Thu, Nov 30, 2017 at 10:38:30AM -0800, James Bottomley wrote: > > On Thu, 2017-11-30 at 18:43 +0200, Jarkko Sakkinen wrote: > > > On Wed, Nov 29, 2017 at 03:13:57PM -0800, Christoph Hellwig wrote: > > > > > > > > On Tue, Nov 28, 2017 at 11:57:53PM +0200, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > > > Yes. You still shall not play nasty games with file > > > > > > descriptors. > > > > > > > > > > I need to put something to file descriptors in order to have a IO > > > > > channels for the launch enclave hosting process. > > > > > > > > Just do it like any other program - open it from your userspace > > > > program using open() and related syscalls. > > > > > > In this case it would not work as the launch enclave is still part of > > > the kernel and it would create a dependency how the user space > > > defines paths. If using pipe specifically is an issue, I could easily > > > use shmem file as a mean for communiation. > > > > Can't you simply use > > > > sys_pipe2() > > sys_close() > > sys_dup2() > > > > To achieve the same effect as replace_fd()/create_pipe_files()? > > > > The point Christoph is making is that you can call sys_ interfaces from > > within the kernel (carefully) and have them operate like direct > > invocations. Look at main.c:kernel_init_freeable() it's doing > > something similar to what you want, except with the console, not a pipe > > and it begins with the file table empty. > > Thank you. I'll take a peek. It doesn't apply here as it depends of the filesystem paths. I think I could replace pipes with anonymous inodes. Is that a better idea than pipes? I can work on that v8 if the export is a show stopper as it seems. We are using that to do some other stuff in tpm_vtpm_proxy. I submitted v7 of the patch set still as a self-contained driver. For that I'm looking forward to get feedback on: 1. Could the first upstream version be a self-contained driver even if some stuff would be eventually moved to arch/x86? There is nothing preventing on doing that and that would be a non-intrusive way to upstream such a large piece of functionality. 2. If for the first upstream version something needs to be placed to arch/x86, I would like to get some guidelines on deployment. I guess it would go under arch/x86/kernel/cpu/sgx? 3. I fixed the AES issue. Is there anything else BIG that needs to be fixed? /Jarkko
On Thu, Dec 07, 2017 at 07:37:38PM +0200, Jarkko Sakkinen wrote: > I think I could replace pipes with anonymous inodes. Is that a better > idea than pipes? I can work on that v8 if the export is a show stopper > as it seems. We are using that to do some other stuff in tpm_vtpm_proxy. I'll go with the anon inode solution. It should be fairly easy to implement. /Jarkko
diff --git a/fs/file.c b/fs/file.c index 1fc7fbbb4510..b1fa28919b22 100644 --- a/fs/file.c +++ b/fs/file.c @@ -871,6 +871,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags) spin_unlock(&files->file_lock); return err; } +EXPORT_SYMBOL_GPL(replace_fd); SYSCALL_DEFINE3(dup3, unsigned int, oldfd, unsigned int, newfd, int, flags) { diff --git a/fs/pipe.c b/fs/pipe.c index 97e5be897753..ee33a84127e7 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -784,6 +784,7 @@ int create_pipe_files(struct file **res, int flags) iput(inode); return err; } +EXPORT_SYMBOL_GPL(create_pipe_files); static int __do_pipe_flags(int *fd, struct file **files, int flags) {
Exported create_pipe_files() and replace_fd() because the SGX driver needs to be able to setup pipes in order to communicate with the helper process that hosts the Launch Enclave (LE). The pipe creation will be done in the init-callback supplied to call_usermodehelper_setup(). The driver will use two pipes for communication with the LE hosting process: * One for writing SIGSTRUCT blobs. * One for reading EINITTOKEN blobs. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- fs/file.c | 1 + fs/pipe.c | 1 + 2 files changed, 2 insertions(+)