linux-kernel - Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1357252019.2685.45.camel@bwh-desktop.uk.solarflarecom.com>
Date:	Thu, 3 Jan 2013 22:26:59 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	David Decotigny <decot@...glers.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Amir Vadai <amirv@...lanox.com>,
	"Paul E. McKenney" <paul.mckenney@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Josh Triplett <josh@...htriplett.org>,
	David Howells <dhowells@...hat.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>
Subject: Re: [PATCH v4] lib: cpu_rmap: avoid flushing all workqueues

On Thu, 2013-01-03 at 13:47 -0800, Andrew Morton wrote:
> On Wed, 2 Jan 2013 18:35:00 -0800
> David Decotigny <decot@...glers.com> wrote:
> >
> 
> (please don't top-post)
> 
> > Thanks. It is not too late to review this code. But I'd prefer not to
> > address refactoring issues with this patch, which is supposed to fix a
> > deadlock with some drivers. So I'd rather not to add function
> > renaming, suppressions, etc. that have an effect outside cpu_rmap's
> > code. Instead, I'd like to propose another patch later, which will be
> > a little more intrusive in that respect, if that's ok with you.
> > 
> > I believe Ben answered your other concerns, I consider him as the
> > expert as to whether there should be finer-grained locking implemented
> > in this subsystem. Let me just add that I second him in saying that
> > the deadlock risk was clearly identified and mentioned in the doc.
> > Unfortunately, initial implementation makes this risk hard to
> > work-around for some drivers, which is what this patch proposes to
> > address.
> 
> ^^ all this pertains to the existing code.
> 
> > So, for now, I'd like to keep v4 as the current version. And some
> > refactoring will be done in a later patch.
> 
> ^^ this pertains to the current patch.  And no reason for ignoring my
> review comments was provided.
> 
> So I did it myself.  It's very simple, as free_cpu_rmap() has no callers.
> 
> Also, free_irq_cpu_rmap() is now distinctly fishy - it runs all the
> notifiers every time it is called.

It removes the notifiers.

> Surely it should only do that when the refcount falls to zero?
[...]

No, absolutely not.  An IRQ cpu_rmap may have multiple references to it
now, but it only has one owner: the driver that allocates it and the
associated IRQs.

As soon as the driver frees those IRQs, they may be allocated by some
other driver, so we require that notifiers are removed from them first
(see the WARN_ON in free_irq()).  Also, if free_irq_cpu_rmap() did not
remove the notifiers, more notifications could be scheduled and keep the
cpu_rmap alive indefinitely.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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