Am 05.05.22 um 10:10 schrieb Jason Ekstrand:
On Thu, May 5, 2022 at 1:25 AM Christian König
<christian.koe...@amd.com> 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
