generic uaccess.h

generic uaccess.h

[Michal Simek]

Hi Arnd,

I have just look at asm-generic uaccess.h and there is one thing which
seems to me wrong.

For put_user macro - you use __copy_to_user but you have for 64bit case
ifdef CONFIG_64BIT
but  look at fs/eventfd: eventfd_read function. At least for this
function(syscall) is necessary "return" 64bit
value on 32bit machines too.
IMHO that ifdef CONFIG_64BIT shouldn't be there.

What do you think?
If you agree with me, I'll generate proper patch with description.

Thanks,
Michal


diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
index b218b85..bbbb983 100644
--- a/include/asm-generic/uaccess.h
+++ b/include/asm-generic/uaccess.h
@@ -119,11 +119,9 @@ static inline __must_check long __copy_to_user(void
__user
                case 4:
                        *(u32 __force *)to = *(u32 *)from;
                        return 0;
-#ifdef CONFIG_64BIT
                case 8:
                        *(u64 __force *)to = *(u64 *)from;
                        return 0;
-#endif
                default:
                        break;
                }


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854

Re: generic uaccess.h

[Arnd Bergmann]

On Friday 24 July 2009, Michal Simek wrote:
> I have just look at asm-generic uaccess.h and there is one thing which
> seems to me wrong.
> 
> For put_user macro - you use __copy_to_user but you have for 64bit case
> ifdef CONFIG_64BIT
> but  look at fs/eventfd: eventfd_read function. At least for this
> function(syscall) is necessary "return" 64bit
> value on 32bit machines too.
> IMHO that ifdef CONFIG_64BIT shouldn't be there.
> 
> What do you think?
> If you agree with me, I'll generate proper patch with description.

The code was intentional, because 32 bit architectures normally
don't acces u64 values efficiently. I would expect the memcpy
to produce better object code in that case.
Did you see an actual bug in my version or are you only
guessing that the assignment should work better than the
memcpy?

What object code do you get with

int test(unsigned long long __user *out, unsigned long long in)
{
	return put_user(in, ptr);
}

in both cases?

	Arnd <><

Re: generic uaccess.h

[Michal Simek]

Arnd Bergmann wrote:
> On Friday 24 July 2009, Michal Simek wrote:
>> I have just look at asm-generic uaccess.h and there is one thing which
>> seems to me wrong.
>>
>> For put_user macro - you use __copy_to_user but you have for 64bit case
>> ifdef CONFIG_64BIT
>> but  look at fs/eventfd: eventfd_read function. At least for this
>> function(syscall) is necessary "return" 64bit
>> value on 32bit machines too.
>> IMHO that ifdef CONFIG_64BIT shouldn't be there.
>>
>> What do you think?
>> If you agree with me, I'll generate proper patch with description.
> 
> The code was intentional, because 32 bit architectures normally
> don't acces u64 values efficiently. I would expect the memcpy
> to produce better object code in that case.
> Did you see an actual bug in my version or are you only
> guessing that the assignment should work better than the
> memcpy?

I have just compile noMMU version and first bootup failed.
I miss that memcpy after it. Ooou.

I'll let you know what my results are.

Michal

> 
> What object code do you get with
> 
> int test(unsigned long long __user *out, unsigned long long in)
> {
> 	return put_user(in, ptr);
> }
> 
> in both cases?


> 
> 	Arnd <><


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854

Re: generic uaccess.h

[Michal Simek]

Arnd Bergmann wrote:
> On Friday 24 July 2009, Michal Simek wrote:
>> I have just look at asm-generic uaccess.h and there is one thing which
>> seems to me wrong.
>>
>> For put_user macro - you use __copy_to_user but you have for 64bit case
>> ifdef CONFIG_64BIT
>> but  look at fs/eventfd: eventfd_read function. At least for this
>> function(syscall) is necessary "return" 64bit
>> value on 32bit machines too.
>> IMHO that ifdef CONFIG_64BIT shouldn't be there.
>>
>> What do you think?
>> If you agree with me, I'll generate proper patch with description.
> 
> The code was intentional, because 32 bit architectures normally
> don't acces u64 values efficiently. I would expect the memcpy
> to produce better object code in that case.
> Did you see an actual bug in my version or are you only
> guessing that the assignment should work better than the
> memcpy?
> 
> What object code do you get with
> 
> int test(unsigned long long __user *out, unsigned long long in)
> {
> 	return put_user(in, ptr);
> }
> 
> in both cases?

I found that I have problem on noMMU with replacing
put_user macro

with

#define put_user(x, ptr)					\
({								\
	might_sleep();						\
	access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)) ?		\
		__put_user(x, ptr) :				\
		-EFAULT;					\
})

(The similar issue is for get_user) (__put_user function is origin - not asm-generic)

I am getting any short write error. It is caused with adding access_ok macro.
It is weird.

RPC: Registered tcp transport module.
VFS: Mounted root (romfs filesystem) readonly on device 31:0.
Freeing unused kernel memory: 100k freed
0: short write
Kernel panic - not syncing: Attempted to kill init!


I have more important work in front of me - I take a look at it later.

Michal


> 
> 	Arnd <><


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854

Re: generic uaccess.h

[Arnd Bergmann]

On Monday 27 July 2009, Michal Simek wrote:
> I am getting any short write error. It is caused with adding access_ok macro.
> It is weird.

Your implementation of access_ok() is minimal, but should be ok
in theory. Note that even on nommu, it might make sense
to check for kernel addresses here and implement get_fs/set_fs,
e.g.

int access_ok(int type, unsigned long addr, size_t len)
{
	/* check wraparound */
	if (addr + len < addr)
		return 0;

	if (addr < memory_start || addr + len > memory_start)
		return 0;

	/* don't allow access to kernel memory */
	if (get_fs() == USER_DS) {
		if (is_kernel(addr) || is_kernel(addr + len))
			return 0;
	}

	return 1;
}

> I have more important work in front of me - I take a look at it later.

ok. When you get to debug this, I guess the easiest way is to stick
some printk into your access_ok() function.

	Arnd <><