Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?

[Laurent Pinchart]

Hi Daniel,

On Monday 09 Jan 2017 11:23:23 Daniel Vetter wrote:
> On Fri, Jan 06, 2017 at 01:04:55PM -0800, Chad Versace wrote:
> > Was this a mistake in the API? If so, can we fix this ABI mistake before
> > kernel consumers rely on this?
> > 
> > I naïvely expected that OUT_FENCE_PTR would be a pointer to, obviously, a
> > fence fd (s32 __user *). But it's not. It's s64 __user *. Due to that
> > surprise, I spent several hours chasing down weird corruption in Rob
> > Clark's kmscube. The kernel unexpectedly cleared the 32 bits *above* an
> > `int kms_fence_fd` in userspace.
> 
> Never use unsized types for uabi. I guess we could have used s32, but then
> someone is going to store this in a long and it goes boom on 64 bit,

Why so ? And why do we care ? The commonly accepted practice is to store file 
descriptors in int variables. s32 is an int on all platforms, so that's fine 
too. If we use a s32 pointer here, and someone decides to store it in a long, 
bool or cast it to a complex, that's their problem :-)

> while it works on 32 bit. "int" doesn't have that problem directly, but it's
> still a red flag imo.
>
> > For reference, here's the relevant DRM code.
> > 
> >     // file: drivers/gpu/drm/drm_atomic.c
> >     struct drm_out_fence_state {
> >             s64 __user *out_fence_ptr;
> >             struct sync_file *sync_file;
> >             int fd;
> >     };
> >     
> >     static int setup_out_fence(struct drm_out_fence_state *fence_state,
> >                                struct dma_fence *fence)
> >     {
> >             fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> >             if (fence_state->fd < 0)
> >                     return fence_state->fd;
> >             
> >             if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> >                     return -EFAULT;
> >             
> >             fence_state->sync_file = sync_file_create(fence);
> >             if (!fence_state->sync_file)
> >                     return -ENOMEM;
> >             
> >             return 0;
> >     }

-- 
Regards,

Laurent Pinchart