Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro <viro@zeniv.linux.org.uk>]

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...