[RESEND,RFC] irqchip: arm-gic: take gic_lock when updating irq type

Message ID 20180326120251.13054-1-aniruddhab@nvidia.com
State Deferred
Headers show
Series
  • [RESEND,RFC] irqchip: arm-gic: take gic_lock when updating irq type
Related show

Commit Message

Aniruddha Banerjee March 26, 2018, 12:02 p.m.
The kernel documentation states that the locking of the irq-chip
registers should be handled by the irq-chip driver. In the irq-gic,
the accesses to the irqchip are seemingly not protected and multiple
writes to SPIs from different irq descriptors do RMW requests without
taking the irq-chip lock. When multiple irqs call the request_irq at
the same time, there can be a simultaneous write at the gic
distributor, leading to a race. Acquire the gic_lock when the
irq_type is updated.

Signed-off-by: Aniruddha Banerjee <aniruddhab@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Marc Zyngier March 26, 2018, 12:14 p.m. | #1
On 26/03/18 13:02, Aniruddha Banerjee wrote:

[an encrypted email, again]

Can you *please* stop sending encrypted emails, specially as you target
linux-tegra@vger.kernel.org?

Thanks,

	M.
Jon Hunter March 26, 2018, 12:49 p.m. | #2
Hi Marc,

On 26/03/18 13:14, Marc Zyngier wrote:
> On 26/03/18 13:02, Aniruddha Banerjee wrote:
> 
> [an encrypted email, again]
> 
> Can you *please* stop sending encrypted emails, specially as you target
> linux-tegra@vger.kernel.org?

That's odd, this does not appear to be encrypted to me. I have also
checked from my gmail as well. I did not see anything in the header?

Cheers
Jon
Marc Zyngier March 26, 2018, 1:10 p.m. | #3
Hi Jon,

On 26/03/18 13:49, Jon Hunter wrote:
> Hi Marc,
> 
> On 26/03/18 13:14, Marc Zyngier wrote:
>> On 26/03/18 13:02, Aniruddha Banerjee wrote:
>> 
>> [an encrypted email, again]
>> 
>> Can you *please* stop sending encrypted emails, specially as you target
>> linux-tegra@vger.kernel.org?
> 
> That's odd, this does not appear to be encrypted to me. I have also
> checked from my gmail as well. I did not see anything in the header?

The email I'm getting from you is encrypted as well:

-----BEGIN PGP MESSAGE-----
Version: PGP Universal 3.3.2 (Build 21495)
Charset: utf-8

qANQR1DBwUwDtofwhD5N/iMBD/0XcZF6OYqXx9NG3lG+01IwThAqc5C2SlT/K29d
ilYqmUWLbklnCBZv7lu1zpeBMxqAw5og74lbCrBrnWmTgrehw9Gs8FDQnylsO07n
yoANBe/9DXykf5P3keX9O8cWdyklMdr1+zbC2xfp2/pn30qt25bc+VHV2QAsOf5j
+tRysiRnGrVlRtt5b8VHryAf1PGsSC3I06MaC0muEmcqO5IWfo82MdJUYSqvvypY
Ry7bzka2Tq7ooQZVGLfjPj7VJIO1V6dQcJW0zfzaVKp3BPgcTgdEoBUl+fDKY7J2
Co//0LxK0/G7oAvO0dtAzXIIP+TVvFcnlZdEoMoF3Uej7y8KvPTSFVWAqtADIY9p
6SRtHHEPaGjggIzfe9uopRJs6tye6oba0ZjpMEcEYiRgyha8hbpWwdB4FA6tyfeh
H5rBtgK/RpauUYsiDZYUMgs0lYT6U6zkfAAb/J+yV95fV3lhrrQTM4UrsFPfnBNL
kv0UyVUsgIH+WGvk2l+hirpwcS/pQNegy34V/pXzMARkzzWhrOauuosbTnYPJ8pO
l36tXl2megqsCuc/FDoPYf/+l5mZ1gX4yVC1Xbm+xBCIc+h+Zu15VBw9nZ2iPMlg
HbUFPxbKfUcgKxpDJPWV+NlcB24GZOoJ0W+IHd6TMeWtdzPevw53mEnBLUskxvpn
Lx/OW8HATAM8ookUzrlK2AEH/i8yp8yZQfnnqeXh6Px3pZLe0eubvqbXHYzcgibG
KWy53H/FiTcVWkzPBml9ZuvQ5ZPL3PZMkJ36DKmXwzd3HMBVGOK3Rgb1cYFX92zk
5q1tGDUwqKcPLX4cXbvownx6GNpCgCgghlBsxM/oaxCWfdoEnzQDMGsQ24za+bzm
VTfipRs/Hiqj0vFvq8No/jAt4KS6YAFIwbIk9RJ8/fLyE6pYrAzvvT0Fu78R9PgW
xIJ2wPh8+ixdFgAacHzbDkYrsHbDBRZCaZtdQmtNGWvc255ePKTUja5pdBY+5bGp
3vFPK3DeTQ14ObbmtJBMPNJbCECRM84BBIXW4RUISIRaQ/jSwo4B3iG245Z4pF3l
XQJElNNX0PksLW9XIgAfvi9W0LTgdpv2mrSr6VG5qAvs+Pyn4GL6DrDKafCBZbvS
yQOukICfIhR0WPHBJVXdmyTrcQ5RyQdgrxkThwtEivG9AwTtEV/Usnd/COuxFPKf
+Lb/tDK8W86cbrSUXZHT8deA3ivfQQRLvSifN8T/+rAFkqKLrOGBGnXpI5ZlH4tV
2eVaGVbDLZaoLvagJiFj3izpmILLTeSe19VfgNWtB1GNRBZexQqJmjP/KKZrJ13f
mITNVbIQDcyoJnkxImE1LWB0ujPL1MXaUA2om9G6DCPdiCnkVHC3/zthR0SR4XFe
zFIWxb4fevxnrEs91lqKlYFBCpc7RrJEvyvlUYw61AObgp6iuUD8gN3WhEYhLCHT
qXDhJH2U4rAvq3BCkZpGa4n8e2KYQvP/dPooILcfQQE8u21/7vdKBIQ0sz2rHteY
ANZ4DiB3e/bxLMttk7wtCb37VLnIvlKtORXtGFbMdyCm/SVhbT5lQ3QjwXjVpp72
B2pzU5839QDU+b511o4MEuONzr/jD7owZu+vDySJ76DDePDa8MDPD+18dMAiVKvb
X4h5dok8JAlrnKrtjKH2HzfDKLeDcRqSJ7cFJKRpg2jiKD5c4XdA3x0m94XJb5FJ
LwEcSMBjin6qa+Z9wKfozacv7VbuaYqLMwNTAHtjdKqBmEldGtv62oQzo9/pCo83
B7rjEoBmrkRB7IJi049m44JLlUXxsna9FOjke1Rcwx68QeGOrqZZ8b9+cG2uVogH
5CNMl+/Ja17mNPrN3vZNwaJIH8/aw52+ufQ5mBNrBpRAwjh/C6gQHjSiBK2zQdeM
E3mtZn387O3N1gErAtCp0uBglm7fcF13Pnvi6GQdHuqQ+nryjxR0GRWIN0lTsnEs
FNfn0SK5Xb2fisrVjfKgYbGdyFg+Wo2zhOWdEC0XWg/DA4ypji6revV2rlrfUDnk
s1CoOxBAsa+0vS7Ujjb+t71KxL15ucRFLjdT3SGOfDe3uLYGcFn7DW1RxfeRHhSX
TcNe68y9j29Rn88+qSQe1xrid/wtDi4AilZa/LTd8IKNrXw1KXQ0GOdFVTjxJ7bu
ezd5CAYpxEfnCO/j8nKqu9M2fcI=
=WVon
-----END PGP MESSAGE-----

Is that an Nvidia thing? Anyway, it looks like things get encrypted
without people asking for it. I'll reply to Aniruddha's email in clear
text, since it has made it to the list unencrypted.

Thanks,

	M.
Jon Hunter March 26, 2018, 1:47 p.m. | #4
Hi Marc,

On 26/03/18 14:10, Marc Zyngier wrote:
> On 26/03/18 13:49, Jon Hunter wrote:
>> Hi Marc,
>>
>> On 26/03/18 13:14, Marc Zyngier wrote:
>>> On 26/03/18 13:02, Aniruddha Banerjee wrote:
>>>
>>> [an encrypted email, again]
>>>
>>> Can you *please* stop sending encrypted emails, specially as you target
>>> linux-tegra@vger.kernel.org?
>>
>> That's odd, this does not appear to be encrypted to me. I have also
>> checked from my gmail as well. I did not see anything in the header?
> 
> The email I'm getting from you is encrypted as well:
> 
> -----BEGIN PGP MESSAGE-----
> Version: PGP Universal 3.3.2 (Build 21495)
> Charset: utf-8
> 
> qANQR1DBwUwDtofwhD5N/iMBD/0XcZF6OYqXx9NG3lG+01IwThAqc5C2SlT/K29d
> ilYqmUWLbklnCBZv7lu1zpeBMxqAw5og74lbCrBrnWmTgrehw9Gs8FDQnylsO07n
> yoANBe/9DXykf5P3keX9O8cWdyklMdr1+zbC2xfp2/pn30qt25bc+VHV2QAsOf5j
> +tRysiRnGrVlRtt5b8VHryAf1PGsSC3I06MaC0muEmcqO5IWfo82MdJUYSqvvypY
> Ry7bzka2Tq7ooQZVGLfjPj7VJIO1V6dQcJW0zfzaVKp3BPgcTgdEoBUl+fDKY7J2
> Co//0LxK0/G7oAvO0dtAzXIIP+TVvFcnlZdEoMoF3Uej7y8KvPTSFVWAqtADIY9p
> 6SRtHHEPaGjggIzfe9uopRJs6tye6oba0ZjpMEcEYiRgyha8hbpWwdB4FA6tyfeh
> H5rBtgK/RpauUYsiDZYUMgs0lYT6U6zkfAAb/J+yV95fV3lhrrQTM4UrsFPfnBNL
> kv0UyVUsgIH+WGvk2l+hirpwcS/pQNegy34V/pXzMARkzzWhrOauuosbTnYPJ8pO
> l36tXl2megqsCuc/FDoPYf/+l5mZ1gX4yVC1Xbm+xBCIc+h+Zu15VBw9nZ2iPMlg
> HbUFPxbKfUcgKxpDJPWV+NlcB24GZOoJ0W+IHd6TMeWtdzPevw53mEnBLUskxvpn
> Lx/OW8HATAM8ookUzrlK2AEH/i8yp8yZQfnnqeXh6Px3pZLe0eubvqbXHYzcgibG
> KWy53H/FiTcVWkzPBml9ZuvQ5ZPL3PZMkJ36DKmXwzd3HMBVGOK3Rgb1cYFX92zk
> 5q1tGDUwqKcPLX4cXbvownx6GNpCgCgghlBsxM/oaxCWfdoEnzQDMGsQ24za+bzm
> VTfipRs/Hiqj0vFvq8No/jAt4KS6YAFIwbIk9RJ8/fLyE6pYrAzvvT0Fu78R9PgW
> xIJ2wPh8+ixdFgAacHzbDkYrsHbDBRZCaZtdQmtNGWvc255ePKTUja5pdBY+5bGp
> 3vFPK3DeTQ14ObbmtJBMPNJbCECRM84BBIXW4RUISIRaQ/jSwo4B3iG245Z4pF3l
> XQJElNNX0PksLW9XIgAfvi9W0LTgdpv2mrSr6VG5qAvs+Pyn4GL6DrDKafCBZbvS
> yQOukICfIhR0WPHBJVXdmyTrcQ5RyQdgrxkThwtEivG9AwTtEV/Usnd/COuxFPKf
> +Lb/tDK8W86cbrSUXZHT8deA3ivfQQRLvSifN8T/+rAFkqKLrOGBGnXpI5ZlH4tV
> 2eVaGVbDLZaoLvagJiFj3izpmILLTeSe19VfgNWtB1GNRBZexQqJmjP/KKZrJ13f
> mITNVbIQDcyoJnkxImE1LWB0ujPL1MXaUA2om9G6DCPdiCnkVHC3/zthR0SR4XFe
> zFIWxb4fevxnrEs91lqKlYFBCpc7RrJEvyvlUYw61AObgp6iuUD8gN3WhEYhLCHT
> qXDhJH2U4rAvq3BCkZpGa4n8e2KYQvP/dPooILcfQQE8u21/7vdKBIQ0sz2rHteY
> ANZ4DiB3e/bxLMttk7wtCb37VLnIvlKtORXtGFbMdyCm/SVhbT5lQ3QjwXjVpp72
> B2pzU5839QDU+b511o4MEuONzr/jD7owZu+vDySJ76DDePDa8MDPD+18dMAiVKvb
> X4h5dok8JAlrnKrtjKH2HzfDKLeDcRqSJ7cFJKRpg2jiKD5c4XdA3x0m94XJb5FJ
> LwEcSMBjin6qa+Z9wKfozacv7VbuaYqLMwNTAHtjdKqBmEldGtv62oQzo9/pCo83
> B7rjEoBmrkRB7IJi049m44JLlUXxsna9FOjke1Rcwx68QeGOrqZZ8b9+cG2uVogH
> 5CNMl+/Ja17mNPrN3vZNwaJIH8/aw52+ufQ5mBNrBpRAwjh/C6gQHjSiBK2zQdeM
> E3mtZn387O3N1gErAtCp0uBglm7fcF13Pnvi6GQdHuqQ+nryjxR0GRWIN0lTsnEs
> FNfn0SK5Xb2fisrVjfKgYbGdyFg+Wo2zhOWdEC0XWg/DA4ypji6revV2rlrfUDnk
> s1CoOxBAsa+0vS7Ujjb+t71KxL15ucRFLjdT3SGOfDe3uLYGcFn7DW1RxfeRHhSX
> TcNe68y9j29Rn88+qSQe1xrid/wtDi4AilZa/LTd8IKNrXw1KXQ0GOdFVTjxJ7bu
> ezd5CAYpxEfnCO/j8nKqu9M2fcI=
> =WVon
> -----END PGP MESSAGE-----
> 
> Is that an Nvidia thing? Anyway, it looks like things get encrypted
> without people asking for it. I'll reply to Aniruddha's email in clear
> text, since it has made it to the list unencrypted.

I wonder if something has changed, as I have not had such problems
before. Thanks for reporting, we will look into it.

Cheers
Jon
Marc Zyngier March 26, 2018, 2:43 p.m. | #5
Hi Aniruddha,

On 26/03/18 13:02, Aniruddha Banerjee wrote:
> The kernel documentation states that the locking of the irq-chip
> registers should be handled by the irq-chip driver. In the irq-gic,
> the accesses to the irqchip are seemingly not protected and multiple
> writes to SPIs from different irq descriptors do RMW requests without
> taking the irq-chip lock. When multiple irqs call the request_irq at
> the same time, there can be a simultaneous write at the gic
> distributor, leading to a race. Acquire the gic_lock when the
> irq_type is updated.
> 
> Signed-off-by: Aniruddha Banerjee <aniruddhab@nvidia.com>
> ---
>  drivers/irqchip/irq-gic.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 4c797b43614d..61380f5a2254 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -67,6 +67,8 @@ static void gic_check_cpu_features(void)
>  #define gic_check_cpu_features()	do { } while(0)
>  #endif
>  
> +static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> +
>  union gic_base {
>  	void __iomem *common_base;
>  	void __percpu * __iomem *percpu_base;
> @@ -529,6 +531,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  {
>  	void __iomem *base = gic_dist_base(d);
>  	unsigned int gicirq = gic_irq(d);
> +	int ret;
>  
>  	/* Interrupt configuration for SGIs can't be changed */
>  	if (gicirq < 16)
> @@ -539,7 +542,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  			    type != IRQ_TYPE_EDGE_RISING)
>  		return -EINVAL;
>  
> -	return gic_configure_irq(gicirq, type, base, NULL);
> +	raw_spin_lock(&irq_controller_lock);
> +	ret = gic_configure_irq(gicirq, type, base, NULL);
> +	raw_spin_unlock(&irq_controller_lock);
> +
> +	return ret;
>  }
>  
>  static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
> 

You do have a good point here, but it'd be better to put it directly in
the gic_configure_irq() function, fixing both GICv1/2 and v3 in one go.
That would allow the sync_access method to be run outside of the
critical section.

Please post it to LKML and add a Cc stable for it.

Thanks,

	M.

Patch

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4c797b43614d..61380f5a2254 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -67,6 +67,8 @@  static void gic_check_cpu_features(void)
 #define gic_check_cpu_features()	do { } while(0)
 #endif
 
+static DEFINE_RAW_SPINLOCK(irq_controller_lock);
+
 union gic_base {
 	void __iomem *common_base;
 	void __percpu * __iomem *percpu_base;
@@ -529,6 +531,7 @@  static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	void __iomem *base = gic_dist_base(d);
 	unsigned int gicirq = gic_irq(d);
+	int ret;
 
 	/* Interrupt configuration for SGIs can't be changed */
 	if (gicirq < 16)
@@ -539,7 +542,11 @@  static int gic_set_type(struct irq_data *d, unsigned int type)
 			    type != IRQ_TYPE_EDGE_RISING)
 		return -EINVAL;
 
-	return gic_configure_irq(gicirq, type, base, NULL);
+	raw_spin_lock(&irq_controller_lock);
+	ret = gic_configure_irq(gicirq, type, base, NULL);
+	raw_spin_unlock(&irq_controller_lock);
+
+	return ret;
 }
 
 static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)