| Message ID | alpine.LNX.2.00.1208020911300.14910@pobox.suse.cz (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, 2012-08-02 at 09:12 +0200, Jiri Kosina wrote: > Directly comparing task_struct->personality against PER_* is not fully > correct, as it doesn't take flags potentially stored in top three bytes > into account. > > Analogically, directly forcefully setting personality to PER_LINUX32 or > PER_LINUX discards any flags stored in the top three bytes. > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > --- > > changed since v1: fix the bit ops to reflect the fact that PER_LINUX is > actually 0 Tell me what you're trying to achieve (or what problem you're trying to solve), because personality is notoriously tricky. Usually personality = <whatever> is desired because if you look at the enum for personality, it does set the top three bits in quite a few cases. We actually don't need any of the top 3 byte features ... but I think making sure they're zeroed out is the correct thing to do. In particular, if we ever get around to a 64 bit user space, our personality will be PER_LINUX_FDPIC and then we will be setting the top three bytes. James > arch/parisc/kernel/process.c | 2 +- > arch/parisc/kernel/sys_parisc.c | 8 ++++---- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c > index d4b94b3..2c05a92 100644 > --- a/arch/parisc/kernel/process.c > +++ b/arch/parisc/kernel/process.c > @@ -309,7 +309,7 @@ copy_thread(unsigned long clone_flags, unsigned long usp, > cregs->ksp = (unsigned long)stack > + (pregs->gr[21] & (THREAD_SIZE - 1)); > cregs->gr[30] = usp; > - if (p->personality == PER_HPUX) { > + if (personality(p->personality) == PER_HPUX) { > #ifdef CONFIG_HPUX > cregs->kpc = (unsigned long) &hpux_child_return; > #else > diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c > index c9b9322..7f9658e 100644 > --- a/arch/parisc/kernel/sys_parisc.c > +++ b/arch/parisc/kernel/sys_parisc.c > @@ -225,12 +225,12 @@ long parisc_personality(unsigned long personality) > long err; > > if (personality(current->personality) == PER_LINUX32 > - && personality == PER_LINUX) > - personality = PER_LINUX32; > + && personality(personality) == PER_LINUX) > + personality |= PER_LINUX32; > > err = sys_personality(personality); > - if (err == PER_LINUX32) > - err = PER_LINUX; > + if (personality(err) == PER_LINUX32) > + err &= ~PER_LINUX32; > > return err; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2 Aug 2012, James Bottomley wrote: > > Directly comparing task_struct->personality against PER_* is not fully > > correct, as it doesn't take flags potentially stored in top three bytes > > into account. > > > > Analogically, directly forcefully setting personality to PER_LINUX32 or > > PER_LINUX discards any flags stored in the top three bytes. > > > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > > --- > > > > changed since v1: fix the bit ops to reflect the fact that PER_LINUX is > > actually 0 > > Tell me what you're trying to achieve (or what problem you're trying to > solve), because personality is notoriously tricky. Long story short -- I was debugging a problem where 'setarch --uname-2.6' would not work on s390 on (older) 3.x kernel. Turned out to be a corruption of top bytes of personality across exec() on s390. I was doing quite some over-the-tree grepping during this, and found out that at least parisc, powerpc and sparc64 (davem already queued my patch for this) would under some silently ignore discard the top bytes of personality flags passed to sys_personality(). In case of parsic, let's take a process with current->personality == PER_LINUX32 callling personality(PER_LINUX | UNAME26). The if (personality(current->personality) == PER_LINUX32 && personality == PER_LINUX) personality = PER_LINUX32; would that have no effect, and sys_personality() would be called with (PER_LINUX | UNAME26) instead of PER_LINUX32, just because of UNAME26 being set as well. That doesn't seem really correct. Is it?
On Thu, 2012-08-02 at 13:45 +0200, Jiri Kosina wrote: > On Thu, 2 Aug 2012, James Bottomley wrote: > > > > Directly comparing task_struct->personality against PER_* is not fully > > > correct, as it doesn't take flags potentially stored in top three bytes > > > into account. > > > > > > Analogically, directly forcefully setting personality to PER_LINUX32 or > > > PER_LINUX discards any flags stored in the top three bytes. > > > > > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > > > --- > > > > > > changed since v1: fix the bit ops to reflect the fact that PER_LINUX is > > > actually 0 > > > > Tell me what you're trying to achieve (or what problem you're trying to > > solve), because personality is notoriously tricky. > > Long story short -- I was debugging a problem where 'setarch --uname-2.6' > would not work on s390 on (older) 3.x kernel. Turned out to be a > corruption of top bytes of personality across exec() on s390. > > I was doing quite some over-the-tree grepping during this, and found out > that at least parisc, powerpc and sparc64 (davem already queued my patch > for this) would under some silently ignore discard the top bytes of > personality flags passed to sys_personality(). > > In case of parsic, let's take a process with current->personality == > PER_LINUX32 callling personality(PER_LINUX | UNAME26). The > > if (personality(current->personality) == PER_LINUX32 > && personality == PER_LINUX) > personality = PER_LINUX32; > > would that have no effect, and sys_personality() would be called with > (PER_LINUX | UNAME26) instead of PER_LINUX32, just because of UNAME26 > being set as well. That doesn't seem really correct. Is it? Heh, no. This is a nasty mess. Our assumption was that we own the flags ... hence the code. There are some flags we can't allow to be set or reset ... but that's only for the mythical 64 bit userspace, so I suppose we just ignore that for now and fix it if this ever appears. I don't like the hidden assumption that PER_LINUX == 0, but otherwise the code looks fine. How about personality = (personality & ~PER_MASK) | PER_LINUX32; and err = (personality & ~PER_MASK) | PER_LINUX; James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c index d4b94b3..2c05a92 100644 --- a/arch/parisc/kernel/process.c +++ b/arch/parisc/kernel/process.c @@ -309,7 +309,7 @@ copy_thread(unsigned long clone_flags, unsigned long usp, cregs->ksp = (unsigned long)stack + (pregs->gr[21] & (THREAD_SIZE - 1)); cregs->gr[30] = usp; - if (p->personality == PER_HPUX) { + if (personality(p->personality) == PER_HPUX) { #ifdef CONFIG_HPUX cregs->kpc = (unsigned long) &hpux_child_return; #else diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c index c9b9322..7f9658e 100644 --- a/arch/parisc/kernel/sys_parisc.c +++ b/arch/parisc/kernel/sys_parisc.c @@ -225,12 +225,12 @@ long parisc_personality(unsigned long personality) long err; if (personality(current->personality) == PER_LINUX32 - && personality == PER_LINUX) - personality = PER_LINUX32; + && personality(personality) == PER_LINUX) + personality |= PER_LINUX32; err = sys_personality(personality); - if (err == PER_LINUX32) - err = PER_LINUX; + if (personality(err) == PER_LINUX32) + err &= ~PER_LINUX32; return err; }
Directly comparing task_struct->personality against PER_* is not fully correct, as it doesn't take flags potentially stored in top three bytes into account. Analogically, directly forcefully setting personality to PER_LINUX32 or PER_LINUX discards any flags stored in the top three bytes. Signed-off-by: Jiri Kosina <jkosina@suse.cz> --- changed since v1: fix the bit ops to reflect the fact that PER_LINUX is actually 0 arch/parisc/kernel/process.c | 2 +- arch/parisc/kernel/sys_parisc.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)