Re: Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?
On Thu 12 Jan 2017, Gustavo Padovan wrote: > 2017-01-12 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 : > > > >> 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
Re: Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?
2017-01-12 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 : > > >> 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?
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: > >> 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?
On Thu, Jan 12, 2017 at 05:17:26PM -0200, Gustavo Padovan wrote: > 2017-01-10 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 :-) > > 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?
2017-01-10 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 :-) 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
Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?
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
Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?
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
Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?
+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 at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Why does OUT_FENCE_PTR point not to an fd (s32) but to an s64?
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; }