[PATCH 1/2] dma-buf: Add an API for exporting sync files (v13)

[Christian König]

Am 05.05.22 um 10:10 schrieb Jason Ekstrand:
> On Thu, May 5, 2022 at 1:25 AM Christian König 
> <christian.koenig at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220505/d7cdf42a/attachment.htm>