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 05:55:16 +0000 [thread overview]
Message-ID: <YHkmxCyJ8yekgGKl@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <YHkedhnn1wdVFTV3@zeniv-ca.linux.org.uk>
On Fri, Apr 16, 2021 at 05:19:50AM +0000, Al Viro wrote:
> On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote:
> > and see whether all of them can be switched to simply using
> > receive_fd(). I did a completely untested rough sketch to illustrate
> > what I meant by using binder and devpts Xie seems to have just picked
> > those two. But the change is obviously only worth it if all or nearly
> > all callers can be switched over without risk of regression.
> > It would most likely simplify quite a few codepaths though especially in
> > the error paths since we can get rid of put_unused_fd() cleanup.
> >
> > But it requires buy in from others obviously.
>
> Opening a file can have non-trivial side effects; reserving a descriptor
> can't. Moreover, if you look at the second hit in your list, you'll see
> that we do *NOT* want that combined thing there - fd_install() is
> completely irreversible, so we can't do that until we made sure the
> reply (struct vtpm_proxy_new_dev) has been successfully copied to
> userland. No, your receive_fd_user() does not cover that.
>
> There's a bunch of other cases like that - the next ones on your list
> 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...
next prev parent reply other threads:[~2021-04-16 5:55 UTC|newest]
Thread overview: 29+ 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 ` [PATCH 1/2] file: Export receive_fd() to modules Xie Yongji
2021-04-01 9:52 ` Greg KH
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:54 ` Greg KH
2021-04-01 10:12 ` Yongji Xie
2021-04-01 10:42 ` Greg KH
2021-04-01 11:29 ` Yongji Xie
2021-04-01 11:33 ` Greg KH
2021-04-01 12:28 ` Yongji Xie
2021-04-01 14:09 ` Greg KH
2021-04-02 9:12 ` Kees Cook
2021-04-01 10:40 ` Christian Brauner
2021-04-01 11:11 ` Yongji Xie
2021-04-16 5:19 ` Al Viro
2021-04-16 5:55 ` Al Viro [this message]
2021-04-16 13:42 ` Christian Brauner
2021-04-16 14:09 ` Al Viro
2021-04-16 15:13 ` Christian Brauner
2021-04-16 15:35 ` Al Viro
2021-04-16 15:58 ` Christian Brauner
2021-04-16 16:00 ` Christian Brauner
2021-04-16 17:00 ` Al Viro
2021-04-16 17: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 10:00 ` Yongji Xie
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=YHkmxCyJ8yekgGKl@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).