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

[Christian Brauner]

On Fri, Apr 16, 2021 at 02:09:35PM +0000, Al Viro wrote:
> 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...

Yeah, I know. I was just referring to that line:

> > > might be served by that receive_fd_user() helper; anything that has several

I wasn't trying to imply you were the author or instigator of the api.

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

My point here was more that the _file_ has already been opened _before_
that call to io_uring_add_task_file(). But any potential non-trivial
side-effects of opening that file that you correctly pointed out in an
earlier mail has already happened by that time. Granted there are more
obvious examples, e.g. the binder stuff.

		int fd = get_unused_fd_flags(O_CLOEXEC);

		if (fd < 0) {
			binder_debug(BINDER_DEBUG_TRANSACTION,
				     "failed fd fixup txn %d fd %d\n",
				     t->debug_id, fd);
			ret = -ENOMEM;
			break;
		}
		binder_debug(BINDER_DEBUG_TRANSACTION,
			     "fd fixup txn %d fd %d\n",
			     t->debug_id, fd);
		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
		fd_install(fd, fixup->file);
		fixup->file = NULL;
		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
						fixup->offset, &fd,
						sizeof(u32))) {
			ret = -EINVAL;
			break;
		}