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

#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]

#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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web