Re: Thread flags modified without set_thread_flag() (non atomically)

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 26 Feb 2007 12:10:37 -0800 Mathieu Desnoyers <compudj@google.com> 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.