Thread flags modified without set_thread_flag() (non atomically)

Thread flags modified without set_thread_flag() (non atomically)

[Mathieu Desnoyers]

Hi,

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.

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 ?) ?

Mathieu


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;

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;
avr32/kernel/ptrace.c:                  ti->flags |= TIF_SINGLE_STEP;

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

[Andrew Morton]

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.

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

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Wed, 28 Feb 2007 22:03:49 -0800

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

Provide the counter example, what other threads of control can modify
relevant flags while a thread is exec()'ing?  It's essentially frozen
outside of these code paths, otherwise we wouldn't be able to make all
of these modifications to the task state.

I think these cases are very safe.

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

[Andrew Morton]

On Wed, 28 Feb 2007 22:23:41 -0800 (PST) David Miller <davem@davemloft.net> wrote:

> > 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.
> 
> Provide the counter example, what other threads of control can modify
> relevant flags while a thread is exec()'ing?  It's essentially frozen
> outside of these code paths, otherwise we wouldn't be able to make all
> of these modifications to the task state.

As Mathieu points out, resched_task(), perhaps.  It can be called from interrupt.

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

[Haavard Skinnemoen]

[trimming cc list since I'm only replying to the avr32 part]

On Wed, 28 Feb 2007 22:03:49 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 26 Feb 2007 12:10:37 -0800 Mathieu Desnoyers <compudj@google.com> wrote:

> > 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.

Heh, yeah. That would indeed explain some strange gdb behaviour. It
will only trigger when single-stepping into an exception or interrupt
handler so thanks for pointing it out; I would have had a hard time
figuring it out on my own...

I don't think either of those need to be atomic though, since both of
them happen in monitor mode with interrupts disabled.

Haavard

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

[Andrew Morton]

On Thu, 1 Mar 2007 10:34:51 +0100 Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:

> > On Mon, 26 Feb 2007 12:10:37 -0800 Mathieu Desnoyers <compudj@google.com> wrote:
> 
> > > 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.
> 
> Heh, yeah. That would indeed explain some strange gdb behaviour. It
> will only trigger when single-stepping into an exception or interrupt
> handler so thanks for pointing it out; I would have had a hard time
> figuring it out on my own...

yup, tricky.

If there's a lesson here, it is "don't provide #defines in the header for
both versions".

The block code does a similar thing:

#define REQ_RW          (1 << __REQ_RW)
#define REQ_FAILFAST    (1 << __REQ_FAILFAST)
#define REQ_SORTED      (1 << __REQ_SORTED)
#define REQ_SOFTBARRIER (1 << __REQ_SOFTBARRIER)

and I've caught Jens using the wrong identifier at least twice in the past.

It's better I think to just provide #defines for the bit offsets and
open-code the shifting if needed.  Like PG_foo and BH_Foo.

> I don't think either of those need to be atomic though, since both of
> them happen in monitor mode with interrupts disabled.

That's true until you implement SMP ;)

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

[Haavard Skinnemoen]

On Thu, 1 Mar 2007 01:45:23 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> If there's a lesson here, it is "don't provide #defines in the header for
> both versions".

Yes, that's true. It looks like all the other arches do the same thing
with the _TIF flags, however, so just ripping it out probably won't
work. Maybe I'll give it a try just to see how much trouble it is.

> > I don't think either of those need to be atomic though, since both of
> > them happen in monitor mode with interrupts disabled.
> 
> That's true until you implement SMP ;)

True. I'm not sure what it can race against though...perhaps something
in kernel/signal.c?

Haavard

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

[Haavard Skinnemoen]

On Thu, 1 Mar 2007 11:14:47 +0100
Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:

> On Thu, 1 Mar 2007 01:45:23 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > If there's a lesson here, it is "don't provide #defines in the header for
> > both versions".
> 
> Yes, that's true. It looks like all the other arches do the same thing
> with the _TIF flags, however, so just ripping it out probably won't
> work. Maybe I'll give it a try just to see how much trouble it is.

Actually, it wasn't much trouble at all since the _TIF definitions were
only used by arch code (at least in the configurations I tested, and I
couldn't find any users by grepping.) The below patch rips them out and
fixes up the few users that depend on them.

This is just an RFC, btw. The ptrace fix needs to go in earlier than the
rest, so I have to split this differently.

Is this something other arches should consider doing as well?

> > > I don't think either of those need to be atomic though, since both of
> > > them happen in monitor mode with interrupts disabled.
> > 
> > That's true until you implement SMP ;)
> 
> True. I'm not sure what it can race against though...perhaps something
> in kernel/signal.c?

Nevermind, I didn't pay attention to the rest of the thread.

Haavard

===============[cut here]==============
From: Haavard Skinnemoen <hskinnemoen@atmel.com>
Subject: [AVR32] Eliminate the _TIF definitions in asm/thread_info.h

Having definitions for both bit offsets and bitmasks that only differ
by a single underscore may introduce nasty bugs that are very hard
to spot. This patch removes the thread flag bitmask definitions from
asm-avr32/thread_info.h and converts the few cases that depends on
them to use an explicit shift instead.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 arch/avr32/kernel/entry-avr32b.S |    6 +++---
 arch/avr32/kernel/ptrace.c       |    4 ++--
 arch/avr32/kernel/signal.c       |    2 +-
 include/asm-avr32/thread_info.h  |   10 ----------
 4 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/avr32/kernel/entry-avr32b.S b/arch/avr32/kernel/entry-avr32b.S
index eeb6679..fc1c435 100644
--- a/arch/avr32/kernel/entry-avr32b.S
+++ b/arch/avr32/kernel/entry-avr32b.S
@@ -250,7 +250,7 @@ syscall_exit_work:
 	ld.w	r1, r0[TI_flags]
 	rjmp	1b
 
-2:	mov	r2, _TIF_SIGPENDING | _TIF_RESTORE_SIGMASK
+2:	mov	r2, (1 << TIF_SIGPENDING) | (1 << TIF_RESTORE_SIGMASK)
 	tst	r1, r2
 	breq	3f
 	unmask_interrupts
@@ -479,7 +479,7 @@ fault_exit_work:
 	ld.w	r1, r0[TI_flags]
 	rjmp	fault_exit_work
 
-1:	mov	r2, _TIF_SIGPENDING | _TIF_RESTORE_SIGMASK
+1:	mov	r2, (1 << TIF_SIGPENDING) | (1 << TIF_RESTORE_SIGMASK)
 	tst	r1, r2
 	breq	2f
 	unmask_interrupts
@@ -588,7 +588,7 @@ debug_resume_user:
 	ld.w	r1, r0[TI_flags]
 	rjmp	1b
 
-2:	mov	r2, _TIF_SIGPENDING | _TIF_RESTORE_SIGMASK
+2:	mov	r2, (1 << TIF_SIGPENDING) | (1 << TIF_RESTORE_SIGMASK)
 	tst	r1, r2
 	breq	3f
 	unmask_interrupts
diff --git a/arch/avr32/kernel/ptrace.c b/arch/avr32/kernel/ptrace.c
index ce6734d..6f4388f 100644
--- a/arch/avr32/kernel/ptrace.c
+++ b/arch/avr32/kernel/ptrace.c
@@ -313,7 +313,7 @@ asmlinkage void do_debug_priv(struct pt_regs *regs)
 		__mtdr(DBGREG_DC, dc);
 
 		ti = current_thread_info();
-		ti->flags |= _TIF_BREAKPOINT;
+		set_ti_thread_flag(ti, TIF_BREAKPOINT);
 
 		/* The TLB miss handlers don't check thread flags */
 		if ((regs->pc >= (unsigned long)&itlb_miss)
@@ -328,7 +328,7 @@ asmlinkage void do_debug_priv(struct pt_regs *regs)
 		 * single step.
 		 */
 		if ((regs->sr & MODE_MASK) != MODE_SUPERVISOR)
-			ti->flags |= TIF_SINGLE_STEP;
+			set_ti_thread_flag(ti, TIF_SINGLE_STEP);
 	} else {
 		panic("Unable to handle debug trap at pc = %08lx\n",
 		      regs->pc);
diff --git a/arch/avr32/kernel/signal.c b/arch/avr32/kernel/signal.c
index 0ec1485..cf52d47 100644
--- a/arch/avr32/kernel/signal.c
+++ b/arch/avr32/kernel/signal.c
@@ -323,6 +323,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs, struct thread_info *ti)
 	if ((sysreg_read(SR) & MODE_MASK) == MODE_SUPERVISOR)
 		syscall = 1;
 
-	if (ti->flags & (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK))
+	if (ti->flags & ((1 << TIF_SIGPENDING) | (1 << TIF_RESTORE_SIGMASK)))
 		do_signal(regs, &current->blocked, syscall);
 }
diff --git a/include/asm-avr32/thread_info.h b/include/asm-avr32/thread_info.h
index d1f5b35..a037dde 100644
--- a/include/asm-avr32/thread_info.h
+++ b/include/asm-avr32/thread_info.h
@@ -85,16 +85,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_RESTORE_SIGMASK	8	/* restore signal mask in do_signal */
 #define TIF_USERSPACE		31      /* true if FS sets userspace */
 
-#define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
-#define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
-#define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
-#define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
-#define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
-#define _TIF_BREAKPOINT		(1 << TIF_BREAKPOINT)
-#define _TIF_SINGLE_STEP	(1 << TIF_SINGLE_STEP)
-#define _TIF_MEMDIE		(1 << TIF_MEMDIE)
-#define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
-
 /* XXX: These two masks must never span more than 16 bits! */
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK		0x0000013e
-- 
1.4.4.4

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

[Mathieu Desnoyers]

Andrew Morton wrote:
> On Thu, 1 Mar 2007 10:34:51 +0100 Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
>
>   
>>> On Mon, 26 Feb 2007 12:10:37 -0800 Mathieu Desnoyers <compudj@google.com> wrote:
>>>       
>>>> 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.
>>>       
>> Heh, yeah. That would indeed explain some strange gdb behaviour. It
>> will only trigger when single-stepping into an exception or interrupt
>> handler so thanks for pointing it out; I would have had a hard time
>> figuring it out on my own...
>>     
>
> yup, tricky.
>
> If there's a lesson here, it is "don't provide #defines in the header for
> both versions".
>
>   

Hrm, but the bitmask version is useful (and correctly used) whenever
the flag is read and tested.

The proper way to do this would be to change every use the _TIF_* flag 
in a flag comparison
for a call to test_ti_thread_flag(). I wonder if gcc optimizes multiple 
constant test_bit() applying
on the same variable linked by logical and/or so it becomes a single 
read and a small set of comparisons.

> The block code does a similar thing:
>
> #define REQ_RW          (1 << __REQ_RW)
> #define REQ_FAILFAST    (1 << __REQ_FAILFAST)
> #define REQ_SORTED      (1 << __REQ_SORTED)
> #define REQ_SOFTBARRIER (1 << __REQ_SOFTBARRIER)
>
> and I've caught Jens using the wrong identifier at least twice in the past.
>
> It's better I think to just provide #defines for the bit offsets and
> open-code the shifting if needed.  Like PG_foo and BH_Foo.
>
>   
>> I don't think either of those need to be atomic though, since both of
>> them happen in monitor mode with interrupts disabled.
>>     
>
> That's true until you implement SMP ;)
>   

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

[Andrew Morton]

On Thu, 01 Mar 2007 11:59:07 -0800
Mathieu Desnoyers <compudj@google.com> wrote:

> Andrew Morton wrote:
> > On Thu, 1 Mar 2007 10:34:51 +0100 Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> >
> >   
> >>> On Mon, 26 Feb 2007 12:10:37 -0800 Mathieu Desnoyers <compudj@google.com> wrote:
> >>>       
> >>>> 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.
> >>>       
> >> Heh, yeah. That would indeed explain some strange gdb behaviour. It
> >> will only trigger when single-stepping into an exception or interrupt
> >> handler so thanks for pointing it out; I would have had a hard time
> >> figuring it out on my own...
> >>     
> >
> > yup, tricky.
> >
> > If there's a lesson here, it is "don't provide #defines in the header for
> > both versions".
> >
> >   
> 
> Hrm, but the bitmask version is useful (and correctly used) whenever
> the flag is read and tested.
> 
> The proper way to do this would be to change every use the _TIF_* flag 
> in a flag comparison
> for a call to test_ti_thread_flag(). I wonder if gcc optimizes multiple 
> constant test_bit() applying
> on the same variable linked by logical and/or so it becomes a single 
> read and a small set of comparisons.

Well, it's unusual for code to want to test multiple bitflags in the same
operation.  Perhaps thread_info.flags is unusual in that regard.

But one can still do

	if (foo->flags & (1<<bar)|(1<<zot))

Which can get to be a pain if it happens in a lot of places.  But if it
only happens in a few places, this seems a reasonable price to pay, given
the safety gains.

Plus I'm _forever_ having to go into the header file to remember whether
REQ_RW is the bitmask or the bit offset.

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

[Andi Kleen]

> 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?

On flush_thread nobody else can mess with the thread, so yes it's a micro
optimization.

> 
> > 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.

iirc it came from DaveM originally. He just likes to write things in 
comp^wclever ways :0) It's just a little shorter.

> 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.

Normally the process flags atomicity should only matter with signals;
i don't think you can send a signal to a process being in exec this way.

-Andi

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

[Kyle Moffett]

On Mar 01, 2007, at 17:41:38, Andrew Morton wrote:
> Well, it's unusual for code to want to test multiple bitflags in  
> the same operation.  Perhaps thread_info.flags is unusual in that  
> regard.
>
> But one can still do
>
> 	if (foo->flags & (1<<bar)|(1<<zot))
>
> Which can get to be a pain if it happens in a lot of places.  But  
> if it only happens in a few places, this seems a reasonable price  
> to pay, given the safety gains.
>
> Plus I'm _forever_ having to go into the header file to remember  
> whether REQ_RW is the bitmask or the bit offset.

Hey Andrew, I think you just fell victim to an order-of-operations  
bug;  '&' has a higher priority than '|' does, so:

   if ( (foo->flags & (1<<bar)) | (1<<zot))

Or:

   if ( dynamic_value | always_true_constant )

And as such this if statement is always true.  You'd need an extra  
pair of parentheses to make it correct.

Cheers,
Kyle Moffett

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

[Andrew Morton]

On Mon, 5 Mar 2007 15:40:33 +0100
Andi Kleen <ak@suse.de> wrote:

> > 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?
> 
> On flush_thread nobody else can mess with the thread,

What about resched_task()?

> so yes it's a micro
> optimization.
> 
> > 
> > > 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.
> 
> iirc it came from DaveM originally. He just likes to write things in 
> comp^wclever ways :0) It's just a little shorter.
> 
> > 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.
> 
> Normally the process flags atomicity should only matter with signals;

Thread flags.  yes, most of them are synchronously set by their owner, but
not all, I think.


> i don't think you can send a signal to a process being in exec this way.
> 
> -Andi

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

[Mathieu Desnoyers]

Andi Kleen wrote:
>> 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?
>>     
>
> On flush_thread nobody else can mess with the thread, so yes it's a micro
> optimization.
>
>   
Hi Andi,

Here is what I think would be a counter example :

If, at the same time, we have, on x86_64 :

parent process executing :
sys_ptrace()
  (lock_kernel())
  (ptrace_get_task_struct(pid))
  arch_ptrace()
    ptrace_detach()
      ptrace_disable(child);
        clear_singlestep(child);
          clear_tsk_thread_flag(child, TIF_SINGLESTEP);
          (which clears the TIF_SINGLESTEP  flag atomically from a 
different process)
  (put_task_struct(child))
  (unlock_kernel())

And at the same time, in the child process :
sys_execve()
  do_execve()
    search_binary_handler()
      load_elf_binary()
        flush_old_exec()
          flush_thread()
            doing a non-atomic thread flag update

Is there any protection mechanism that would protect from this race 
condition
that I have missed ?

>>> 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.
>>     
>
> iirc it came from DaveM originally. He just likes to write things in 
> comp^wclever ways :0) It's just a little shorter.
>
>   
>> 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.
>>     
>
> Normally the process flags atomicity should only matter with signals;
> i don't think you can send a signal to a process being in exec this way.
>
> -Andi
>