On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:
> 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.
The file comes from io_uring_get_file(), the entire thing is within the
io_ring_ctx constructor and the only side effect there is ->ring_sock
creation. And that stays until the io_ring_ctx is freed. I'm _not_
saying I like io_uring style in general, BTW - in particular,
->ring_sock->file handling is a kludge (as is too much of interation
with AF_UNIX machinery there). But from side effects POV we are fine
there.
> 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;
> }
... and it's actually broken, since this
/* All copies must be 32-bit aligned and 32-bit size */
if (!check_buffer(alloc, buffer, buffer_offset, bytes))
return -EINVAL;
in binder_alloc_copy_to_buffer() should've been done *before*
fd_install(). If anything, it's an example of the situation when
fd_receive() would be wrong...
