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

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

[Chad Versace]

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.

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

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

[Chad Versace]

+rantogno

Rafael, I finally discovered the source of the bug I was hitting.

On Fri 06 Jan 2017, 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.
> 
> 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;
>     }
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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, while
it works on 32 bit. "int" doesn't have that problem directly, but it's
still a red flag imo.
-Daniel

> 
> 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;
>     }

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Gustavo Padovan]

2017-01-10 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:

> 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 :-)

The only thing that really needs to be s64 here is the OUT_FENCE_PTR
property in the Atomic interface because we carry a pointer there, but
all the manipulation after that is actually done after can easily be
done on s32 or int.

We can't expect that userspace will know that we store as s64 and clear
the bits above if a int was passed down. if we use s32 we will be in
complaince with other linux apis that deals with fds. I'd say we fix
this before it can cause more damage out there.

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Thu, Jan 12, 2017 at 05:17:26PM -0200, Gustavo Padovan wrote:
> 2017-01-10 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> > 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 :-)
> 
> The only thing that really needs to be s64 here is the OUT_FENCE_PTR
> property in the Atomic interface because we carry a pointer there, but
> all the manipulation after that is actually done after can easily be
> done on s32 or int.
> 
> We can't expect that userspace will know that we store as s64 and clear
> the bits above if a int was passed down. if we use s32 we will be in
> complaince with other linux apis that deals with fds. I'd say we fix
> this before it can cause more damage out there.

Changing uabi is kinda tricky, but it's still very new, so if we make sure
it gets applied everywhere and doesn't accidentally ship we can it. Iirc
fences are only in 4.10, so we should be fine ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Daniel,

On Thursday 12 Jan 2017 20:26:40 Daniel Vetter wrote:
> On Thu, Jan 12, 2017 at 05:17:26PM -0200, Gustavo Padovan wrote:
> > 2017-01-10 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> >> 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
> >> :-)
> > 
> > The only thing that really needs to be s64 here is the OUT_FENCE_PTR
> > property in the Atomic interface because we carry a pointer there, but
> > all the manipulation after that is actually done after can easily be
> > done on s32 or int.
> > 
> > We can't expect that userspace will know that we store as s64 and clear
> > the bits above if a int was passed down. if we use s32 we will be in
> > complaince with other linux apis that deals with fds. I'd say we fix
> > this before it can cause more damage out there.
> 
> Changing uabi is kinda tricky, but it's still very new, so if we make sure
> it gets applied everywhere and doesn't accidentally ship we can it. Iirc
> fences are only in 4.10, so we should be fine ...

Correct. That sounds good to me, there's still time for a v4.10-rc fix.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Gustavo Padovan]

2017-01-12 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:

> Hi Daniel,
> 
> On Thursday 12 Jan 2017 20:26:40 Daniel Vetter wrote:
> > On Thu, Jan 12, 2017 at 05:17:26PM -0200, Gustavo Padovan wrote:
> > > 2017-01-10 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > >> 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
> > >> :-)
> > > 
> > > The only thing that really needs to be s64 here is the OUT_FENCE_PTR
> > > property in the Atomic interface because we carry a pointer there, but
> > > all the manipulation after that is actually done after can easily be
> > > done on s32 or int.
> > > 
> > > We can't expect that userspace will know that we store as s64 and clear
> > > the bits above if a int was passed down. if we use s32 we will be in
> > > complaince with other linux apis that deals with fds. I'd say we fix
> > > this before it can cause more damage out there.
> > 
> > Changing uabi is kinda tricky, but it's still very new, so if we make sure
> > it gets applied everywhere and doesn't accidentally ship we can it. Iirc
> > fences are only in 4.10, so we should be fine ...
> 
> Correct. That sounds good to me, there's still time for a v4.10-rc fix.

I'd expect users defining an int and hitting the issue Chad hit instead
of going to long types. So I hope we are safe here. I'll prepare a
patch to make it s32.

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chad Versace]

On Thu 12 Jan 2017, Gustavo Padovan wrote:
> 2017-01-12 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> > Hi Daniel,
> > 
> > On Thursday 12 Jan 2017 20:26:40 Daniel Vetter wrote:
> > > On Thu, Jan 12, 2017 at 05:17:26PM -0200, Gustavo Padovan wrote:
> > > > 2017-01-10 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > > >> 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'd expect users defining an int and hitting the issue Chad hit instead
> of going to long types. So I hope we are safe here. I'll prepare a
> patch to make it s32.

Thanks for fixing this. I'm glad we caught it before the uabi became
frozen.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel