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

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

[Joe Perches]

On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
> 2016-03-17 Gustavo Padovan <gustavo.padovan@collabora.co.uk>:
> > 2016-03-17 Joe Perches <joe@perches.com>:
> > > 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?

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

[Rob Clark]

On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
>> 2016-03-17 Gustavo Padovan <gustavo.padovan@collabora.co.uk>:
>> > 2016-03-17 Joe Perches <joe@perches.com>:
>> > > 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

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

[Joe Perches]

On Thu, 2016-03-17 at 16:33 -0400, Rob Clark wrote:
> On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches <joe@perches.com> wrote:
> > On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
> > > 2016-03-17 Gustavo Padovan <gustavo.padovan@collabora.co.uk>:
> > > > 2016-03-17 Joe Perches <joe@perches.com>:
> > > > > 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.

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

[Rob Clark]

On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2016-03-17 at 16:33 -0400, Rob Clark wrote:
>> On Thu, Mar 17, 2016 at 4:22 PM, Joe Perches <joe@perches.com> wrote:
>> > On Thu, 2016-03-17 at 15:43 -0300, Gustavo Padovan wrote:
>> > > 2016-03-17 Gustavo Padovan <gustavo.padovan@collabora.co.uk>:
>> > > > 2016-03-17 Joe Perches <joe@perches.com>:
>> > > > > 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.
>
>

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

[Joe Perches]

On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <joe@perches.com> 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?

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

[Gustavo Padovan]

2016-03-17 Joe Perches <joe@perches.com>:

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

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

[Rob Clark]

On Thu, Mar 17, 2016 at 5:19 PM, Gustavo Padovan <gustavo@padovan.org> wrote:
> 2016-03-17 Joe Perches <joe@perches.com>:
>
>> On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
>> > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <joe@perches.com> 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

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

[Joe Perches]

On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
> 2016-03-17 Joe Perches <joe@perches.com>:
> > On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> > > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <joe@perches.com> 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.

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

[Gustavo Padovan]

2016-03-17 Joe Perches <joe@perches.com>:

> On Thu, 2016-03-17 at 18:19 -0300, Gustavo Padovan wrote:
> > 2016-03-17 Joe Perches <joe@perches.com>:
> > > On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> > > > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <joe@perches.com> wrot=
e:
> > > []
> > > > > It's a name that seems like it should be a straightforward
> > > > > cast of a kernel pointer to a __user pointer like:
> > > > >=20
> > > > > static inline void __user *to_user_ptr(void *p)
> > > > > {
> > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return (void __us=
er *)p;
> > > > > }
> > > > ahh, ok.=C2=A0=C2=A0I guess I was used to using it in the context o=
f ioctl
> > > > structs..=C2=A0=C2=A0in that context u64 -> (void __user *) made mo=
re sense.
> > > >=20
> > > > Maybe uapi_to_ptr()?=C2=A0=C2=A0(ok, not super-creative.. maybe som=
eone 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?
>=20
> 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 =E2=80=98sync_file_ioctl_fence_inf=
o=E2=80=99:
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

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

[Daniel Vetter]

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 <joe@perches.com>:
> > > On Thu, 2016-03-17 at 16:50 -0400, Rob Clark wrote:
> > > > On Thu, Mar 17, 2016 at 4:40 PM, Joe Perches <joe@perches.com> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch