[PATCH -next] spi: Fix double IDR allocation with DT aliases

[PATCH -next] spi: Fix double IDR allocation with DT aliases

[Geert Uytterhoeven]

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);
-- 
2.17.1

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Greg KH]

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>

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Kirill Kapranov]

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);
> 

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Kirill Kapranov]

> 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

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Fabio Estevam]

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>

Re: [PATCH -next] spi: Fix double IDR allocation with DT aliases

[Kirill kapranov]

> 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++;
-- 
2.11.0

Applied "spi: Fix double IDR allocation with DT aliases" to the spi tree

[Mark Brown]

The patch

   spi: Fix double IDR allocation with DT aliases

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 04b2d03a75652bda989de1595048f0501dc0c0a0 Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <geert+renesas@glider.be>
Date: Tue, 21 Aug 2018 11:53:03 +0200
Subject: [PATCH] spi: Fix double IDR allocation with DT aliases

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>
Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 a00d006d4c3a..9da0bc5a036c 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);
-- 
2.18.0