Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1669452 > unrolled thread

__user with scalar data types

Back to article view | Back to linux.kernel

Contents

  __user with scalar data types Jordan Crouse <jcrouse@codeaurora.org> - 2017-06-19 18:20 +0200
    Re: __user with scalar data types Al Viro <viro@ZenIV.linux.org.uk> - 2017-06-19 18:40 +0200
      Re: __user with scalar data types Daniel Vetter <daniel@ffwll.ch> - 2017-06-20 09:20 +0200
        Re: __user with scalar data types Gerd Hoffmann <kraxel@redhat.com> - 2017-06-20 09:50 +0200
          Re: __user with scalar data types Daniel Vetter <daniel@ffwll.ch> - 2017-06-20 10:40 +0200
    Re: __user with scalar data types Luc Van Oostenryck <luc.vanoostenryck@gmail.com> - 2017-06-19 21:30 +0200
    Re: __user with scalar data types Luc Van Oostenryck <luc.vanoostenryck@gmail.com> - 2017-06-19 22:40 +0200
      Re: __user with scalar data types Al Viro <viro@ZenIV.linux.org.uk> - 2017-06-19 22:50 +0200
        Re: __user with scalar data types Luc Van Oostenryck <luc.vanoostenryck@gmail.com> - 2017-06-20 00:40 +0200

#1669452 — __user with scalar data types

A number of us over in DRM land have been using __u64 scalar types
to store pointers for uapi structures in accordance with Daniel Vetter's
now classic treatise on ioctls:

http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

A smaller number of us have further been marking the __u64 with __user,
to wit:

struct uapistruct {
	...
	__u64 __user myptr;
	---
};

And then converting it for use in the kernel as such:

{
	void __user *userptr = (void __user *)(uintptr_t)args->myptr;

	copy_from_user(local, userptr, size);
	...
}

The problem is that sparse doesn't like the momentary switch to
uintptr_t:

	warning: dereference of noderef expression

Which raised a bikeshed debate over whether it is appropriate to mark a scalar
type as __user.  My opinion is that it is appropriate because __user should mark
user memory regardless of the container.

I'm looking for opinions or semi-authoritative edicts to determine if we should
either start changing our uapi headers or go off and try to figure out how to
make sparse understand this particular usage.

Thanks!
Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1669472

On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:

> Which raised a bikeshed debate over whether it is appropriate to mark a scalar
> type as __user.  My opinion is that it is appropriate because __user should mark
> user memory regardless of the container.

What the hell?  __user is a qualifier like const, volatile, etc.  It's a property
of *pointer* *type*.  Not some nebulous "marks userland memory" thing.

> I'm looking for opinions or semi-authoritative edicts to determine if we should
> either start changing our uapi headers or go off and try to figure out how to
> make sparse understand this particular usage.

Stop cargo-culting, please.

[toc] | [prev] | [next] | [standalone]

#1670363

On Mon, Jun 19, 2017 at 6:34 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
>
>> Which raised a bikeshed debate over whether it is appropriate to mark a scalar
>> type as __user.  My opinion is that it is appropriate because __user should mark
>> user memory regardless of the container.
>
> What the hell?  __user is a qualifier like const, volatile, etc.  It's a property
> of *pointer* *type*.  Not some nebulous "marks userland memory" thing.
>
>> I'm looking for opinions or semi-authoritative edicts to determine if we should
>> either start changing our uapi headers or go off and try to figure out how to
>> make sparse understand this particular usage.
>
> Stop cargo-culting, please.

Yep that's cargo-culted, but from a quick grep only msm and qxl
headers do this (the other __user annotations in uapi/drm are for
pointers, where it's correct). Adding those maintainers.

Also, if you use u64_to_user_ptr helper macro sparse should have
caught this (if not we'd need to improve the macro).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1670382

[Multipart message — attachments visible in raw view] — view raw

  Hi,

> Yep that's cargo-culted, but from a quick grep only msm and qxl
> headers do this (the other __user annotations in uapi/drm are for
> pointers, where it's correct). Adding those maintainers.

Yep, those looks pointless indeed.

> Also, if you use u64_to_user_ptr helper macro sparse should have
> caught this (if not we'd need to improve the macro).

And qxl should actually use it.

Fix attached (compile-tested only so far), does that look ok?

cheers,
  Gerd

[toc] | [prev] | [next] | [standalone]

#1670430

On Tue, Jun 20, 2017 at 9:42 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> Yep that's cargo-culted, but from a quick grep only msm and qxl
>> headers do this (the other __user annotations in uapi/drm are for
>> pointers, where it's correct). Adding those maintainers.
>
> Yep, those looks pointless indeed.
>
>> Also, if you use u64_to_user_ptr helper macro sparse should have
>> caught this (if not we'd need to improve the macro).
>
> And qxl should actually use it.
>
> Fix attached (compile-tested only so far), does that look ok?

Yup. Assuming sparse is happy: Acked-by: me.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1669831

On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> A number of us over in DRM land have been using __u64 scalar types
> to store pointers for uapi structures in accordance with Daniel Vetter's
> now classic treatise on ioctls:
> 
> http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
> 
> A smaller number of us have further been marking the __u64 with __user,
> to wit:
> 
> struct uapistruct {
> 	...
> 	__u64 __user myptr;
> 	---
> };

It wouldn't make sense to have this:
	struct uapistruct {
		__u64 __user myptr;
		__u64        anothermember;
	};
In other words, eiter all members are in the user address space
or none are. So, a struct member should not be marked __user
(exactly as for 'const' or 'volatile').

It wouldn't also make sense to move the __user to the whole struct,
giving something like:
	struct uapistruct {
		__u64 myptr;
		__u64 anothermember;
	} __user;
because it's not the type that belong to the user address space
but some specific objects.

Of course, your real problem here is that you're using a __u64 to
store a pointer and then expect this __u64 to have some properties
unique to pointers.


-- Luc Van Oostenryck (sparse hacker)

[toc] | [prev] | [next] | [standalone]

#1669928

On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> struct uapistruct {
> 	...
> 	__u64 __user myptr;
> 	---
> };
> 
> And then converting it for use in the kernel as such:
> 
> {
> 	void __user *userptr = (void __user *)(uintptr_t)args->myptr;
> 
> 	copy_from_user(local, userptr, size);
> 	...
> }
> 
> The problem is that sparse doesn't like the momentary switch to
> uintptr_t:
> 
> 	warning: dereference of noderef expression

This warning doesn't come from the cast to uintptr_t but
simply from dereferencing the field which can't be dereferenced
since it's marked as '__user'. In other words, doing
'args->myptr' rightfully trigger the warning and no cast
will or should stop that.

Also, you can't expect the '__user' to be transmitted from
'myptr' to the pointer (without taking the address of 'myptr').
It's exactly like 'const int' vs. 'const int *': the '__user' or
the 'const' is not at the same level in the type hierarchy
('const object' vs. 'non-const pointer to const object').


-- Luc Van Oostenryck

[toc] | [prev] | [next] | [standalone]

#1669940

On Mon, Jun 19, 2017 at 10:32:18PM +0200, Luc Van Oostenryck wrote:
> On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> > struct uapistruct {
> > 	...
> > 	__u64 __user myptr;
> > 	---
> > };
> > 
> > And then converting it for use in the kernel as such:
> > 
> > {
> > 	void __user *userptr = (void __user *)(uintptr_t)args->myptr;
> > 
> > 	copy_from_user(local, userptr, size);
> > 	...
> > }
> > 
> > The problem is that sparse doesn't like the momentary switch to
> > uintptr_t:
> > 
> > 	warning: dereference of noderef expression
> 
> This warning doesn't come from the cast to uintptr_t but
> simply from dereferencing the field which can't be dereferenced
> since it's marked as '__user'. In other words, doing
> 'args->myptr' rightfully trigger the warning and no cast
> will or should stop that.
> 
> Also, you can't expect the '__user' to be transmitted from
> 'myptr' to the pointer (without taking the address of 'myptr').
> It's exactly like 'const int' vs. 'const int *': the '__user' or
> the 'const' is not at the same level in the type hierarchy
> ('const object' vs. 'non-const pointer to const object').

Besides, suppose you add a special type for that.  How would it
have to behave, really?  AFAICS, you want something similar to
__bitwise, except that (assuming this type is T)
	T + integer => T
	T - integer => T
	T & integer => integer
	T | integer => T
	T - T => integer (quietly decay to underlying type for both
arguments, then treat as normal -)
	T & T => T (probably, but might be worth a warning)
	T | T => T (ditto)
	comparison - same as for __bitwise
	constant conversion: 0 should convert clean, anything else - a warning
	cast to pointer => warn unless the target type is __user?  But that's
not going to help with cast through uintptr_t...
	?: as usual
	any other arithmetics => warn and decay to underlying integer type

It might be not impossible to implement, but it sure as hell won't be __user
and it'll need careful thinking about the semantics of those annotations.
The outline above is just that - figuring out if there are any nasty corner
cases will take some work.

[toc] | [prev] | [next] | [standalone]

#1670010

On Mon, Jun 19, 2017 at 09:46:37PM +0100, Al Viro wrote:
> On Mon, Jun 19, 2017 at 10:32:18PM +0200, Luc Van Oostenryck wrote:
> > On Mon, Jun 19, 2017 at 10:15:09AM -0600, Jordan Crouse wrote:
> > > struct uapistruct {
> > > 	...
> > > 	__u64 __user myptr;
> > > 	---
> > > };
> > > 
> > > And then converting it for use in the kernel as such:
> > > 
> > > {
> > > 	void __user *userptr = (void __user *)(uintptr_t)args->myptr;
> > > 
> > > 	copy_from_user(local, userptr, size);
> > > 	...
> > > }
> > > 
> > > The problem is that sparse doesn't like the momentary switch to
> > > uintptr_t:
> > > 
> > > 	warning: dereference of noderef expression
> > 
> > This warning doesn't come from the cast to uintptr_t but
> > simply from dereferencing the field which can't be dereferenced
> > since it's marked as '__user'. In other words, doing
> > 'args->myptr' rightfully trigger the warning and no cast
> > will or should stop that.
> > 
> > Also, you can't expect the '__user' to be transmitted from
> > 'myptr' to the pointer (without taking the address of 'myptr').
> > It's exactly like 'const int' vs. 'const int *': the '__user' or
> > the 'const' is not at the same level in the type hierarchy
> > ('const object' vs. 'non-const pointer to const object').
> 
> Besides, suppose you add a special type for that.  How would it
> have to behave, really?  AFAICS, you want something similar to
> __bitwise, except that (assuming this type is T)
> 	T + integer => T
> 	T - integer => T
> 	T & integer => integer
> 	T | integer => T
> 	T - T => integer (quietly decay to underlying type for both
> arguments, then treat as normal -)
> 	T & T => T (probably, but might be worth a warning)
> 	T | T => T (ditto)
> 	comparison - same as for __bitwise
> 	constant conversion: 0 should convert clean, anything else - a warning
> 	cast to pointer => warn unless the target type is __user?  But that's
> not going to help with cast through uintptr_t...
> 	?: as usual
> 	any other arithmetics => warn and decay to underlying integer type

And how it should behave with typeof()?
Because it's already unclear to me what should be the result of:
	typeof(X {__user,__noderef,__nocast,__bitwise} [*])
and I don't think sparse do the right thing with this.

That said, I'm of the opinion that simply thinking about implementing this
special type is close to a capital sin.

-- Luc

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web