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

Groups > linux.kernel > #1495240 > unrolled thread

[PATCH] x86: suppress sparse warning in copy_to_user()

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86: suppress sparse warning in copy_to_user() Johannes Berg <johannes@sipsolutions.net> - 2016-10-04 09:40 +0200
    Re: [PATCH] x86: suppress sparse warning in copy_to_user() "Jan Beulich" <JBeulich@suse.com> - 2016-10-04 10:00 +0200
      Re: [PATCH] x86: suppress sparse warning in copy_to_user() Johannes Berg <johannes@sipsolutions.net> - 2016-10-04 10:10 +0200
        Re: [PATCH] x86: suppress sparse warning in copy_to_user() "Jan Beulich" <JBeulich@suse.com> - 2016-10-04 10:40 +0200
          Re: [PATCH] x86: suppress sparse warning in copy_to_user() Johannes Berg <johannes@sipsolutions.net> - 2016-10-04 10:50 +0200
            Re: [PATCH] x86: suppress sparse warning in copy_to_user() "Jan Beulich" <JBeulich@suse.com> - 2016-10-04 11:10 +0200

#1495240 — [PATCH] x86: suppress sparse warning in copy_to_user()

From: Johannes Berg <johannes.berg@intel.com>

__compiletime_object_size() is simply defined to __builtin_object_size()
which gcc declares with (void *, int type) prototype. This is also done
by sparse, since it follows gcc, which leads it to warn, many times, on
any usage of copy_to_user(), about it:

  arch/x86/include/asm/uaccess.h:735:18: warning: incorrect type in argument 1 (different modifiers)
  arch/x86/include/asm/uaccess.h:735:18:    expected void *<noident>
  arch/x86/include/asm/uaccess.h:735:18:    got void const *from

Suppress this by adding a (void *) cast. The compiler probably should
have declared it as const, but as it doesn't this is necessary. If it
changes, then the cast also won't matter.

Fixes: 7a3d9b0f3abbe ("x86: Unify copy_to_user() and add size checking to it")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/x86/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 2131c4ce7d8a..0bd4a5a86199 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -732,7 +732,7 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
 static __always_inline unsigned long __must_check
 copy_to_user(void __user *to, const void *from, unsigned long n)
 {
-	int sz = __compiletime_object_size(from);
+	int sz = __compiletime_object_size((void *)from);
 
 	kasan_check_read(from, n);
 
-- 
2.8.1

[toc] | [next] | [standalone]

#1495250

>>> On 04.10.16 at 09:33, <johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> __compiletime_object_size() is simply defined to __builtin_object_size()
> which gcc declares with (void *, int type) prototype.

If that was the case, everyone should have seen such warnings from
the day the original patch got introduced. And the compiler warnings
I get when testing with all four combinations of const and volatile also
supports this by saying "expected 'const void *' but ..." (arguably the
compiler should accept volatile here too). To be honest, for code in
other trees where I'm maintainer, I'd reject such casting away of
constness, and demand the utility to get fixed instead.

Jan

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

#1495251

On Tue, 2016-10-04 at 01:51 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 04.10.16 at 09:33, <johannes@sipsolutions.net> wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > __compiletime_object_size() is simply defined to
> > __builtin_object_size()
> > which gcc declares with (void *, int type) prototype.
> 
> If that was the case, everyone should have seen such warnings from
> the day the original patch got introduced. 

Only if they run sparse. Clearly people don't, or we wouldn't have a
history of a ton of such problems, e.g.

112dc0c8069e ("locking/barriers: Suppress sparse warnings in lockless_dereference()")
c15c0ab12fd6 ("ipv6: suppress sparse warnings in IP6_ECN_set_ce()")
1ea049b2de5d ("bvec: avoid variable shadowing warning")

(just to give a few of the examples I fixed recently). These are of
course double-plus annoying in header files, since they show up in
completely unrelated code when the header file is including, making the
tools effectively useless.

> And the compiler warnings
> I get when testing with all four combinations of const and volatile
> also supports this by saying "expected 'const void *' but ..." 

It's not a compiler warning though that I'm getting.

What tool are you using to get such a warning?

On gcc 6.1.1, I'm getting no warning (from the compiler) either way,
even with W=2, and the gcc documentation notes the fact that it treats
it as passing void *:

https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html

> (arguably the compiler should accept volatile here too). To be
> honest, for code in other trees where I'm maintainer, I'd reject such
> casting away of constness, and demand the utility to get fixed
> instead.

That could be done, but arguably "the tool" (I suppose you also never
run sparse) is actually behaving correctly here - the "function" *is*
defined to pass void *, so it's a correct warning.

Regardless though, it's fairly pointless to worry about it here since
it's a builtin that's evaluated at compile time, so nothing can really
happen.

johannes

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

#1495291

>>> On 04.10.16 at 10:02, <johannes@sipsolutions.net> wrote:
> On Tue, 2016-10-04 at 01:51 -0600, Jan Beulich wrote:
>> > 
>> > > 
>> > > > 
>> > > > On 04.10.16 at 09:33, <johannes@sipsolutions.net> wrote:
>> > From: Johannes Berg <johannes.berg@intel.com>
>> > 
>> > __compiletime_object_size() is simply defined to
>> > __builtin_object_size()
>> > which gcc declares with (void *, int type) prototype.
>> 
>> If that was the case, everyone should have seen such warnings from
>> the day the original patch got introduced. 
> 
> Only if they run sparse. Clearly people don't, or we wouldn't have a
> history of a ton of such problems, e.g.

No - you say "which gcc declares with (void *, int type) prototype".
If that was the case, there would need to be a warning.

>> And the compiler warnings
>> I get when testing with all four combinations of const and volatile
>> also supports this by saying "expected 'const void *' but ..." 
> 
> It's not a compiler warning though that I'm getting.
> 
> What tool are you using to get such a warning?

I'm talking about gcc and the warning surfacing when I additonally
add volatile.

> On gcc 6.1.1, I'm getting no warning (from the compiler) either way,
> even with W=2, and the gcc documentation notes the fact that it treats
> it as passing void *:
> 
> https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html 

Perhaps it's just the documentation which is imprecise here?

Jan

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

#1495297

> > > If that was the case, everyone should have seen such warnings
> > > from the day the original patch got introduced. 
> > 
> > Only if they run sparse. Clearly people don't, or we wouldn't have
> > a history of a ton of such problems, e.g.
> 
> No - you say "which gcc declares with (void *, int type) prototype".
> If that was the case, there would need to be a warning.

There would need to be a warning when?

> > > And the compiler warnings
> > > I get when testing with all four combinations of const and
> > > volatile
> > > also supports this by saying "expected 'const void *' but ..." 
> > 
> > It's not a compiler warning though that I'm getting.
> > 
> > What tool are you using to get such a warning?
> 
> I'm talking about gcc and the warning surfacing when I additonally
> add volatile.

Oh, sorry. If you get the warning, it prints "expected 'const void *'"
... yeah, I see.

> > https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html 
> 
> Perhaps it's just the documentation which is imprecise here?

Hmm, yeah, that could be right, or maybe it changed at some point?

If it were defined the way the documentation says, you should have
gotten a compiler warning ("passing argument 1 ... discards ‘const’
qualifier from pointer target type") with the code as it is (without my
patch), since you can't pass a const pointer to a function that expects
a non-const pointer. Clearly that didn't happen.

That does indicate that the prototype is indeed with the const, I guess
I'll go fix sparse instead.

Sorry I misread your earlier explanation entirely!

johannes

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

#1495307

>>> On 04.10.16 at 10:49, <johannes@sipsolutions.net> wrote:
>> > > If that was the case, everyone should have seen such warnings
>> > > from the day the original patch got introduced. 
>> > 
>> > Only if they run sparse. Clearly people don't, or we wouldn't have
>> > a history of a ton of such problems, e.g.
>> 
>> No - you say "which gcc declares with (void *, int type) prototype".
>> If that was the case, there would need to be a warning.
> 
> There would need to be a warning when?

If the declaration used "void *" instead of "const void *".

Jan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web