linux-kernel - Re: [GIT pull] irq updates for 4.13

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:   Tue, 11 Jul 2017 19:17:14 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Tony Lindgren <tony@...mide.com>
cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Sebastian Reichel <sebastian.reichel@...labora.co.uk>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Pavel Machek <pavel@....cz>,
        Linus Walleij <linus.walleij@...aro.org>,
        Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [GIT pull] irq updates for 4.13

On Tue, 11 Jul 2017, Tony Lindgren wrote:
> * Linus Torvalds <torvalds@...ux-foundation.org> [170711 08:40]:
> > On Tue, Jul 11, 2017 at 7:41 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> > >
> > > Ah. Now that makes sense.
> > >
> > > Unpatched the ordering is:
> > >
> > >           chip_bus_lock(desc);
> > >           irq_request_resources(desc);
> > 
> > I *looked* at that ordering and then went "Naah, that makes no sense".
> > 
> > But if that's the only issue, how about we just re-order those things
> > - we still don't need to move the irq_request_resources() into the
> > spinlock, we just move it to below the chip_bus_lock().
> > 
> > IOW, something like the (COMPLETELY UNTEESTED!) attached patch.
> 
> Yeah that fixes the issue:
> 
> Tested-by: Tony Lindgren <tony@...mide.com>
> 
> > This assumes that the chip_bus_lock() thing is still ok for the RT
> > case, but it looks like it might be: the only other one I looked at
> > (apart from the gpio-omap one) used a mutex.
> 
> Yeah and the ordering below makes more sense to me at least. That is
> assuming we want to call chip_bus_lock() before we start calling the
> chip functions :)

We can do that, just the free path is ugly and does not really work that
way.

__free_irq()
	....
	chip_bus_sync_unlock(desc);
	...
	synchronize_irq(irq);
	...
	if (!desc->action) {
		irq_release_resources();
		irq_remove_timings();
	}
	mutex_unlock(&desc->request_mutex);

We can't release request_mutex early otherwise we run into the issue of a
concurrent request_irq() trying to reuse stuff which we just release, but
we can't reacquire bus_lock under request_mutex either when we change the
lock ordering to bus_lock -> desc->request_mutex -> desc->lock.

We really want to have both the release_resources() and the
remove_timings() calls outside of the spinlocked region. That's not only a
RT issue, there have been requests for making the resource call 'sleepable'
for mainline as well.

Below is a slightly different fix, which keeps the lock order

      desc->request_mutex -> bus_lock -> desc->lock

intact and conditionally reacquired the bus lock for the release call.

Thanks,

	tglx

8<------------------------
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1036,13 +1036,20 @@ static int irq_request_resources(struct
 	return c->irq_request_resources ? c->irq_request_resources(d) : 0;
 }
 
-static void irq_release_resources(struct irq_desc *desc)
+static void irq_release_resources(struct irq_desc *desc, bool buslock)
 {
 	struct irq_data *d = &desc->irq_data;
 	struct irq_chip *c = d->chip;
 
-	if (c->irq_release_resources)
-		c->irq_release_resources(d);
+	if (!c->irq_release_resources)
+		return;
+	if (buslock)
+		chip_bus_lock(desc);
+
+	c->irq_release_resources(d);
+
+	if (buslock)
+		chip_bus_sync_unlock(desc);
 }
 
 static int
@@ -1168,17 +1175,16 @@ static int
 		new->flags &= ~IRQF_ONESHOT;
 
 	mutex_lock(&desc->request_mutex);
+	chip_bus_lock(desc);
 	if (!desc->action) {
 		ret = irq_request_resources(desc);
 		if (ret) {
 			pr_err("Failed to request resources for %s (irq %d) on irqchip %s\n",
 			       new->name, irq, desc->irq_data.chip->name);
-			goto out_mutex;
+			goto out_bus;
 		}
 	}
 
-	chip_bus_lock(desc);
-
 	/*
 	 * The following block of code has to be executed atomically
 	 */
@@ -1286,10 +1292,8 @@ static int
 			ret = __irq_set_trigger(desc,
 						new->flags & IRQF_TRIGGER_MASK);
 
-			if (ret) {
-				irq_release_resources(desc);
+			if (ret)
 				goto out_unlock;
-			}
 		}
 
 		desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
@@ -1385,12 +1389,10 @@ static int
 out_unlock:
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
-	chip_bus_sync_unlock(desc);
-
 	if (!desc->action)
-		irq_release_resources(desc);
-
-out_mutex:
+		irq_release_resources(desc, false);
+out_bus:
+	chip_bus_sync_unlock(desc);
 	mutex_unlock(&desc->request_mutex);
 
 out_thread:
@@ -1472,6 +1474,7 @@ static struct irqaction *__free_irq(unsi
 			WARN(1, "Trying to free already-free IRQ %d\n", irq);
 			raw_spin_unlock_irqrestore(&desc->lock, flags);
 			chip_bus_sync_unlock(desc);
+			mutex_unlock(&desc->request_mutex);
 			return NULL;
 		}
 
@@ -1531,7 +1534,7 @@ static struct irqaction *__free_irq(unsi
 	}
 
 	if (!desc->action) {
-		irq_release_resources(desc);
+		irq_release_resources(desc, true);
 		irq_remove_timings(desc);
 	}