| Message ID | 20170907184226.27482-15-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > Rename __close_fd() to close_fd() and export it to be able close files > in modules using file descriptors. > > The usecase that motivates this change happens in V4L2 where we send > events to userspace with a fd that has file installed in it. But if for > some reason we have to cancel the video stream we need to close the files > that haven't been shared with userspace yet. Thus the export of > close_fd() becomes necessary. > > fd_install() happens when we call an ioctl to queue a buffer, but we only > share the fd with userspace later, and that may happen in a kernel thread > instead. What do you mean? A file descriptor is shared with userspace as soon as it's installed in the fdtable by fd_install(). As soon as it's there, another thread can use it (or close it, duplicate it, etc.), even before the syscall that installed it returns... Eric
On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > Rename __close_fd() to close_fd() and export it to be able close files > in modules using file descriptors. > > The usecase that motivates this change happens in V4L2 where we send > events to userspace with a fd that has file installed in it. But if for > some reason we have to cancel the video stream we need to close the files > that haven't been shared with userspace yet. Thus the export of > close_fd() becomes necessary. > > fd_install() happens when we call an ioctl to queue a buffer, but we only > share the fd with userspace later, and that may happen in a kernel thread > instead. NAK. As soon as the reference is in descriptor table, you *can't* do anything to it. This "sharing" part is complete BS - being _told_ that descriptor is there does not matter at all. That descriptor might be hit with dup2() as soon as fd_install() has happened. Or be closed, or any number of other things. You can not take it back. Once fd_install() is done, it's fucking done, period. If V4L2 requires removing it from descriptor table, it's a shitty API and needs to be fixed. Again, NAK.
2017-09-07 Al Viro <viro@ZenIV.linux.org.uk>: > On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > > > Rename __close_fd() to close_fd() and export it to be able close files > > in modules using file descriptors. > > > > The usecase that motivates this change happens in V4L2 where we send > > events to userspace with a fd that has file installed in it. But if for > > some reason we have to cancel the video stream we need to close the files > > that haven't been shared with userspace yet. Thus the export of > > close_fd() becomes necessary. > > > > fd_install() happens when we call an ioctl to queue a buffer, but we only > > share the fd with userspace later, and that may happen in a kernel thread > > instead. > > NAK. As soon as the reference is in descriptor table, you *can't* do anything > to it. This "sharing" part is complete BS - being _told_ that descriptor is > there does not matter at all. That descriptor might be hit with dup2() as > soon as fd_install() has happened. Or be closed, or any number of other things. > > You can not take it back. Once fd_install() is done, it's fucking done, period. > If V4L2 requires removing it from descriptor table, it's a shitty API and needs > to be fixed. Sorry for my lack of knowledge here and thank you for the explanation, things are a lot clear to me. For some reasons I were trying to delay the sharing of the fd to a event later. I can delay the install of it but that my require __fd_install() to be available and exportedi as it may happen in a thread, but I believe you wouldn't be okay with that either, is that so? Gustavo
On Thu, Sep 07, 2017 at 06:22:45PM -0300, Gustavo Padovan wrote: > Sorry for my lack of knowledge here and thank you for the explanation, > things are a lot clear to me. For some reasons I were trying to delay > the sharing of the fd to a event later. I can delay the install of it > but that my require __fd_install() to be available and exportedi as it > may happen in a thread, but I believe you wouldn't be okay with that either, > is that so? Only if it has been given a reference to descriptor table to start with. Which reference should've been acquired by the target process itself. Why bother, anyway? You need to handle the case when the stream has ended just after you'd copied the value to userland; at that point you obviously can't go hunting for all references to struct file in question, so you have to guaratee that methods will start giving an error from that point on. What's the problem with just leaving it installed? Both userland and kernel must cope with that sort of thing anyway, so what does removing it from descriptor table and not reporting it buy you? AFAICS, it's an extra layer of complexity for no good reason - you are not getting it offset by simplifications anywhere else...
On 09/07/2017 08:42 PM, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > Rename __close_fd() to close_fd() and export it to be able close files > in modules using file descriptors. > > The usecase that motivates this change happens in V4L2 where we send > events to userspace with a fd that has file installed in it. But if for > some reason we have to cancel the video stream we need to close the files > that haven't been shared with userspace yet. Thus the export of > close_fd() becomes necessary. > > fd_install() happens when we call an ioctl to queue a buffer, but we only > share the fd with userspace later, and that may happen in a kernel thread > instead. This isn't the way to do this. You should only create the out fence file descriptor when userspace dequeues (i.e. calls VIDIOC_DQEVENT) the BUF_QUEUED event. That's when you give it to userspace and at that moment closing the fd is the responsibility of userspace. There is no point creating it earlier anyway since userspace can't get to it until it dequeues the event. It does mean some more work in the V4L2 core since you need to hook into the DQEVENT code in order to do this. Regards, Hans > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: linux-fsdevel@vger.kernel.org > Cc: Riley Andrews <riandrews@android.com> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com> > --- > This is more like a question, I don't know how unhappy people will be with > this proposal to expose close_fd(). I'm all ears for more interesting > ways of doing it! > --- > drivers/android/binder.c | 2 +- > fs/file.c | 5 +++-- > fs/open.c | 2 +- > include/linux/fdtable.h | 2 +- > 4 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index f7665c31feca..5a9bc73012df 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -440,7 +440,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd) > if (proc->files == NULL) > return -ESRCH; > > - retval = __close_fd(proc->files, fd); > + retval = close_fd(proc->files, fd); > /* can't restart close syscall because file table entry was cleared */ > if (unlikely(retval == -ERESTARTSYS || > retval == -ERESTARTNOINTR || > diff --git a/fs/file.c b/fs/file.c > index 1fc7fbbb4510..111d387ac190 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -618,7 +618,7 @@ EXPORT_SYMBOL(fd_install); > /* > * The same warnings as for __alloc_fd()/__fd_install() apply here... > */ > -int __close_fd(struct files_struct *files, unsigned fd) > +int close_fd(struct files_struct *files, unsigned fd) > { > struct file *file; > struct fdtable *fdt; > @@ -640,6 +640,7 @@ int __close_fd(struct files_struct *files, unsigned fd) > spin_unlock(&files->file_lock); > return -EBADF; > } > +EXPORT_SYMBOL(close_fd); > > void do_close_on_exec(struct files_struct *files) > { > @@ -856,7 +857,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags) > struct files_struct *files = current->files; > > if (!file) > - return __close_fd(files, fd); > + return close_fd(files, fd); > > if (fd >= rlimit(RLIMIT_NOFILE)) > return -EBADF; > diff --git a/fs/open.c b/fs/open.c > index 35bb784763a4..30907d967443 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -1152,7 +1152,7 @@ EXPORT_SYMBOL(filp_close); > */ > SYSCALL_DEFINE1(close, unsigned int, fd) > { > - int retval = __close_fd(current->files, fd); > + int retval = close_fd(current->files, fd); > > /* can't restart close syscall because file table entry was cleared */ > if (unlikely(retval == -ERESTARTSYS || > diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h > index 6e84b2cae6ad..511fd38d5e4b 100644 > --- a/include/linux/fdtable.h > +++ b/include/linux/fdtable.h > @@ -115,7 +115,7 @@ extern int __alloc_fd(struct files_struct *files, > unsigned start, unsigned end, unsigned flags); > extern void __fd_install(struct files_struct *files, > unsigned int fd, struct file *file); > -extern int __close_fd(struct files_struct *files, > +extern int close_fd(struct files_struct *files, > unsigned int fd); > > extern struct kmem_cache *files_cachep; >
On Fri, 2017-09-08 at 00:09 +0200, Hans Verkuil wrote: > On 09/07/2017 08:42 PM, Gustavo Padovan wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.com> > > > > Rename __close_fd() to close_fd() and export it to be able close > > files > > in modules using file descriptors. > > > > The usecase that motivates this change happens in V4L2 where we > > send > > events to userspace with a fd that has file installed in it. But if > > for > > some reason we have to cancel the video stream we need to close the > > files > > that haven't been shared with userspace yet. Thus the export of > > close_fd() becomes necessary. > > > > fd_install() happens when we call an ioctl to queue a buffer, but > > we only > > share the fd with userspace later, and that may happen in a kernel > > thread > > instead. > > This isn't the way to do this. > > You should only create the out fence file descriptor when userspace > dequeues > (i.e. calls VIDIOC_DQEVENT) the BUF_QUEUED event. That's when you > give it to > userspace and at that moment closing the fd is the responsibility of > userspace. > There is no point creating it earlier anyway since userspace can't > get to it > until it dequeues the event. > > It does mean some more work in the V4L2 core since you need to hook > into the > DQEVENT code in order to do this. Right, that makes a lot more sense. I'll change the implementation so it can reflecting that. Thanks. Gustavo
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index f7665c31feca..5a9bc73012df 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -440,7 +440,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd) if (proc->files == NULL) return -ESRCH; - retval = __close_fd(proc->files, fd); + retval = close_fd(proc->files, fd); /* can't restart close syscall because file table entry was cleared */ if (unlikely(retval == -ERESTARTSYS || retval == -ERESTARTNOINTR || diff --git a/fs/file.c b/fs/file.c index 1fc7fbbb4510..111d387ac190 100644 --- a/fs/file.c +++ b/fs/file.c @@ -618,7 +618,7 @@ EXPORT_SYMBOL(fd_install); /* * The same warnings as for __alloc_fd()/__fd_install() apply here... */ -int __close_fd(struct files_struct *files, unsigned fd) +int close_fd(struct files_struct *files, unsigned fd) { struct file *file; struct fdtable *fdt; @@ -640,6 +640,7 @@ int __close_fd(struct files_struct *files, unsigned fd) spin_unlock(&files->file_lock); return -EBADF; } +EXPORT_SYMBOL(close_fd); void do_close_on_exec(struct files_struct *files) { @@ -856,7 +857,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags) struct files_struct *files = current->files; if (!file) - return __close_fd(files, fd); + return close_fd(files, fd); if (fd >= rlimit(RLIMIT_NOFILE)) return -EBADF; diff --git a/fs/open.c b/fs/open.c index 35bb784763a4..30907d967443 100644 --- a/fs/open.c +++ b/fs/open.c @@ -1152,7 +1152,7 @@ EXPORT_SYMBOL(filp_close); */ SYSCALL_DEFINE1(close, unsigned int, fd) { - int retval = __close_fd(current->files, fd); + int retval = close_fd(current->files, fd); /* can't restart close syscall because file table entry was cleared */ if (unlikely(retval == -ERESTARTSYS || diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h index 6e84b2cae6ad..511fd38d5e4b 100644 --- a/include/linux/fdtable.h +++ b/include/linux/fdtable.h @@ -115,7 +115,7 @@ extern int __alloc_fd(struct files_struct *files, unsigned start, unsigned end, unsigned flags); extern void __fd_install(struct files_struct *files, unsigned int fd, struct file *file); -extern int __close_fd(struct files_struct *files, +extern int close_fd(struct files_struct *files, unsigned int fd); extern struct kmem_cache *files_cachep;