linux-kernel - Re: Bug in on_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 01 Mar 2007 12:03:57 -0800
From:	Zachary Amsden <zach@...are.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	Daniel Hecht <dhecht@...are.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: Bug in on_each_cpu?

Andrew Morton wrote:
> On Thu, 01 Mar 2007 03:34:18 -0800 Zachary Amsden <zach@...are.com> wrote:
>
>   
>>> Why is it a bug?  Because there's a deadlock where this CPU is waiting for
>>> CPU A to take the IPI, but CPU A is waiting (with interrupts disabled) for
>>> this CPU to take an IPI.
>>>   
>>>       
>> Then the bug is not in on_each_cpu().  It is in the usage of 
>> clock_was_set().  For example, look at do_settimeofday in kernel/timer.c:
>>
>>         write_sequnlock_irqrestore(&xtime_lock, flags);
>>
>>         /* signal hrtimers about time change */
>>         clock_was_set();
>>
>>         return 0;
>>     
>
> Perhaps a WARN_ON(irqs_disabled()) in clock_was_set() would help.  But probably
> the one in smp_call_function() will suffice.
>   

I'm a little unsure of how the timers play with sysfs suspend/resume, 
but calling do_settimeofday with irqs disabled is now clearly a bug.  
I'm thinking if these two cases don't fire under use, then we can safely 
down-convert the call sites.  Ingo, Thomas, what do you think of this 
(untested, unverified, patch attached):  is it true?

> yup.  I once played with adding warnings in places like spin_lock_irq(), 
> but there were false positives from places which were odd-but-correct.
>
> It would be worth revisiting however.
>   

Yes, I recall non-linear control flow in the floppy and ide drivers that 
did this, and were odd, but correct under certain call conventions.  
Whether they remain correct or still exist should be worth revisiting.

I think at least in the timer code, we should preserve the same 
invariants across the two implementations of do_getttimeofday.  One uses 
write_seqlock_irq, the other write_seqlock_irqsave.  This confused me 
into thinking it was safe to call do_gettimeofday with irqs disabled (it 
used to be in 2.6.20).  But they both call clock_was_set, and thus 
on_each_cpu, so either both of them have a bug or they both can be 
reduced to irq instead of irqsave.  I'm guessing the latter is the case.

Zach

View attachment "gettimeofday-irq-reduce.patch" of type "text/plain" (1169 bytes)