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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
