[v9,2/3] kernel.h: add to_user_ptr()

Commit Message

Gustavo Padovan March 17, 2016, 5:30 p.m. UTC

From: Gustavo Padovan <[e-mail]>

This function had copies in 3 different files. Unify them in kernel.h.

Cc: Andrew Morton <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Rob Clark <[e-mail]>
Signed-off-by: Gustavo Padovan <[e-mail]>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 5 -----
 drivers/gpu/drm/i915/i915_drv.h              | 5 -----
 drivers/gpu/drm/msm/msm_gem_submit.c         | 5 -----
 include/linux/kernel.h                       | 5 +++++
 4 files changed, 5 insertions(+), 15 deletions(-)

Comments

Joe Perches March 17, 2016, 5:41 p.m. UTC | #1

On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
> This function had copies in 3 different files. Unify them in
> kernel.h.

This is only used by gpu/drm.

I think this is a poor name for a generic function
that would be in kernel.h.

Isn't there an include file in linux/drm that's
appropriate for this.  Maybe drmP.h

Maybe prefix this function name with drm_ too.

Also, there's this that might conflict:

arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ptr_to_compat(p)
arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ((unsigned long)(p))

Gustavo Padovan March 17, 2016, 6:05 p.m. UTC | #2

2016-03-17 Joe Perches <[e-mail]>:

> On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
> > This function had copies in 3 different files. Unify them in
> > kernel.h.
> 
> This is only used by gpu/drm.
> 
> I think this is a poor name for a generic function
> that would be in kernel.h.
> 
> Isn't there an include file in linux/drm that's
> appropriate for this.  Maybe drmP.h
> 
> Maybe prefix this function name with drm_ too.

No, the next patch adds a user to drivers/staging (which will be moved
to drivers/dma-buf) soon. Maybe move to a different header in
include/linux/? not sure which one.

> Also, there's this that might conflict:
> 
> arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ptr_to_compat(p)
> arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ((unsigned long)(p))

Right, I'll figure out how to replace these two too.

	Gustavo

Gustavo Padovan March 17, 2016, 6:43 p.m. UTC | #3

2016-03-17 Gustavo Padovan <[e-mail]>:

> 2016-03-17 Joe Perches <[e-mail]>:
> 
> > On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
> > > This function had copies in 3 different files. Unify them in
> > > kernel.h.
> > 
> > This is only used by gpu/drm.
> > 
> > I think this is a poor name for a generic function
> > that would be in kernel.h.
> > 
> > Isn't there an include file in linux/drm that's
> > appropriate for this.  Maybe drmP.h
> > 
> > Maybe prefix this function name with drm_ too.
> 
> No, the next patch adds a user to drivers/staging (which will be moved
> to drivers/dma-buf) soon. Maybe move to a different header in
> include/linux/? not sure which one.
> 
> > Also, there's this that might conflict:
> > 
> > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ptr_to_compat(p)
> > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ((unsigned long)(p))
> 
> Right, I'll figure out how to replace these two too.

The powerpc to_user_ptr has a different meaning from the one I'm adding
in this patch. I propose we just rename powerpc's to_user_ptr to
__to_user_ptr and leave the rest as is.

	Gustavo

Joe Perches March 17, 2016, 8:22 p.m. UTC | #4

On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
> 2016-03-17 Gustavo Padovan <[e-mail]>:
> > 2016-03-17 Joe Perches <[e-mail]>:
> > > On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
> > > > 
> > > > This function had copies in 3 different files. Unify them in
> > > > kernel.h.
> > > This is only used by gpu/drm.
> > > 
> > > I think this is a poor name for a generic function
> > > that would be in kernel.h.
> > > 
> > > Isn't there an include file in linux/drm that's
> > > appropriate for this.  Maybe drmP.h
> > > 
> > > Maybe prefix this function name with drm_ too.
> > No, the next patch adds a user to drivers/staging (which will be moved
> > to drivers/dma-buf) soon. Maybe move to a different header in
> > include/linux/? not sure which one.
> > 
> > > 
> > > Also, there's this that might conflict:
> > > 
> > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ptr_to_compat(p)
> > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ((unsigned long)(p))
> > Right, I'll figure out how to replace these two too.
> The powerpc to_user_ptr has a different meaning from the one I'm adding
> in this patch. I propose we just rename powerpc's to_user_ptr to
> __to_user_ptr and leave the rest as is.

I think that's not a good idea, and you should really check
this concept with the powerpc folk (added to to:s and cc:ed)

If it were really added, then the function meaning is incorrect.

This is taking a u64, casting that to (unsigned long/uint_ptr_t),
then converting that to a user pointer.

Does that naming and use make sense on x86-32 or arm32?

Rob Clark March 17, 2016, 8:33 p.m. UTC | #5

On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches <[e-mail]> wrote:
> On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
>> 2016-03-17 Gustavo Padovan <[e-mail]>:
>> > 2016-03-17 Joe Perches <[e-mail]>:
>> > > On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
>> > > >
>> > > > This function had copies in 3 different files. Unify them in
>> > > > kernel.h.
>> > > This is only used by gpu/drm.
>> > >
>> > > I think this is a poor name for a generic function
>> > > that would be in kernel.h.
>> > >
>> > > Isn't there an include file in linux/drm that's
>> > > appropriate for this.  Maybe drmP.h
>> > >
>> > > Maybe prefix this function name with drm_ too.
>> > No, the next patch adds a user to drivers/staging (which will be moved
>> > to drivers/dma-buf) soon. Maybe move to a different header in
>> > include/linux/? not sure which one.
>> >
>> > >
>> > > Also, there's this that might conflict:
>> > >
>> > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ptr_to_compat(p)
>> > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ((unsigned long)(p))
>> > Right, I'll figure out how to replace these two too.
>> The powerpc to_user_ptr has a different meaning from the one I'm adding
>> in this patch. I propose we just rename powerpc's to_user_ptr to
>> __to_user_ptr and leave the rest as is.
>
> I think that's not a good idea, and you should really check
> this concept with the powerpc folk (added to to:s and cc:ed)
>
> If it were really added, then the function meaning is incorrect.
>
> This is taking a u64, casting that to (unsigned long/uint_ptr_t),
> then converting that to a user pointer.
>
> Does that naming and use make sense on x86-32 or arm32?
>

fwiw Gustavo's version of to_user_ptr() is in use on arm32 and arm64..
Not entirely sure what doesn't make sense about it

BR,
-R

Joe Perches March 17, 2016, 8:40 p.m. UTC | #6

On Thu, 2016-03-17 at 16:33 -0400, Rob Clark wrote:
> On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches <[e-mail]> wrote:
> > On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
> > > 2016-03-17 Gustavo Padovan <[e-mail]>:
> > > > 2016-03-17 Joe Perches <[e-mail]>:
> > > > > On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
> > > > > > This function had copies in 3 different files. Unify them in
> > > > > > kernel.h.
> > > > > This is only used by gpu/drm.
> > > > > 
> > > > > I think this is a poor name for a generic function
> > > > > that would be in kernel.h.
> > > > > 
> > > > > Isn't there an include file in linux/drm that's
> > > > > appropriate for this.  Maybe drmP.h
> > > > > 
> > > > > Maybe prefix this function name with drm_ too.
> > > > No, the next patch adds a user to drivers/staging (which will be moved
> > > > to drivers/dma-buf) soon. Maybe move to a different header in
> > > > include/linux/? not sure which one.
> > > > 
> > > > > 
> > > > > 
> > > > > Also, there's this that might conflict:
> > > > > 
> > > > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ptr_to_compat(p)
> > > > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ((unsigned long)(p))
> > > > Right, I'll figure out how to replace these two too.
> > > The powerpc to_user_ptr has a different meaning from the one I'm adding
> > > in this patch. I propose we just rename powerpc's to_user_ptr to
> > > __to_user_ptr and leave the rest as is.
> > I think that's not a good idea, and you should really check
> > this concept with the powerpc folk (added to to:s and cc:ed)
> > 
> > If it were really added, then the function meaning is incorrect.
> > 
> > This is taking a u64, casting that to (unsigned long/uint_ptr_t),
> > then converting that to a user pointer.
> > 
> > Does that naming and use make sense on x86-32 or arm32?
> > 
> fwiw Gustavo's version of to_user_ptr() is in use on arm32 and arm64..
> Not entirely sure what doesn't make sense about it

It's a name that seems like it should be a straightforward
cast of a kernel pointer to a __user pointer like:

static inline void __user *to_user_ptr(void *p)
{
	return (void __user *)p;
}

As a static function in a single file, it's not
great, but OK, fine, it's static.

As a global function in kernel.h, it's misleading.

Rob Clark March 17, 2016, 8:50 p.m. UTC | #7

On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <[e-mail]> wrote:
> On Thu, 2016-03-17 at 16:33 -0400, Rob Clark wrote:
>> On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches <[e-mail]> wrote:
>> > On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
>> > > 2016-03-17 Gustavo Padovan <[e-mail]>:
>> > > > 2016-03-17 Joe Perches <[e-mail]>:
>> > > > > On Thu, 2016-03-17 at 14:30 -0300, Gustavo Padovan wrote:
>> > > > > > This function had copies in 3 different files. Unify them in
>> > > > > > kernel.h.
>> > > > > This is only used by gpu/drm.
>> > > > >
>> > > > > I think this is a poor name for a generic function
>> > > > > that would be in kernel.h.
>> > > > >
>> > > > > Isn't there an include file in linux/drm that's
>> > > > > appropriate for this.  Maybe drmP.h
>> > > > >
>> > > > > Maybe prefix this function name with drm_ too.
>> > > > No, the next patch adds a user to drivers/staging (which will be moved
>> > > > to drivers/dma-buf) soon. Maybe move to a different header in
>> > > > include/linux/? not sure which one.
>> > > >
>> > > > >
>> > > > >
>> > > > > Also, there's this that might conflict:
>> > > > >
>> > > > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ptr_to_compat(p)
>> > > > > arch/powerpc/kernel/signal_32.c:#define to_user_ptr(p)          ((unsigned long)(p))
>> > > > Right, I'll figure out how to replace these two too.
>> > > The powerpc to_user_ptr has a different meaning from the one I'm adding
>> > > in this patch. I propose we just rename powerpc's to_user_ptr to
>> > > __to_user_ptr and leave the rest as is.
>> > I think that's not a good idea, and you should really check
>> > this concept with the powerpc folk (added to to:s and cc:ed)
>> >
>> > If it were really added, then the function meaning is incorrect.
>> >
>> > This is taking a u64, casting that to (unsigned long/uint_ptr_t),
>> > then converting that to a user pointer.
>> >
>> > Does that naming and use make sense on x86-32 or arm32?
>> >
>> fwiw Gustavo's version of to_user_ptr() is in use on arm32 and arm64..
>> Not entirely sure what doesn't make sense about it
>
> It's a name that seems like it should be a straightforward
> cast of a kernel pointer to a __user pointer like:
>
> static inline void __user *to_user_ptr(void *p)
> {
>         return (void __user *)p;
> }

ahh, ok.  I guess I was used to using it in the context of ioctl
structs..  in that context u64 -> (void __user *) made more sense.

Maybe uapi_to_ptr()?  (ok, not super-creative.. maybe someone has a better idea)

BR,
-R

> As a static function in a single file, it's not
> great, but OK, fine, it's static.
>
> As a global function in kernel.h, it's misleading.
>
>

Joe Perches March 17, 2016, 9:10 p.m. UTC | #8

On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <[e-mail]> wrote:
[]
> > It's a name that seems like it should be a straightforward
> > cast of a kernel pointer to a __user pointer like:
> > 
> > static inline void __user *to_user_ptr(void *p)
> > {
> >         return (void __user *)p;
> > }
> ahh, ok.  I guess I was used to using it in the context of ioctl
> structs..  in that context u64 -> (void __user *) made more sense.
> 
> Maybe uapi_to_ptr()?  (ok, not super-creative.. maybe someone has a
> better idea)

Maybe u64_to_user_ptr?

Gustavo Padovan March 17, 2016, 9:19 p.m. UTC | #9

2016-03-17 Joe Perches <[e-mail]>:

> On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <[e-mail]> wrote:
> []
> > > It's a name that seems like it should be a straightforward
> > > cast of a kernel pointer to a __user pointer like:
> > > 
> > > static inline void __user *to_user_ptr(void *p)
> > > {
> > >         return (void __user *)p;
> > > }
> > ahh, ok.  I guess I was used to using it in the context of ioctl
> > structs..  in that context u64 -> (void __user *) made more sense.
> > 
> > Maybe uapi_to_ptr()?  (ok, not super-creative.. maybe someone has a
> > better idea)
> 
> Maybe u64_to_user_ptr?

That is a good name. If everyone agrees I can resend this patch
changing it to u64_to_user_ptr. Then should we still keep it on
kernel.h?

	Gustavo

Rob Clark March 17, 2016, 9:25 p.m. UTC | #10

On Thu, Mar 17, 2016 at 5:19 PM, Gustavo Padovan <[e-mail]> wrote:
> 2016-03-17 Joe Perches <[e-mail]>:
>
>> On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
>> > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <[e-mail]> wrote:
>> []
>> > > It's a name that seems like it should be a straightforward
>> > > cast of a kernel pointer to a __user pointer like:
>> > >
>> > > static inline void __user *to_user_ptr(void *p)
>> > > {
>> > >         return (void __user *)p;
>> > > }
>> > ahh, ok.  I guess I was used to using it in the context of ioctl
>> > structs..  in that context u64 -> (void __user *) made more sense.
>> >
>> > Maybe uapi_to_ptr()?  (ok, not super-creative.. maybe someone has a
>> > better idea)
>>
>> Maybe u64_to_user_ptr?
>
> That is a good name. If everyone agrees I can resend this patch
> changing it to u64_to_user_ptr. Then should we still keep it on
> kernel.h?


works for me

BR,
-R

Joe Perches March 17, 2016, 9:33 p.m. UTC | #11

On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
> 2016-03-17 Joe Perches <[e-mail]>:
> > On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> > > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <[e-mail]> wrote:
> > []
> > > > It's a name that seems like it should be a straightforward
> > > > cast of a kernel pointer to a __user pointer like:
> > > > 
> > > > static inline void __user *to_user_ptr(void *p)
> > > > {
> > > >         return (void __user *)p;
> > > > }
> > > ahh, ok.  I guess I was used to using it in the context of ioctl
> > > structs..  in that context u64 -> (void __user *) made more sense.
> > > 
> > > Maybe uapi_to_ptr()?  (ok, not super-creative.. maybe someone has a
> > > better idea)
> > Maybe u64_to_user_ptr?
> That is a good name. If everyone agrees I can resend this patch
> changing it to u64_to_user_ptr. Then should we still keep it on
> kernel.h?

I've no particular opinion about location,
but maybe compat.h might be appropriate.

Maybe add all variants:

	void __user *u32_to_user_ptr(u32 val)
	void __user *u64_to_user_ptr(u64 val)
	u32 user_ptr_to_u32(void __user *p)
	u64 user_ptr_to_u64(void __user *p)

Maybe there's something about 32 bit userspace on
64 OS that should be done too.

Gustavo Padovan March 17, 2016, 10:16 p.m. UTC | #12

2016-03-17 Joe Perches <[e-mail]>:

> On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
> > 2016-03-17 Joe Perches <[e-mail]>:
> > > On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> > > > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <[e-mail]> wrote:
> > > []
> > > > > It's a name that seems like it should be a straightforward
> > > > > cast of a kernel pointer to a __user pointer like:
> > > > > 
> > > > > static inline void __user *to_user_ptr(void *p)
> > > > > {
> > > > >         return (void __user *)p;
> > > > > }
> > > > ahh, ok.  I guess I was used to using it in the context of ioctl
> > > > structs..  in that context u64 -> (void __user *) made more sense.
> > > > 
> > > > Maybe uapi_to_ptr()?  (ok, not super-creative.. maybe someone has a
> > > > better idea)
> > > Maybe u64_to_user_ptr?
> > That is a good name. If everyone agrees I can resend this patch
> > changing it to u64_to_user_ptr. Then should we still keep it on
> > kernel.h?
> 
> I've no particular opinion about location,
> but maybe compat.h might be appropriate.

I don't think this is really related to compat. I'd keep kernel.h.

The problem I'm trying to solve here is:

CC      drivers/dma-buf/sync_file.o
drivers/dma-buf/sync_file.c: In function ‘sync_file_ioctl_fence_info’:
drivers/dma-buf/sync_file.c:341:19: warning: cast to pointer from
integer of different size [-Wint-to-pointer-cast]
  if (copy_to_user((void __user *)info.sync_fence_info, fence_info,

where info.sync_fence_info is __u64.

	Gustavo

Daniel Vetter March 18, 2016, 8:23 a.m. UTC | #13

On Thu, Mar 17, 2016 at 02:33:50PM -0700, Joe Perches wrote:
> On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
> > 2016-03-17 Joe Perches <[e-mail]>:
> > > On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> > > > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <[e-mail]> wrote:
> > > []
> > > > > It's a name that seems like it should be a straightforward
> > > > > cast of a kernel pointer to a __user pointer like:
> > > > > 
> > > > > static inline void __user *to_user_ptr(void *p)
> > > > > {
> > > > >         return (void __user *)p;
> > > > > }
> > > > ahh, ok.  I guess I was used to using it in the context of ioctl
> > > > structs..  in that context u64 -> (void __user *) made more sense.
> > > > 
> > > > Maybe uapi_to_ptr()?  (ok, not super-creative.. maybe someone has a
> > > > better idea)
> > > Maybe u64_to_user_ptr?
> > That is a good name. If everyone agrees I can resend this patch
> > changing it to u64_to_user_ptr. Then should we still keep it on
> > kernel.h?
> 
> I've no particular opinion about location,
> but maybe compat.h might be appropriate.
> 
> Maybe add all variants:
> 
> 	void __user *u32_to_user_ptr(u32 val)
> 	void __user *u64_to_user_ptr(u64 val)
> 	u32 user_ptr_to_u32(void __user *p)
> 	u64 user_ptr_to_u64(void __user *p)
> 
> Maybe there's something about 32 bit userspace on
> 64 OS that should be done too.

Tbh I really don't think we should add 32bit variants and encourage the
mispractice of having 32bit user ptrs in ioctl structs and stuff. Anyway,
just my bikeshed on top ;-)
-Daniel

Patch

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 1aba01a..b1fafb6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -28,11 +28,6 @@ 
 #define BO_LOCKED   0x4000
 #define BO_PINNED   0x2000
 
-static inline void __user *to_user_ptr(u64 address)
-{
-	return (void __user *)(uintptr_t)address;
-}
-
 static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
 		struct etnaviv_gpu *gpu, size_t nr)
 {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b0847b9..c446895 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3564,11 +3564,6 @@  static inline i915_reg_t i915_vgacntrl_reg(struct drm_device *dev)
 		return VGACNTRL;
 }
 
-static inline void __user *to_user_ptr(u64 address)
-{
-	return (void __user *)(uintptr_t)address;
-}
-
 static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
 {
 	unsigned long j = msecs_to_jiffies(m);
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 6d7cd3f..e9c8b96 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -28,11 +28,6 @@ 
 #define BO_LOCKED   0x4000
 #define BO_PINNED   0x2000
 
-static inline void __user *to_user_ptr(u64 address)
-{
-	return (void __user *)(uintptr_t)address;
-}
-
 static struct msm_gem_submit *submit_create(struct drm_device *dev,
 		struct msm_gpu *gpu, int nr)
 {
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f31638c..c0a6001 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -53,6 +53,11 @@ 
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
+static inline void __user *to_user_ptr(u64 address)
+{
+	return (void __user *)(uintptr_t)address;
+}
+
 /*
  * This looks more complex than it should be. But we need to
  * get the type for the ~ right in round_down (it needs to be