On Mon, 26 Feb 2007 12:10:37 -0800 Mathieu Desnoyers <[EMAIL PROTECTED]> wrote:
> Hi,
How come I'm the only person around here with a Reply button?
> Looking into the thread flags, I found out that some architecture
> specific kernel functions (in 2.6.20) sets the thread flags with non
> atomic operation.
>
> A good way to list the most trivial : grep -r TIF_ * | grep =
>
> Some examples follows. If, for instance,
> x86_64/kernel/process.c:flush_thread is called from an exec system call,
> it will do the following :
>
> x86_64/kernel/process.c: t->flags ^= (_TIF_ABI_PENDING |
> _TIF_IA32);
> x86_64/kernel/process.c: t->flags &= ~_TIF_DEBUG;
>
> void flush_thread(void)
> {
> struct task_struct *tsk = current;
> struct thread_info *t = current_thread_info();
>
> if (t->flags & _TIF_ABI_PENDING) {
> t->flags ^= (_TIF_ABI_PENDING | _TIF_IA32);
> if (t->flags & _TIF_IA32)
> current_thread_info()->status |= TS_COMPAT;
> }
> t->flags &= ~_TIF_DEBUG;
> ....
>
> As long as the flags are only updated by the thread itself at this
> moment, it seems safe, but if other updates coming from other threads
> are expected, wouldn't it result in a bad behavior ?
>
> i.e if resched_task ia being called by another CPU at the same time for
> this specific thread would set the TIF_NEED_RESCHED flag, but it could
> be overwritten by the non-atomic modification in flush_thread.
It does seem risky. Perhaps it is a micro-optimisation which utilises
knowledge that this thread_struct cannot be looked up via any path in this
context.
Or perhaps it is a bug. Andi, can you please comment?
> And about this specific flush_thread, I am puzzled about the t->flags ^=
> (_TIF_ABI_PENDING | _TIF_IA32); line. The XOR will clearly flip the
> _TIF_ABI_PENDING bit to 0, and very likely set _TIF_IA32 to the opposite
> of its current value. Why does this change need to be written atomically
> (can other threads play with these flags ?) ?
>
Don't know.
>
>
> Other examples :
>
> sparc64/kernel/ptrace.c: if
> ((task_thread_info(child)->flags & _TIF_32BIT) != 0) {
> sparc64/kernel/process.c: t->flags ^= (_TIF_ABI_PENDING |
> _TIF_32BIT);
> sparc64/kernel/process.c: t->flags &= ~_TIF_PERFCTR;
>
> sparc/kernel/process.c: current_thread_info()->flags &=
> ~_TIF_USEDFPU;
> sparc/kernel/process.c: current_thread_info()->flags &=
> ~_TIF_USEDFPU;
> sparc/kernel/process.c: current_thread_info()->flags &=
> ~_TIF_USEDFPU;
> sparc/kernel/process.c: current_thread_info()->flags &=
> ~(_TIF_USEDFPU);
> sparc/kernel/traps.c: current_thread_info()->flags |= _TIF_USEDFPU;
> sparc/kernel/traps.c: task_thread_info(fpt)->flags &= ~_TIF_USEDFPU;
That all looks rather deliberate.
> powerpc/kernel/process.c: t->flags ^= (_TIF_ABI_PENDING |
> _TIF_32BIT);
>
> ia64/kernel/mca.c: ti->flags = _TIF_MCA_INIT;
>
> avr32/kernel/ptrace.c: ti->flags |= _TIF_BREAKPOINT;
No, I don't immediately see anything in the flush_old_exec() code path
which tells us that nobody else can look up this thread_info (or be holding
a ref to it) in this context.
> avr32/kernel/ptrace.c: ti->flags |= TIF_SINGLE_STEP;
heh. Haarvard, you got a bug.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
