[PATCH] x86: Fixes warning: cast removes address space '__user' of expression in uaccess_64.h

[PATCH] x86: Fixes warning: cast removes address space '__user' of expression in uaccess_64.h

[Dipendra Khadka]

Sparse has identified a warning as follows:

./arch/x86/include/asm/uaccess_64.h:88:24: warning: cast removes address space '__user' of expression.

Since the valid_user_address(x) macro implicitly casts the argument
to long and compares the converted value of x to zero, casting ptr
to unsigned long has no functional impact and does not trigger a 
Sparse warning either.

Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com>
---
 arch/x86/include/asm/uaccess_64.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index f2c02e4469cc..da24d807e101 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -85,7 +85,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
 static inline bool __access_ok(const void __user *ptr, unsigned long size)
 {
 	if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) {
-		return valid_user_address(ptr);
+		return valid_user_address((unsigned long)ptr);
 	} else {
 		unsigned long sum = size + (unsigned long)ptr;
 		return valid_user_address(sum) && sum >= (unsigned long)ptr;
-- 
2.34.1

Re: [PATCH] x86: Fixes warning: cast removes address space '__user' of expression in uaccess_64.h

[Dave Hansen]

On 11/16/23 09:38, Dipendra Khadka wrote:
> Sparse has identified a warning as follows:
> 
> ./arch/x86/include/asm/uaccess_64.h:88:24: warning: cast removes address space '__user' of expression.
> 
> Since the valid_user_address(x) macro implicitly casts the argument
> to long and compares the converted value of x to zero, casting ptr
> to unsigned long has no functional impact and does not trigger a 
> Sparse warning either.

Why does sparse complain about a cast to 'long' but not 'unsigned long'?
 Both remove the '__user' address space from the expression.  Were there
just so many __user pointers being cast to 'unsigned long' that there's
an exception in sparse for 'void __user *' => 'unsigned long'?

Either way, if we're going to fix it it seems like it would be better to
valid_user_address() actually handle, well, __user addresses rather than
expecting callers to do it.

Re: [PATCH] x86: Fixes warning: cast removes address space '__user' of expression in uaccess_64.h

[Dipendra Khadka]

Hi,

I am not sure why spare did not find the difference between 'long' and
'unsigned long'
in this particular case. I saw that in else case,there is use of
unsigned long and sparse
does not report a warning .Hence I thought casting to unsigned long
will solve the problem.
Also, there are not any other warnings thrown by spare in the uaccess_64.h file.

I think casting x  to  'void __user *'before checking whether it is
greater than or equal to zero
in valid_user_address() will be more sensible and fix the warning either.

Is this ok for you? Or have to cast to 'unsigned long' or other
changes or no need to do anything?



On Sat, 18 Nov 2023 at 00:00, Dipendra Khadka <kdipendra88@gmail.com> wrote:
>
> Hi,
>
> I am not sure why spare did not find the difference between 'long' and 'unsigned long'
> in this particular case. I saw that in else case,there is use of unsigned long and sparse
> does not report a warning .Hence I thought casting to unsigned long will solve the problem.
> Also, there are not any other warnings thrown by spare in the uaccess_64.h file.
>
> I think casting x  to  'void __user *'before checking whether it is greater than or equal to zero
> in valid_user_address() will be more sensible and fix the warning either.
>
> Is this ok for you? Or have to cast to 'unsigned long' or no need to do anything?
>
>
> On Fri, 17 Nov 2023 at 21:04, Dave Hansen <dave.hansen@intel.com> wrote:
>>
>> On 11/16/23 09:38, Dipendra Khadka wrote:
>> > Sparse has identified a warning as follows:
>> >
>> > ./arch/x86/include/asm/uaccess_64.h:88:24: warning: cast removes address space '__user' of expression.
>> >
>> > Since the valid_user_address(x) macro implicitly casts the argument
>> > to long and compares the converted value of x to zero, casting ptr
>> > to unsigned long has no functional impact and does not trigger a
>> > Sparse warning either.
>>
>> Why does sparse complain about a cast to 'long' but not 'unsigned long'?
>>  Both remove the '__user' address space from the expression.  Were there
>> just so many __user pointers being cast to 'unsigned long' that there's
>> an exception in sparse for 'void __user *' => 'unsigned long'?
>>
>> Either way, if we're going to fix it it seems like it would be better to
>> valid_user_address() actually handle, well, __user addresses rather than
>> expecting callers to do it.

Re: [PATCH] x86: Fixes warning: cast removes address space '__user' of expression in uaccess_64.h

[Dipendra Khadka]

I am sorry for the formatting of the text as it was due to changing to text
from html and also I wrote "why spare did not find the difference between
'long' and 'unsigned long' in this particular case." instead of "why Sparse
found the difference between 'long' and 'unsigned long' in this
particular case."

Thank you for your consideration.

On Sat, 18 Nov 2023 at 01:16, Dipendra Khadka <kdipendra88@gmail.com> wrote:
>
> Hi,
>
> I am not sure why spare did not find the difference between 'long' and
> 'unsigned long'
> in this particular case. I saw that in else case,there is use of
> unsigned long and sparse
> does not report a warning .Hence I thought casting to unsigned long
> will solve the problem.
> Also, there are not any other warnings thrown by spare in the uaccess_64.h file.
>
> I think casting x  to  'void __user *'before checking whether it is
> greater than or equal to zero
> in valid_user_address() will be more sensible and fix the warning either.
>
> Is this ok for you? Or have to cast to 'unsigned long' or other
> changes or no need to do anything?
>
>
>
> On Sat, 18 Nov 2023 at 00:00, Dipendra Khadka <kdipendra88@gmail.com> wrote:
> >
> > Hi,
> >
> > I am not sure why spare did not find the difference between 'long' and 'unsigned long'
> > in this particular case. I saw that in else case,there is use of unsigned long and sparse
> > does not report a warning .Hence I thought casting to unsigned long will solve the problem.
> > Also, there are not any other warnings thrown by spare in the uaccess_64.h file.
> >
> > I think casting x  to  'void __user *'before checking whether it is greater than or equal to zero
> > in valid_user_address() will be more sensible and fix the warning either.
> >
> > Is this ok for you? Or have to cast to 'unsigned long' or no need to do anything?
> >
> >
> > On Fri, 17 Nov 2023 at 21:04, Dave Hansen <dave.hansen@intel.com> wrote:
> >>
> >> On 11/16/23 09:38, Dipendra Khadka wrote:
> >> > Sparse has identified a warning as follows:
> >> >
> >> > ./arch/x86/include/asm/uaccess_64.h:88:24: warning: cast removes address space '__user' of expression.
> >> >
> >> > Since the valid_user_address(x) macro implicitly casts the argument
> >> > to long and compares the converted value of x to zero, casting ptr
> >> > to unsigned long has no functional impact and does not trigger a
> >> > Sparse warning either.
> >>
> >> Why does sparse complain about a cast to 'long' but not 'unsigned long'?
> >>  Both remove the '__user' address space from the expression.  Were there
> >> just so many __user pointers being cast to 'unsigned long' that there's
> >> an exception in sparse for 'void __user *' => 'unsigned long'?
> >>
> >> Either way, if we're going to fix it it seems like it would be better to
> >> valid_user_address() actually handle, well, __user addresses rather than
> >> expecting callers to do it.

Re: [PATCH] x86: Fixes warning: cast removes address space '__user' of expression in uaccess_64.h

[Dipendra Khadka]

Hi,

On Sat, 18 Nov 2023 at 01:32, Dipendra Khadka <kdipendra88@gmail.com> wrote:
>
> I am sorry for the formatting of the text as it was due to changing to text
> from html and also I wrote "why spare did not find the difference between
> 'long' and 'unsigned long' in this particular case." instead of "why Sparse
> found the difference between 'long' and 'unsigned long' in this
> particular case."
>
> Thank you for your consideration.
>
> On Sat, 18 Nov 2023 at 01:16, Dipendra Khadka <kdipendra88@gmail.com> wrote:
> >
> > Hi,
> >
> > I am not sure why spare did not find the difference between 'long' and
> > 'unsigned long'
> > in this particular case. I saw that in else case,there is use of
> > unsigned long and sparse
> > does not report a warning .Hence I thought casting to unsigned long
> > will solve the problem.
> > Also, there are not any other warnings thrown by spare in the uaccess_64.h file.
> >
> > I think casting x  to  'void __user *'before checking whether it is
> > greater than or equal to zero
> > in valid_user_address() will be more sensible and fix the warning either.
> >
> > Is this ok for you? Or have to cast to 'unsigned long' or other
> > changes or no need to do anything?

I thought it would be a good time to ask again whether I need patching
or not for this warning?

Thanks

> >
> >
> >
> > On Sat, 18 Nov 2023 at 00:00, Dipendra Khadka <kdipendra88@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > I am not sure why spare did not find the difference between 'long' and 'unsigned long'
> > > in this particular case. I saw that in else case,there is use of unsigned long and sparse
> > > does not report a warning .Hence I thought casting to unsigned long will solve the problem.
> > > Also, there are not any other warnings thrown by spare in the uaccess_64.h file.
> > >
> > > I think casting x  to  'void __user *'before checking whether it is greater than or equal to zero
> > > in valid_user_address() will be more sensible and fix the warning either.
> > >
> > > Is this ok for you? Or have to cast to 'unsigned long' or no need to do anything?
> > >
> > >
> > > On Fri, 17 Nov 2023 at 21:04, Dave Hansen <dave.hansen@intel.com> wrote:
> > >>
> > >> On 11/16/23 09:38, Dipendra Khadka wrote:
> > >> > Sparse has identified a warning as follows:
> > >> >
> > >> > ./arch/x86/include/asm/uaccess_64.h:88:24: warning: cast removes address space '__user' of expression.
> > >> >
> > >> > Since the valid_user_address(x) macro implicitly casts the argument
> > >> > to long and compares the converted value of x to zero, casting ptr
> > >> > to unsigned long has no functional impact and does not trigger a
> > >> > Sparse warning either.
> > >>
> > >> Why does sparse complain about a cast to 'long' but not 'unsigned long'?
> > >>  Both remove the '__user' address space from the expression.  Were there
> > >> just so many __user pointers being cast to 'unsigned long' that there's
> > >> an exception in sparse for 'void __user *' => 'unsigned long'?
> > >>
> > >> Either way, if we're going to fix it it seems like it would be better to
> > >> valid_user_address() actually handle, well, __user addresses rather than
> > >> expecting callers to do it.
--
#Dipendra Khadka

Re: [PATCH] x86: Fixes warning: cast removes address space '__user' of expression in uaccess_64.h

[Dmitry Torokhov]

On Fri, Nov 17, 2023 at 07:19:12AM -0800, Dave Hansen wrote:
> On 11/16/23 09:38, Dipendra Khadka wrote:
> > Sparse has identified a warning as follows:
> > 
> > ./arch/x86/include/asm/uaccess_64.h:88:24: warning: cast removes address space '__user' of expression.
> > 
> > Since the valid_user_address(x) macro implicitly casts the argument
> > to long and compares the converted value of x to zero, casting ptr
> > to unsigned long has no functional impact and does not trigger a 
> > Sparse warning either.
> 
> Why does sparse complain about a cast to 'long' but not 'unsigned long'?
>  Both remove the '__user' address space from the expression.  Were there
> just so many __user pointers being cast to 'unsigned long' that there's
> an exception in sparse for 'void __user *' => 'unsigned long'?

Yes, unsigned long is special:

commit 7816e4c4a2dba6fef24c9a52c6b17a8cde0c8138
Author: Linus Torvalds <torvalds@ppc970.osdl.org>
Date:   Mon May 31 13:18:57 2004 -0700

    Allow casting of user pointers to "unsigned long".

    It's reasonably common to do special pointer arithmetic
    in unsigned long, and making people force the cast just
    adds noise.


I wonder if we should have:

#define valid_user_address(x) ((__force long)(x) >= 0)

or  

#define valid_user_address(x) ((long)(unsigned long)(x) >= 0)

Thanks.

-- 
Dmitry