linux-kernel - Re: [PATCH] synchronize

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]

Message-ID: <alpine.LFD.0.999.0710171906150.26902@woody.linux-foundation.org>
Date:	Wed, 17 Oct 2007 19:12:20 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
cc:	akpm <akpm@...ux-foundation.org>,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	linuxppc-dev list <linuxppc-dev@...abs.org>
Subject: Re: [PATCH] synchronize_irq needs a barrier

On Thu, 18 Oct 2007, Benjamin Herrenschmidt wrote:
>  
> +	smp_mb();
>  	while (desc->status & IRQ_INPROGRESS)
>  		cpu_relax();

So, what exactly does it protect against? At a minimum, this needs a 
comment in the changelog, and probably preferably in the source code too.

The thing is, synchronize_irq() can only protect against interrupts that 
are *already*running* on another CPU, and the caller must have made sure 
that no new interrupts are coming in (or at least that whatever new 
interrupts that come in will not pick up a certain piece of data). 

So I can imagine that the smb_mb() is there so that the caller - who has 
cleared some list or done something like that - will have any preceding 
writes that it did be serialized with actually checking the old state of 
"desc->status".

Fair enough - I can see that a smp_mb() is useful. But I think it needs 
documenting as such, and preferably with an example of how this actually 
happened in the first place (do you have one?)

The synchronize_irq() users that are really fundamental (ie the irq 
datastructures themselves) all should use the irq descriptor spinlock, and 
should *not* be needing any of this, since they would have serialized with 
whoever actually set the IRQ_INPROGRESS bit in the first place.

So in *that* sense, I think the memory barrier is useless, and I can't 
make up my mind whether it's good or bad. Which is why I'd really like to 
have an example scenario spelled out where it makes a difference..

Ok?

		Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/