From: Al Viro <viro@zeniv.linux.org.uk> To: Christian Brauner <christian.brauner@ubuntu.com> Cc: Greg KH <gregkh@linuxfoundation.org>, Xie Yongji <xieyongji@bytedance.com>, hch@infradead.org, arve@android.com, tkjos@android.com, maco@android.com, joel@joelfernandes.org, hridya@google.com, surenb@google.com, sargun@sargun.me, keescook@chromium.org, jasowang@redhat.com, devel@driverdev.osuosl.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process Date: Fri, 16 Apr 2021 14:09:35 +0000 [thread overview] Message-ID: <YHmanzAMdeCtZUjy@zeniv-ca.linux.org.uk> (raw) In-Reply-To: <20210416134252.v3zfjp36tpk33tqz@wittgenstein> On Fri, Apr 16, 2021 at 03:42:52PM +0200, Christian Brauner wrote: > > > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc. > > > > FWIW, pretty much all ioctls that return descriptor as part of a structure > > stored to user-supplied address tend to be that way; some don't have any > > other output fields (in which case they probably would've been better off > > with just passing the descriptor as return value of ioctl(2)). Those > > might be served by that receive_fd_user() helper; anything that has several > > outputs won't be. The same goes for anything that has hard-to-undo > > operations as part of what they need to do: > > reserve fd > > set file up > > do hard-to-undo stuff > > install into descriptor table > > is the only feasible order of operations - reservation can fail, so > > it must go before the hard-to-undo part and install into descriptor > > table can't be undone at all, so it must come last. Looks like > > e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of > > that sort... > > If receive_fd() or your receive_fd_user() proposal can replace a chunk My what proposal? The thing is currently in linux/file.h, put there by Kees half a year ago... > of open-coded places in modules where the split between reserving the > file descriptor and installing it is pointless it's probably already > worth it. A helper for use in some of the simplest cases, with big fat warnings not to touch if the things are not entirely trivial - sure, why not, same as we have anon_inode_getfd(). But that's a convenience helper, not a general purpose primitive. > Random example from io_uring where the file is already opened > way before (which yes, isn't a module afaik but another place where we > have that pattern): > > static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file) > { > int ret, fd; > > fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC); > if (fd < 0) > return fd; > > ret = io_uring_add_task_file(ctx); Huh? It's ret = io_uring_add_task_file(ctx, file); in the mainline and I don't see how that sucker could work without having file passed to it. > if (ret) { > put_unused_fd(fd); > return ret; > } > fd_install(fd, file); > return fd; > } ... and that's precisely the situation where we have something that is not obvious how to undo; look into io_uring_add_task_file()... We have three things to do: (1) reserve a descriptor, (2) io_uring_add_task_file(), (3) install the file. (1) and (2) may fail, (1) is trivial to undo, (2) might be not, (3) is impossible to undo. So I'd say that in this particular case io_uring is being perfectly reasonable...
WARNING: multiple messages have this Message-ID (diff)
From: Al Viro <viro@zeniv.linux.org.uk> To: Christian Brauner <christian.brauner@ubuntu.com> Cc: devel@driverdev.osuosl.org, tkjos@android.com, keescook@chromium.org, Greg KH <gregkh@linuxfoundation.org>, jasowang@redhat.com, linux-fsdevel@vger.kernel.org, sargun@sargun.me, hch@infradead.org, Xie Yongji <xieyongji@bytedance.com>, arve@android.com, joel@joelfernandes.org, hridya@google.com, maco@android.com, surenb@google.com Subject: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process Date: Fri, 16 Apr 2021 14:09:35 +0000 [thread overview] Message-ID: <YHmanzAMdeCtZUjy@zeniv-ca.linux.org.uk> (raw) In-Reply-To: <20210416134252.v3zfjp36tpk33tqz@wittgenstein> On Fri, Apr 16, 2021 at 03:42:52PM +0200, Christian Brauner wrote: > > > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc. > > > > FWIW, pretty much all ioctls that return descriptor as part of a structure > > stored to user-supplied address tend to be that way; some don't have any > > other output fields (in which case they probably would've been better off > > with just passing the descriptor as return value of ioctl(2)). Those > > might be served by that receive_fd_user() helper; anything that has several > > outputs won't be. The same goes for anything that has hard-to-undo > > operations as part of what they need to do: > > reserve fd > > set file up > > do hard-to-undo stuff > > install into descriptor table > > is the only feasible order of operations - reservation can fail, so > > it must go before the hard-to-undo part and install into descriptor > > table can't be undone at all, so it must come last. Looks like > > e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of > > that sort... > > If receive_fd() or your receive_fd_user() proposal can replace a chunk My what proposal? The thing is currently in linux/file.h, put there by Kees half a year ago... > of open-coded places in modules where the split between reserving the > file descriptor and installing it is pointless it's probably already > worth it. A helper for use in some of the simplest cases, with big fat warnings not to touch if the things are not entirely trivial - sure, why not, same as we have anon_inode_getfd(). But that's a convenience helper, not a general purpose primitive. > Random example from io_uring where the file is already opened > way before (which yes, isn't a module afaik but another place where we > have that pattern): > > static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file) > { > int ret, fd; > > fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC); > if (fd < 0) > return fd; > > ret = io_uring_add_task_file(ctx); Huh? It's ret = io_uring_add_task_file(ctx, file); in the mainline and I don't see how that sucker could work without having file passed to it. > if (ret) { > put_unused_fd(fd); > return ret; > } > fd_install(fd, file); > return fd; > } ... and that's precisely the situation where we have something that is not obvious how to undo; look into io_uring_add_task_file()... We have three things to do: (1) reserve a descriptor, (2) io_uring_add_task_file(), (3) install the file. (1) and (2) may fail, (1) is trivial to undo, (2) might be not, (3) is impossible to undo. So I'd say that in this particular case io_uring is being perfectly reasonable... _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2021-04-16 14:10 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-01 9:09 [PATCH 0/2] Export receive_fd() to modules and do some cleanups Xie Yongji 2021-04-01 9:09 ` Xie Yongji 2021-04-01 9:09 ` [PATCH 1/2] file: Export receive_fd() to modules Xie Yongji 2021-04-01 9:09 ` Xie Yongji 2021-04-01 9:52 ` Greg KH 2021-04-01 9:52 ` Greg KH 2021-04-01 10:24 ` Yongji Xie 2021-04-01 10:24 ` Yongji Xie 2021-04-01 9:09 ` [PATCH 2/2] binder: Use receive_fd() to receive file from another process Xie Yongji 2021-04-01 9:09 ` Xie Yongji 2021-04-01 9:54 ` Greg KH 2021-04-01 9:54 ` Greg KH 2021-04-01 10:12 ` Yongji Xie 2021-04-01 10:12 ` Yongji Xie 2021-04-01 10:42 ` Greg KH 2021-04-01 10:42 ` Greg KH 2021-04-01 11:29 ` Yongji Xie 2021-04-01 11:29 ` Yongji Xie 2021-04-01 11:33 ` Greg KH 2021-04-01 11:33 ` Greg KH 2021-04-01 12:28 ` Yongji Xie 2021-04-01 12:28 ` Yongji Xie 2021-04-01 14:09 ` Greg KH 2021-04-01 14:09 ` Greg KH 2021-04-02 9:12 ` Kees Cook 2021-04-02 9:12 ` Kees Cook 2021-04-01 10:40 ` Christian Brauner 2021-04-01 10:40 ` Christian Brauner 2021-04-01 11:11 ` Yongji Xie 2021-04-01 11:11 ` Yongji Xie 2021-04-16 5:19 ` Al Viro 2021-04-16 5:19 ` Al Viro 2021-04-16 5:55 ` Al Viro 2021-04-16 5:55 ` Al Viro 2021-04-16 13:42 ` Christian Brauner 2021-04-16 13:42 ` Christian Brauner 2021-04-16 14:09 ` Al Viro [this message] 2021-04-16 14:09 ` Al Viro 2021-04-16 15:13 ` Christian Brauner 2021-04-16 15:13 ` Christian Brauner 2021-04-16 15:35 ` Al Viro 2021-04-16 15:35 ` Al Viro 2021-04-16 15:58 ` Christian Brauner 2021-04-16 15:58 ` Christian Brauner 2021-04-16 16:00 ` Christian Brauner 2021-04-16 16:00 ` Christian Brauner 2021-04-16 17:00 ` Al Viro 2021-04-16 17:00 ` Al Viro 2021-04-16 17:30 ` Al Viro 2021-04-16 17:30 ` Al Viro 2021-04-17 1:30 ` Al Viro 2021-04-17 1:30 ` Al Viro 2021-04-01 9:53 ` [PATCH 0/2] Export receive_fd() to modules and do some cleanups Greg KH 2021-04-01 9:53 ` Greg KH 2021-04-01 10:00 ` Yongji Xie 2021-04-01 10:00 ` Yongji Xie 2021-04-01 10:20 ` Christian Brauner 2021-04-01 10:20 ` Christian Brauner
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=YHmanzAMdeCtZUjy@zeniv-ca.linux.org.uk \ --to=viro@zeniv.linux.org.uk \ --cc=arve@android.com \ --cc=christian.brauner@ubuntu.com \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=hch@infradead.org \ --cc=hridya@google.com \ --cc=jasowang@redhat.com \ --cc=joel@joelfernandes.org \ --cc=keescook@chromium.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=maco@android.com \ --cc=sargun@sargun.me \ --cc=surenb@google.com \ --cc=tkjos@android.com \ --cc=xieyongji@bytedance.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.