On Thu, May 5, 2022 at 1:25 AM Christian König <christian.koenig@xxxxxxx> wrote:
[SNIP]
> + fd_install(fd, sync_file->file);
> +
> + arg.fd = fd;
> + if (copy_to_user(user_data, &arg, sizeof(arg)))
> + return -EFAULT;
I know we had that discussion before, but I'm not 100% any more what the
outcome was.
The problem here is that when the copy_to_user fails we have a file
descriptor which is valid, but userspace doesn't know anything about it.
I only see a few possibilities here:
1. Keep it like this and just assume that a process which you can't copy
the fd to is also dying (a bit to much assumption for my taste).
2. Close the file descriptor when this happens (not ideal either).
3. Instead of returning the fd in the parameter structure return it as
IOCTL result.
Number 3 is what drm_prime_handle_to_fd_ioctl() is doing as well and
IIRC we said that this is probably the best option.
I don't have a strong preference here, so I'll go with whatever in the end but let me at least explain my reasoning. First, this was based on the FD import/export in syncobj which stuffs the FD in the args struct. If `copy_to_user` is a problem here, it's a problem there as well. Second, the only way `copy_to_user` can fail is if the client gives us a read-only page or somehow manages to race removing the page from their address space (via unmap(), for instance) with this ioctl. Both of those seem like pretty serious client errors to me. That, or the client is in the process of dying, in which case we really don't care.
Yeah, I know about that copy_to_user() issue in the syncobj and also some driver specific handling.
That's why we discussed this before and IIRC somebody indeed ran into an issue with -EFAULT and that was the reason all this bubbled up.
I don't have a strong preference either, but I think we should try to learn from previous mistakes and design new interfaces based on such experience.
Christian.
--Jason
