lkml.org 
[lkml]   [2010]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Q: sys_personality() && misc oddities
On 05/25, Roland McGrath wrote:
>
> I don't think we're ever going to need or want a 64-bit personality word.
> (There are still 10 bits unused in the middle for more flags.)

OK,

> Though the high bit might be set on 32-bit, there still should not really
> be a danger of misinterpreting a value as an error code--as long as we
> haven't used up all 10 of those middle bits. The test userland (glibc)
> uses is not "long < 0" but "u_long > -4095UL". So as long as at least
> one bit in 0xff00 remains clear, it won't match.

Yes, libc itself is fine. But from the application's pov, personality()
returns int, not long.

> For 64-bit you want to avoid sign-extension of the old value just so it
> looks valid (even though it won't look like an error code). I think the
> most sensible thing is to change the task_struct field to 'unsigned int'.

it is already 'unsigned int' ;)

> I think the 0xffffffff check is intended specifically for personality(-1)
> to be a no-op that returns the old value, and nothing more.

I think the same.

> OTOH, this:
>
> set_personality(personality);
> if (current->personality != personality)
> return -EINVAL;
>
> will then both do the job in set_personality() and return -EINVAL, when
> some high bits are set.

Yes! and despite the fact it returns -EINVAL, current->personality was
changed. This can't be right.

> So, perhaps you are right about checking high
> bits. Then I'd make it:
>
> if ((int) personality != -1) {
> if (unlikely((unsigned int) personality != personality))
> return -EINVAL;

Well. Think about personality(0xffffffff - 1). It passes both checks
and we change current->personality. Then the application calls
personality() again, we return the old value, and since the user-space
expects "int" it gets -2.

How about

if (personality != 0xffffffff) {
if (personality >= 0x7fffffff)
return -EINVAL;
set_personality(personality);
}

? Now that personality always fits into "insigned int" we don't need
to recheck current->personality == personality, and "< 0x7fffffff"
gurantees that "int old_personality = personality(whatever)" in user
space can be never misinterpeted as error.

As for the other oddities, they need the separate patches. Or we can
just leave this code alone ;)

Oleg.



\
 
 \ /
  Last update: 2010-05-26 14:41    [W:0.084 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site