[-next] spi: Fix double IDR allocation with DT aliases
diff mbox series

Message ID 20180821095303.27664-1-geert+renesas@glider.be
State Accepted
Commit 04b2d03a75652bda989de1595048f0501dc0c0a0
Headers show
Series
  • [-next] spi: Fix double IDR allocation with DT aliases
Related show

Commit Message

Geert Uytterhoeven Aug. 21, 2018, 9:53 a.m. UTC
If the SPI bus number is provided by a DT alias, idr_alloc() is called
twice, leading to:

    WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
    couldn't get idr

Fix this by moving the handling of fixed SPI bus numbers up, before the
DT handling code fills in ctlr->bus_num.

Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
---
 drivers/spi/spi.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Greg KH Aug. 21, 2018, 1:40 p.m. UTC | #1
On Tue, Aug 21, 2018 at 11:53:03AM +0200, Geert Uytterhoeven wrote:
> If the SPI bus number is provided by a DT alias, idr_alloc() is called
> twice, leading to:
> 
>     WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
>     couldn't get idr
> 
> Fix this by moving the handling of fixed SPI bus numbers up, before the
> DT handling code fills in ctlr->bus_num.
> 
> Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
> ---
>  drivers/spi/spi.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>
Geert Uytterhoeven Aug. 21, 2018, 5:42 p.m. UTC | #2
Hi Greg,

On Tue, Aug 21, 2018 at 3:40 PM Greg KH <greg@kroah.com> wrote:
> On Tue, Aug 21, 2018 at 11:53:03AM +0200, Geert Uytterhoeven wrote:
> > If the SPI bus number is provided by a DT alias, idr_alloc() is called
> > twice, leading to:
> >
> >     WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
> >     couldn't get idr
> >
> > Fix this by moving the handling of fixed SPI bus numbers up, before the
> > DT handling code fills in ctlr->bus_num.
> >
> > Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
> > ---
> >  drivers/spi/spi.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> >
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>

I know.

I only CCed stable because the acceptance email for the original patch was
CCed to stable, and I wanted to prevent that one from being backported early.

Gr{oetje,eeting}s,

                        Geert
Kirill kapranov Aug. 22, 2018, 5:51 p.m. UTC | #3
Hi Geert

Thank you for keeping me informed.

I have to point at the following threat: a dynamically allocated ID may 
'squat' a bus ID that intended for a device with statically allocated 
ID. This scenario is possible since module loading order is uncertain.
This threat seems to be inevitable...

--
Best regards,
Kirill.


On 08/21/2018 12:53 PM, Geert Uytterhoeven wrote:
> If the SPI bus number is provided by a DT alias, idr_alloc() is called
> twice, leading to:
> 
>      WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
>      couldn't get idr
> 
> Fix this by moving the handling of fixed SPI bus numbers up, before the
> DT handling code fills in ctlr->bus_num.
> 
> Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Seen on e.g. r8a7791/koelsch, breaking both RSPI and MSIOF.
> ---
>   drivers/spi/spi.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index a00d006d4c3a1c5a..9da0bc5a036cfff6 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2143,8 +2143,17 @@ int spi_register_controller(struct spi_controller *ctlr)
>   	 */
>   	if (ctlr->num_chipselect == 0)
>   		return -EINVAL;
> -	/* allocate dynamic bus number using Linux idr */
> -	if ((ctlr->bus_num < 0) && ctlr->dev.of_node) {
> +	if (ctlr->bus_num >= 0) {
> +		/* devices with a fixed bus num must check-in with the num */
> +		mutex_lock(&board_lock);
> +		id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
> +			ctlr->bus_num + 1, GFP_KERNEL);
> +		mutex_unlock(&board_lock);
> +		if (WARN(id < 0, "couldn't get idr"))
> +			return id == -ENOSPC ? -EBUSY : id;
> +		ctlr->bus_num = id;
> +	} else if (ctlr->dev.of_node) {
> +		/* allocate dynamic bus number using Linux idr */
>   		id = of_alias_get_id(ctlr->dev.of_node, "spi");
>   		if (id >= 0) {
>   			ctlr->bus_num = id;
> @@ -2170,15 +2179,6 @@ int spi_register_controller(struct spi_controller *ctlr)
>   		if (WARN(id < 0, "couldn't get idr"))
>   			return id;
>   		ctlr->bus_num = id;
> -	} else {
> -		/* devices with a fixed bus num must check-in with the num */
> -		mutex_lock(&board_lock);
> -		id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
> -			ctlr->bus_num + 1, GFP_KERNEL);
> -		mutex_unlock(&board_lock);
> -		if (WARN(id < 0, "couldn't get idr"))
> -			return id == -ENOSPC ? -EBUSY : id;
> -		ctlr->bus_num = id;
>   	}
>   	INIT_LIST_HEAD(&ctlr->queue);
>   	spin_lock_init(&ctlr->queue_lock);
>
Mark Brown Aug. 23, 2018, 10:21 a.m. UTC | #4
On Wed, Aug 22, 2018 at 08:51:40PM +0300, Kirill Kapranov wrote:

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> I have to point at the following threat: a dynamically allocated ID may
> 'squat' a bus ID that intended for a device with statically allocated ID.
> This scenario is possible since module loading order is uncertain.
> This threat seems to be inevitable...

For DT systems the dynamically allocated IDs start at the maximum
positive ID and work down so in practice it is vanishingly unlikely that
there will be a collision as idiomatic static DT IDs would be low
integers.
Kirill kapranov Aug. 25, 2018, 5:54 p.m. UTC | #5
> For DT systems the dynamically allocated IDs start at the maximum
> positive ID and work down so in practice it is vanishingly unlikely that
> there will be a collision as idiomatic static DT IDs would be low
> integers.

Yes, this algorithm seems really bullet-proof. However, it isn't 
actually used now. The ID allocation algorithm using  atomic_dec_return 
call had been introduced 2006-01-08 as [1]. It was being used in the 
mainline kernel (with some improvements) up to 2017-08-16, when it has 
been replaced with the newer algorithm using Linux idr, accordingly [2].

Since idr_alloc call works incrementally, the situation of a 'fixed' ID 
squatting by a driver with 'dynamic ID' seems more than possible.
Therefore it would be justified to use a hardcoded constant 
SPI_DYN_FIRST_BUS_NUM (that was introduced in [2] and eliminated in 
[3]), but with a sufficiently greater value of the constant.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.18&id=8ae12a0d85987dc138f8c944cb78a92bf466cea0 

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.18&id=9b61e302210eba55768962f2f11e96bb508c2408
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.18&id=42bdd7061a6e24d7b21d3d21973615fecef544ef
Mark Brown Aug. 26, 2018, 1:24 p.m. UTC | #6
On Sat, Aug 25, 2018 at 08:54:53PM +0300, Kirill Kapranov wrote:
> > For DT systems the dynamically allocated IDs start at the maximum
> > positive ID and work down so in practice it is vanishingly unlikely that
> > there will be a collision as idiomatic static DT IDs would be low
> > integers.
> 
> Yes, this algorithm seems really bullet-proof. However, it isn't actually
> used now. The ID allocation algorithm using  atomic_dec_return call had been
> introduced 2006-01-08 as [1]. It was being used in the mainline kernel (with
> some improvements) up to 2017-08-16, when it has been replaced with the
> newer algorithm using Linux idr, accordingly [2].

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

> Since idr_alloc call works incrementally, the situation of a 'fixed' ID
> squatting by a driver with 'dynamic ID' seems more than possible.
> Therefore it would be justified to use a hardcoded constant
> SPI_DYN_FIRST_BUS_NUM (that was introduced in [2] and eliminated in [3]),
> but with a sufficiently greater value of the constant.

Right, that clearly wasn't an intended effect, though - should be using
the max of the big constant and the maximum static ID.
Fabio Estevam Aug. 27, 2018, 2:13 p.m. UTC | #7
Hi Geert,

On Tue, Aug 21, 2018 at 6:53 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> If the SPI bus number is provided by a DT alias, idr_alloc() is called
> twice, leading to:
>
>     WARNING: CPU: 1 PID: 1 at drivers/spi/spi.c:2179 spi_register_controller+0x11c/0x5d8
>     couldn't get idr
>
> Fix this by moving the handling of fixed SPI bus numbers up, before the
> DT handling code fills in ctlr->bus_num.
>
> Fixes: 1a4327fbf4554d5b ("spi: fix IDR collision on systems with both fixed and dynamic SPI bus numbers")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

This fixes SPI on imx51-babbage, thanks.

Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
Kirill kapranov Aug. 28, 2018, 7:47 p.m. UTC | #8
> Right, that clearly wasn't an intended effect, though - should be using
> the max of the big constant and the maximum static ID.

Due to difficulties of enumeration of all device in a system, I propose
to assign safe sub-range for dynamic IDs in the upper-half of ID range the following way.

NOTE-0: This patch has to be applied after Geert's patch,
which fixes double call of idr_alloc. (above in the thread). Many thanks, Geert!

NOTE-1: The best solution, IMHO, would be migration to property_get_*
functions, declared in linux/property.h 

[PATCH] Eliminate possibility of conflict between static and dynamic IDs

For systems without DT support allocate dynamical bus ID in a safe sub-range
(if given), otherwise in upper half of the range so as to avoid possible
conflict between statically and dynamically allocated IDs.

Signed-off-by: Kirill Kapranov <kirill.kapranov@compulab.co.il>
---
 drivers/spi/Kconfig |  8 ++++++++
 drivers/spi/spi.c   | 22 +++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 671d078349cc..7498fae0113b 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -47,6 +47,14 @@ config SPI_MASTER
 
 if SPI_MASTER
 
+config SPI_MASTER_DYN_BUS_NUM_FIRST
+	int "First dynamically assigned SPI bus ID" if EXPERT
+	default 0
+	help
+	  This value can be used as the beginning of sub-range for dynamic
+	  allocation of SPI bus ID in case of absence of DT. If -1 chosen, the
+	  sub-range will be allocated in upper half of the SPI bus ID range.
+
 config SPI_MEM
 	bool "SPI memory extension"
 	help
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9da0bc5a036c..3ac0cf0ab49c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -49,6 +49,18 @@
 
 #include "internals.h"
 
+#ifndef CHAR_BIT
+#define CHAR_BIT 8	/* Normally in <limits.h> */
+#endif
+#if !defined(CONFIG_SPI_MASTER_DYN_BUS_NUM_FIRST) || \
+	CONFIG_SPI_MASTER_DYN_BUS_NUM_FIRST < 0
+//Half of the biggest signed int of this size
+#define SPI_DYN_FIRST_NUM (((1 << \
+	(sizeof(((struct spi_controller*)0)->bus_num) * CHAR_BIT - 1)) - 1) / 2)
+#else
+#define SPI_DYN_FIRST_NUM CONFIG_SPI_MASTER_DYN_BUS_NUM_FIRST
+#endif
+
 static DEFINE_IDR(spi_master_idr);
 
 static void spidev_release(struct device *dev)
@@ -2167,7 +2179,15 @@ int spi_register_controller(struct spi_controller *ctlr)
 	}
 	if (ctlr->bus_num < 0) {
 		first_dynamic = of_alias_get_highest_id("spi");
-		if (first_dynamic < 0)
+
+		if (first_dynamic == -ENOSYS)
+			/*
+			 * In case of system without DT support, allocate
+			 * dynamic bus ID in safe range, higher than the bound,
+			 * to avoid conflict between static and dynamic ID
+			 */
+			first_dynamic = SPI_DYN_FIRST_NUM;
+		else if (first_dynamic < 0)
 			first_dynamic = 0;
 		else
 			first_dynamic++;

Patch
diff mbox series

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a00d006d4c3a1c5a..9da0bc5a036cfff6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2143,8 +2143,17 @@  int spi_register_controller(struct spi_controller *ctlr)
 	 */
 	if (ctlr->num_chipselect == 0)
 		return -EINVAL;
-	/* allocate dynamic bus number using Linux idr */
-	if ((ctlr->bus_num < 0) && ctlr->dev.of_node) {
+	if (ctlr->bus_num >= 0) {
+		/* devices with a fixed bus num must check-in with the num */
+		mutex_lock(&board_lock);
+		id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
+			ctlr->bus_num + 1, GFP_KERNEL);
+		mutex_unlock(&board_lock);
+		if (WARN(id < 0, "couldn't get idr"))
+			return id == -ENOSPC ? -EBUSY : id;
+		ctlr->bus_num = id;
+	} else if (ctlr->dev.of_node) {
+		/* allocate dynamic bus number using Linux idr */
 		id = of_alias_get_id(ctlr->dev.of_node, "spi");
 		if (id >= 0) {
 			ctlr->bus_num = id;
@@ -2170,15 +2179,6 @@  int spi_register_controller(struct spi_controller *ctlr)
 		if (WARN(id < 0, "couldn't get idr"))
 			return id;
 		ctlr->bus_num = id;
-	} else {
-		/* devices with a fixed bus num must check-in with the num */
-		mutex_lock(&board_lock);
-		id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
-			ctlr->bus_num + 1, GFP_KERNEL);
-		mutex_unlock(&board_lock);
-		if (WARN(id < 0, "couldn't get idr"))
-			return id == -ENOSPC ? -EBUSY : id;
-		ctlr->bus_num = id;
 	}
 	INIT_LIST_HEAD(&ctlr->queue);
 	spin_lock_init(&ctlr->queue_lock);