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 05:19:50 +0000 [thread overview]
Message-ID: <YHkedhnn1wdVFTV3@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <20210401104034.52qaaoea27htkpbh@wittgenstein>
On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote:
> My suggestion was to look at all the places were we currently open-code
> this in drivers/:
>
> drivers/android/binder.c: int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/char/tpm/tpm_vtpm_proxy.c: fd = get_unused_fd_flags(O_RDWR);
> drivers/dma-buf/dma-buf.c: fd = get_unused_fd_flags(flags);
> drivers/dma-buf/sw_sync.c: int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/dma-buf/sync_file.c: int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpio/gpiolib-cdev.c: fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> drivers/gpio/gpiolib-cdev.c: fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> drivers/gpio/gpiolib-cdev.c: fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c: fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/drm_atomic_uapi.c: fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/drm_lease.c: fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
> drivers/gpu/drm/drm_syncobj.c: fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/drm_syncobj.c: int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c: out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/msm/msm_gem_submit.c: out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/virtio/virtgpu_ioctl.c: out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c: out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/infiniband/core/rdma_core.c: new_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/media/mc/mc-request.c: fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/misc/cxl/api.c: rc = get_unused_fd_flags(flags);
> drivers/scsi/cxlflash/ocxl_hw.c: rc = get_unused_fd_flags(flags);
> drivers/scsi/cxlflash/ocxl_hw.c: dev_err(dev, "%s: get_unused_fd_flags failed rc=%d\n",
> drivers/tty/pty.c: fd = get_unused_fd_flags(flags);
> drivers/vfio/vfio.c: ret = get_unused_fd_flags(O_CLOEXEC);
> drivers/virt/nitro_enclaves/ne_misc_dev.c: enclave_fd = get_unused_fd_flags(O_CLOEXEC);
>
> 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.
So I would consider receive_fd() as an attractive nuisance if it
is ever suggested (and available) for wide use...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2021-04-16 5:20 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 [this message]
2021-04-16 5:55 ` Al Viro
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=YHkedhnn1wdVFTV3@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 NNTP newsgroup(s).