linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] VLA removal from the GPIO subsystem
@ 2018-03-10  0:10 Laura Abbott
  2018-03-10  0:10 ` [PATCH 1/4] gpio: Remove VLA from gpiolib Laura Abbott
                   ` (4 more replies)
  0 siblings, 5 replies; 26+ messages in thread
From: Laura Abbott @ 2018-03-10  0:10 UTC (permalink / raw)
  To: Linus Walleij, Kees Cook, Lukas Wunner, Nandor Han, Semi Malinen,
	Patrice Chotard
  Cc: Laura Abbott, linux-gpio, linux-kernel, kernel-hardening,
	Mathias Duckeck

Hi,

For those who haven't seen it, there's an effort to remove VLAs from the
kernel so we can turn on -Wvla in the name of security. See
https://lkml.org/lkml/2018/3/7/621 for more details and discussion.

This is a series to remove a few VLAs from the gpio subsystem. These are
compile tested only so I'd appreciate reviews and tests from
maintainers. When these get Acked, I expect these to just go through the
GPIO tree like usual.

Thanks,
Laura

Laura Abbott (4):
  gpio: Remove VLA from gpiolib
  gpio: Remove VLA from MAX3191X driver
  gpio: Remove VLA from xra1403 driver
  gpio: Remove VLA from stmpe driver

 drivers/gpio/gpio-max3191x.c |  7 +++++-
 drivers/gpio/gpio-stmpe.c    |  7 +++++-
 drivers/gpio/gpio-xra1403.c  |  8 ++++++-
 drivers/gpio/gpiolib.c       | 55 ++++++++++++++++++++++++++++++++++++++------
 4 files changed, 67 insertions(+), 10 deletions(-)

-- 
2.14.3

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-10  0:10 [PATCH 0/4] VLA removal from the GPIO subsystem Laura Abbott
@ 2018-03-10  0:10 ` Laura Abbott
  2018-03-12 15:00   ` Rasmus Villemoes
  2018-03-10  0:10 ` [PATCH 2/4] gpio: Remove VLA from MAX3191X driver Laura Abbott
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-10  0:10 UTC (permalink / raw)
  To: Linus Walleij, Kees Cook
  Cc: Laura Abbott, linux-gpio, linux-kernel, kernel-hardening,
	Lukas Wunner, Mathias Duckeck, Nandor Han, Semi Malinen,
	Patrice Chotard



The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces several VLAs with an appropriate call to
kmalloc_array.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/gpio/gpiolib.c | 55 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d66de67ef307..124727c74931 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2662,16 +2662,33 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 
 	while (i < array_size) {
 		struct gpio_chip *chip = desc_array[i]->gdev->chip;
-		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+		unsigned long *mask;
+		unsigned long *bits;
 		int first, j, ret;
 
+		mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
+				sizeof(*mask),
+				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+		if (!mask)
+			return -ENOMEM;
+
+		bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
+				sizeof(*bits),
+				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+		if (!bits) {
+			kfree(mask);
+			return -ENOMEM;
+		}
+
+
 		if (!can_sleep)
 			WARN_ON(chip->can_sleep);
 
 		/* collect all inputs belonging to the same chip */
 		first = i;
-		memset(mask, 0, sizeof(mask));
+		memset(mask, 0, sizeof(*mask));
 		do {
 			const struct gpio_desc *desc = desc_array[i];
 			int hwgpio = gpio_chip_hwgpio(desc);
@@ -2682,8 +2699,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			 (desc_array[i]->gdev->chip == chip));
 
 		ret = gpio_chip_get_multiple(chip, mask, bits);
-		if (ret)
+		if (ret) {
+			kfree(bits);
+			kfree(mask);
 			return ret;
+		}
 
 		for (j = first; j < i; j++) {
 			const struct gpio_desc *desc = desc_array[j];
@@ -2695,6 +2715,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			value_array[j] = value;
 			trace_gpio_value(desc_to_gpio(desc), 1, value);
 		}
+		kfree(bits);
+		kfree(mask);
 	}
 	return 0;
 }
@@ -2887,14 +2909,30 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
 
 	while (i < array_size) {
 		struct gpio_chip *chip = desc_array[i]->gdev->chip;
-		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+		unsigned long *mask;
+		unsigned long *bits;
 		int count = 0;
 
+		mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
+				sizeof(*mask),
+				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+		if (!mask)
+			return;
+
+		bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
+				sizeof(*bits),
+				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+		if (!bits) {
+			kfree(mask);
+			return;
+		}
+
 		if (!can_sleep)
 			WARN_ON(chip->can_sleep);
 
-		memset(mask, 0, sizeof(mask));
+		memset(mask, 0, sizeof(*mask));
 		do {
 			struct gpio_desc *desc = desc_array[i];
 			int hwgpio = gpio_chip_hwgpio(desc);
@@ -2925,6 +2963,9 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
 		/* push collected bits to outputs */
 		if (count != 0)
 			gpio_chip_set_multiple(chip, mask, bits);
+
+		kfree(mask);
+		kfree(bits);
 	}
 }
 
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/4] gpio: Remove VLA from MAX3191X driver
  2018-03-10  0:10 [PATCH 0/4] VLA removal from the GPIO subsystem Laura Abbott
  2018-03-10  0:10 ` [PATCH 1/4] gpio: Remove VLA from gpiolib Laura Abbott
@ 2018-03-10  0:10 ` Laura Abbott
  2018-03-26  9:07   ` Linus Walleij
  2018-03-10  0:10 ` [PATCH 3/4] gpio: Remove VLA from xra1403 driver Laura Abbott
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-10  0:10 UTC (permalink / raw)
  To: Linus Walleij, Kees Cook, Lukas Wunner
  Cc: Laura Abbott, linux-gpio, linux-kernel, kernel-hardening,
	Mathias Duckeck


The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces several a VLA with an appropriate call to
kmalloc_array.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/gpio/gpio-max3191x.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-max3191x.c b/drivers/gpio/gpio-max3191x.c
index f74b1072e84b..b5b9cb1fda50 100644
--- a/drivers/gpio/gpio-max3191x.c
+++ b/drivers/gpio/gpio-max3191x.c
@@ -315,12 +315,17 @@ static void gpiod_set_array_single_value_cansleep(unsigned int ndescs,
 						  struct gpio_desc **desc,
 						  int value)
 {
-	int i, values[ndescs];
+	int i, *values;
+
+	values = kmalloc_array(ndescs, sizeof(*values), GFP_KERNEL);
+	if (!values)
+		return;
 
 	for (i = 0; i < ndescs; i++)
 		values[i] = value;
 
 	gpiod_set_array_value_cansleep(ndescs, desc, values);
+	kfree(values);
 }
 
 static struct gpio_descs *devm_gpiod_get_array_optional_count(
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/4] gpio: Remove VLA from xra1403 driver
  2018-03-10  0:10 [PATCH 0/4] VLA removal from the GPIO subsystem Laura Abbott
  2018-03-10  0:10 ` [PATCH 1/4] gpio: Remove VLA from gpiolib Laura Abbott
  2018-03-10  0:10 ` [PATCH 2/4] gpio: Remove VLA from MAX3191X driver Laura Abbott
@ 2018-03-10  0:10 ` Laura Abbott
  2018-03-12  6:06   ` EXT: " Nandor Han
                     ` (2 more replies)
  2018-03-10  0:10 ` [PATCH 4/4] gpio: Remove VLA from stmpe driver Laura Abbott
  2018-03-13  9:42 ` [PATCH 0/4] VLA removal from the GPIO subsystem Linus Walleij
  4 siblings, 3 replies; 26+ messages in thread
From: Laura Abbott @ 2018-03-10  0:10 UTC (permalink / raw)
  To: Linus Walleij, Kees Cook, Nandor Han, Semi Malinen
  Cc: Laura Abbott, linux-gpio, linux-kernel, kernel-hardening


The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/gpio/gpio-xra1403.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c
index 0230e4b7a2fb..8d4c8e99b251 100644
--- a/drivers/gpio/gpio-xra1403.c
+++ b/drivers/gpio/gpio-xra1403.c
@@ -126,11 +126,16 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 {
 	int reg;
 	struct xra1403 *xra = gpiochip_get_data(chip);
-	int value[xra1403_regmap_cfg.max_register];
+	int *value;
 	int i;
 	unsigned int gcr;
 	unsigned int gsr;
 
+	value = kmalloc_array(xra1403_regmap_cfg.max_register, sizeof(*value),
+				GFP_KERNEL);
+	if (!value)
+		return;
+
 	seq_puts(s, "xra reg:");
 	for (reg = 0; reg <= xra1403_regmap_cfg.max_register; reg++)
 		seq_printf(s, " %2.2x", reg);
@@ -154,6 +159,7 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 			   (gcr & BIT(i)) ? "in" : "out",
 			   (gsr & BIT(i)) ? "hi" : "lo");
 	}
+	kfree(value);
 }
 #else
 #define xra1403_dbg_show NULL
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/4] gpio: Remove VLA from stmpe driver
  2018-03-10  0:10 [PATCH 0/4] VLA removal from the GPIO subsystem Laura Abbott
                   ` (2 preceding siblings ...)
  2018-03-10  0:10 ` [PATCH 3/4] gpio: Remove VLA from xra1403 driver Laura Abbott
@ 2018-03-10  0:10 ` Laura Abbott
  2018-03-13  9:13   ` Phil Reid
  2018-03-13  9:42 ` [PATCH 0/4] VLA removal from the GPIO subsystem Linus Walleij
  4 siblings, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-10  0:10 UTC (permalink / raw)
  To: Linus Walleij, Kees Cook, Patrice Chotard
  Cc: Laura Abbott, linux-gpio, linux-kernel, kernel-hardening

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/gpio/gpio-stmpe.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..b7854850bcdb 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
 	struct stmpe *stmpe = stmpe_gpio->stmpe;
 	u8 statmsbreg;
 	int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-	u8 status[num_banks];
+	u8 *status;
 	int ret;
 	int i;
 
+	status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
+	if (!status)
+		return IRQ_NONE;
+
 	/*
 	 * the stmpe_block_read() call below, imposes to set statmsbreg
 	 * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
 		}
 	}
 
+	kfree(status);
 	return IRQ_HANDLED;
 }
 
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: EXT: [PATCH 3/4] gpio: Remove VLA from xra1403 driver
  2018-03-10  0:10 ` [PATCH 3/4] gpio: Remove VLA from xra1403 driver Laura Abbott
@ 2018-03-12  6:06   ` Nandor Han
  2018-03-26  9:09   ` Linus Walleij
  2018-03-28  7:27   ` Geert Uytterhoeven
  2 siblings, 0 replies; 26+ messages in thread
From: Nandor Han @ 2018-03-12  6:06 UTC (permalink / raw)
  To: Laura Abbott, Linus Walleij, Kees Cook, Semi Malinen
  Cc: linux-gpio, linux-kernel, kernel-hardening

On 10/03/18 02:10, Laura Abbott wrote:
> 
> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
> 
> This patch replaces a VLA with an appropriate call to kmalloc_array.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---

This looks good to me.
Reviewed-by: Nandor Han <nandor.han@ge.com>

Nandor

>   drivers/gpio/gpio-xra1403.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c
> index 0230e4b7a2fb..8d4c8e99b251 100644
> --- a/drivers/gpio/gpio-xra1403.c
> +++ b/drivers/gpio/gpio-xra1403.c
> @@ -126,11 +126,16 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>   {
>   	int reg;
>   	struct xra1403 *xra = gpiochip_get_data(chip);
> -	int value[xra1403_regmap_cfg.max_register];
> +	int *value;
>   	int i;
>   	unsigned int gcr;
>   	unsigned int gsr;
>   
> +	value = kmalloc_array(xra1403_regmap_cfg.max_register, sizeof(*value),
> +				GFP_KERNEL);
> +	if (!value)
> +		return;
> +
>   	seq_puts(s, "xra reg:");
>   	for (reg = 0; reg <= xra1403_regmap_cfg.max_register; reg++)
>   		seq_printf(s, " %2.2x", reg);
> @@ -154,6 +159,7 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>   			   (gcr & BIT(i)) ? "in" : "out",
>   			   (gsr & BIT(i)) ? "hi" : "lo");
>   	}
> +	kfree(value);
>   }
>   #else
>   #define xra1403_dbg_show NULL
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-10  0:10 ` [PATCH 1/4] gpio: Remove VLA from gpiolib Laura Abbott
@ 2018-03-12 15:00   ` Rasmus Villemoes
  2018-03-12 23:40     ` Laura Abbott
  2018-03-17  8:25     ` Lukas Wunner
  0 siblings, 2 replies; 26+ messages in thread
From: Rasmus Villemoes @ 2018-03-12 15:00 UTC (permalink / raw)
  To: Laura Abbott, Linus Walleij, Kees Cook
  Cc: linux-gpio, linux-kernel, kernel-hardening, Lukas Wunner,
	Mathias Duckeck, Nandor Han, Semi Malinen, Patrice Chotard

On 2018-03-10 01:10, Laura Abbott wrote:
>  		/* collect all inputs belonging to the same chip */
>  		first = i;
> -		memset(mask, 0, sizeof(mask));
> +		memset(mask, 0, sizeof(*mask));

see below

> @@ -2887,14 +2909,30 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
>  
>  	while (i < array_size) {
>  		struct gpio_chip *chip = desc_array[i]->gdev->chip;
> -		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
> -		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
> +		unsigned long *mask;
> +		unsigned long *bits;
>  		int count = 0;
>  
> +		mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
> +				sizeof(*mask),
> +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> +
> +		if (!mask)
> +			return;
> +
> +		bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
> +				sizeof(*bits),
> +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> +
> +		if (!bits) {
> +			kfree(mask);
> +			return;
> +		}
> +
>  		if (!can_sleep)
>  			WARN_ON(chip->can_sleep);
>  
> -		memset(mask, 0, sizeof(mask));
> +		memset(mask, 0, sizeof(*mask));

Hm, it seems you're now only clearing the first word of mask, not the
entire allocation. Why not just use kcalloc() instead of kmalloc_array
to have it automatically cleared?

Other random thoughts: maybe two allocations for each loop iteration is
a bit much. Maybe do a first pass over the array and collect the maximal
chip->ngpio, do the memory allocation and freeing outside the loop (then
you'd of course need to preserve the memset() with appropriate length
computed). And maybe even just do one allocation, making bits point at
the second half.

Does the set function need to be updated to return an int to be able to
inform the caller that memory allocation failed?

Rasmus

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-12 15:00   ` Rasmus Villemoes
@ 2018-03-12 23:40     ` Laura Abbott
  2018-03-13  7:23       ` Rasmus Villemoes
  2018-03-17  8:25     ` Lukas Wunner
  1 sibling, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-12 23:40 UTC (permalink / raw)
  To: Rasmus Villemoes, Linus Walleij, Kees Cook
  Cc: linux-gpio, linux-kernel, kernel-hardening, Lukas Wunner,
	Mathias Duckeck, Nandor Han, Semi Malinen, Patrice Chotard

On 03/12/2018 08:00 AM, Rasmus Villemoes wrote:
> On 2018-03-10 01:10, Laura Abbott wrote:
>>   		/* collect all inputs belonging to the same chip */
>>   		first = i;
>> -		memset(mask, 0, sizeof(mask));
>> +		memset(mask, 0, sizeof(*mask));
> 
> see below
> 
>> @@ -2887,14 +2909,30 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
>>   
>>   	while (i < array_size) {
>>   		struct gpio_chip *chip = desc_array[i]->gdev->chip;
>> -		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
>> -		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
>> +		unsigned long *mask;
>> +		unsigned long *bits;
>>   		int count = 0;
>>   
>> +		mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
>> +				sizeof(*mask),
>> +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
>> +
>> +		if (!mask)
>> +			return;
>> +
>> +		bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
>> +				sizeof(*bits),
>> +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
>> +
>> +		if (!bits) {
>> +			kfree(mask);
>> +			return;
>> +		}
>> +
>>   		if (!can_sleep)
>>   			WARN_ON(chip->can_sleep);
>>   
>> -		memset(mask, 0, sizeof(mask));
>> +		memset(mask, 0, sizeof(*mask));
> 
> Hm, it seems you're now only clearing the first word of mask, not the
> entire allocation. Why not just use kcalloc() instead of kmalloc_array
> to have it automatically cleared?
> 

Bleh, I didn't think through that carefully. I'll just switch
to kcalloc, especially since it calls kmalloc_array.

> Other random thoughts: maybe two allocations for each loop iteration is
> a bit much. Maybe do a first pass over the array and collect the maximal
> chip->ngpio, do the memory allocation and freeing outside the loop (then
> you'd of course need to preserve the memset() with appropriate length
> computed). And maybe even just do one allocation, making bits point at
> the second half.
> 

I was trying to make minimal changes and match the existing code. Is this
likely to be an actual hot path to optimize?

> Does the set function need to be updated to return an int to be able to
> inform the caller that memory allocation failed?
> 

That would involve changing the public API. I don't have a problem
doing so if that's what you want.

> Rasmus
> 

Thanks,
Laura

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-12 23:40     ` Laura Abbott
@ 2018-03-13  7:23       ` Rasmus Villemoes
  0 siblings, 0 replies; 26+ messages in thread
From: Rasmus Villemoes @ 2018-03-13  7:23 UTC (permalink / raw)
  To: Laura Abbott, Rasmus Villemoes, Linus Walleij, Kees Cook
  Cc: linux-gpio, linux-kernel, kernel-hardening, Lukas Wunner,
	Mathias Duckeck, Nandor Han, Semi Malinen, Patrice Chotard

On 2018-03-13 00:40, Laura Abbott wrote:
> On 03/12/2018 08:00 AM, Rasmus Villemoes wrote:
>>
>> Hm, it seems you're now only clearing the first word of mask, not the
>> entire allocation. Why not just use kcalloc() instead of kmalloc_array
>> to have it automatically cleared?
> 
> Bleh, I didn't think through that carefully. I'll just switch
> to kcalloc, especially since it calls kmalloc_array.
> 
>> Other random thoughts: maybe two allocations for each loop iteration is
>> a bit much. Maybe do a first pass over the array and collect the maximal
>> chip->ngpio, do the memory allocation and freeing outside the loop (then
>> you'd of course need to preserve the memset() with appropriate length
>> computed). And maybe even just do one allocation, making bits point at
>> the second half.
>>
> 
> I was trying to make minimal changes and match the existing code.

Well, textually you may be making the minimal changes (though the error
handling adds some "boilerplate" kfree()s etc.), but semantically adding
two kmallocs in a loop could be a big deal. Dunno, maybe in practice all
the gpios (almost always) belong to the same chip, so there's only one
iteration through the loop anyway.

> Is this likely to be an actual hot path to optimize?

No idea, it was just a drive-by comment, so also...

>> Does the set function need to be updated to return an int to be able to
>> inform the caller that memory allocation failed?
> 
> That would involve changing the public API. I don't have a problem
> doing so if that's what you want.

... I don't "want" anything, that'll be for the gpio maintainers to decide.

Rasmus

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver
  2018-03-10  0:10 ` [PATCH 4/4] gpio: Remove VLA from stmpe driver Laura Abbott
@ 2018-03-13  9:13   ` Phil Reid
  2018-03-14  0:18     ` Laura Abbott
  0 siblings, 1 reply; 26+ messages in thread
From: Phil Reid @ 2018-03-13  9:13 UTC (permalink / raw)
  To: Laura Abbott, Linus Walleij, Kees Cook, Patrice Chotard
  Cc: linux-gpio, linux-kernel, kernel-hardening

On 10/03/2018 08:10, Laura Abbott wrote:
> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
> 
> This patch replaces a VLA with an appropriate call to kmalloc_array.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
> index f8d7d1cd8488..b7854850bcdb 100644
> --- a/drivers/gpio/gpio-stmpe.c
> +++ b/drivers/gpio/gpio-stmpe.c
> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>   	struct stmpe *stmpe = stmpe_gpio->stmpe;
>   	u8 statmsbreg;
>   	int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
> -	u8 status[num_banks];
> +	u8 *status;
>   	int ret;
>   	int i;
>   
> +	status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
> +	if (!status)
> +		return IRQ_NONE;
> +
>   	/*
>   	 * the stmpe_block_read() call below, imposes to set statmsbreg
>   	 * with the register located at the lowest address. As STMPE1600
> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>   		}
>   	}
>   
> +	kfree(status);
>   	return IRQ_HANDLED;
>   }
>   
> 

Doing this in an irq handler seems wrong.
Perhaps better if a buffer is pre-allocated in stmpe_gpio


-- 
Regards
Phil Reid

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/4] VLA removal from the GPIO subsystem
  2018-03-10  0:10 [PATCH 0/4] VLA removal from the GPIO subsystem Laura Abbott
                   ` (3 preceding siblings ...)
  2018-03-10  0:10 ` [PATCH 4/4] gpio: Remove VLA from stmpe driver Laura Abbott
@ 2018-03-13  9:42 ` Linus Walleij
  4 siblings, 0 replies; 26+ messages in thread
From: Linus Walleij @ 2018-03-13  9:42 UTC (permalink / raw)
  To: Laura Abbott
  Cc: Kees Cook, Lukas Wunner, Nandor Han, Semi Malinen,
	Patrice Chotard, open list:GPIO SUBSYSTEM, linux-kernel,
	kernel-hardening, Mathias Duckeck

On Sat, Mar 10, 2018 at 1:10 AM, Laura Abbott <labbott@redhat.com> wrote:

> For those who haven't seen it, there's an effort to remove VLAs from the
> kernel so we can turn on -Wvla in the name of security. See
> https://lkml.org/lkml/2018/3/7/621 for more details and discussion.

OK I read up on it, I'm on board! Less is more.

> This is a series to remove a few VLAs from the gpio subsystem. These are
> compile tested only so I'd appreciate reviews and tests from
> maintainers. When these get Acked, I expect these to just go through the
> GPIO tree like usual.

Sure I will just queue it up with ACKs, or if I just think it looks nice
and reliable.

Thank you for doing this!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver
  2018-03-13  9:13   ` Phil Reid
@ 2018-03-14  0:18     ` Laura Abbott
  2018-03-14  1:16       ` Laura Abbott
  0 siblings, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-14  0:18 UTC (permalink / raw)
  To: Phil Reid, Linus Walleij, Kees Cook, Patrice Chotard
  Cc: linux-gpio, linux-kernel, kernel-hardening

On 03/13/2018 02:13 AM, Phil Reid wrote:
> On 10/03/2018 08:10, Laura Abbott wrote:
>> The new challenge is to remove VLAs from the kernel
>> (see https://lkml.org/lkml/2018/3/7/621)
>>
>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>> index f8d7d1cd8488..b7854850bcdb 100644
>> --- a/drivers/gpio/gpio-stmpe.c
>> +++ b/drivers/gpio/gpio-stmpe.c
>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>       struct stmpe *stmpe = stmpe_gpio->stmpe;
>>       u8 statmsbreg;
>>       int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>> -    u8 status[num_banks];
>> +    u8 *status;
>>       int ret;
>>       int i;
>> +    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
>> +    if (!status)
>> +        return IRQ_NONE;
>> +
>>       /*
>>        * the stmpe_block_read() call below, imposes to set statmsbreg
>>        * with the register located at the lowest address. As STMPE1600
>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>           }
>>       }
>> +    kfree(status);
>>       return IRQ_HANDLED;
>>   }
>>
> 
> Doing this in an irq handler seems wrong.
> Perhaps better if a buffer is pre-allocated in stmpe_gpio
> 
> 

Sure, I can pre-allocate the buffer in the probe.

Thanks,
Laura

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver
  2018-03-14  0:18     ` Laura Abbott
@ 2018-03-14  1:16       ` Laura Abbott
  2018-03-14  2:55         ` Phil Reid
  0 siblings, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-14  1:16 UTC (permalink / raw)
  To: Phil Reid, Linus Walleij, Kees Cook, Patrice Chotard
  Cc: linux-gpio, linux-kernel, kernel-hardening

On 03/13/2018 05:18 PM, Laura Abbott wrote:
> On 03/13/2018 02:13 AM, Phil Reid wrote:
>> On 10/03/2018 08:10, Laura Abbott wrote:
>>> The new challenge is to remove VLAs from the kernel
>>> (see https://lkml.org/lkml/2018/3/7/621)
>>>
>>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>>> index f8d7d1cd8488..b7854850bcdb 100644
>>> --- a/drivers/gpio/gpio-stmpe.c
>>> +++ b/drivers/gpio/gpio-stmpe.c
>>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>       struct stmpe *stmpe = stmpe_gpio->stmpe;
>>>       u8 statmsbreg;
>>>       int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>>> -    u8 status[num_banks];
>>> +    u8 *status;
>>>       int ret;
>>>       int i;
>>> +    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
>>> +    if (!status)
>>> +        return IRQ_NONE;
>>> +
>>>       /*
>>>        * the stmpe_block_read() call below, imposes to set statmsbreg
>>>        * with the register located at the lowest address. As STMPE1600
>>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>           }
>>>       }
>>> +    kfree(status);
>>>       return IRQ_HANDLED;
>>>   }
>>>
>>
>> Doing this in an irq handler seems wrong.
>> Perhaps better if a buffer is pre-allocated in stmpe_gpio
>>
>>
> 
> Sure, I can pre-allocate the buffer in the probe.
> 
> Thanks,
> Laura

Actually I wonder if there would be concurrency problems if we
tried to pre-allocate a global buffer. But the IRQ handler
calls stmpe_block_read which takes a mutex to sleep so I think
doing the (small) kmalloc should be fine and I can change it to
a GFP_KERNEL.

Thanks,
Laura

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver
  2018-03-14  1:16       ` Laura Abbott
@ 2018-03-14  2:55         ` Phil Reid
  0 siblings, 0 replies; 26+ messages in thread
From: Phil Reid @ 2018-03-14  2:55 UTC (permalink / raw)
  To: Laura Abbott, Linus Walleij, Kees Cook, Patrice Chotard
  Cc: linux-gpio, linux-kernel, kernel-hardening

On 14/03/2018 09:16, Laura Abbott wrote:
> On 03/13/2018 05:18 PM, Laura Abbott wrote:
>> On 03/13/2018 02:13 AM, Phil Reid wrote:
>>> On 10/03/2018 08:10, Laura Abbott wrote:
>>>> The new challenge is to remove VLAs from the kernel
>>>> (see https://lkml.org/lkml/2018/3/7/621)
>>>>
>>>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>>>
>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>> ---
>>>>   drivers/gpio/gpio-stmpe.c | 7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>>>> index f8d7d1cd8488..b7854850bcdb 100644
>>>> --- a/drivers/gpio/gpio-stmpe.c
>>>> +++ b/drivers/gpio/gpio-stmpe.c
>>>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>>       struct stmpe *stmpe = stmpe_gpio->stmpe;
>>>>       u8 statmsbreg;
>>>>       int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>>>> -    u8 status[num_banks];
>>>> +    u8 *status;
>>>>       int ret;
>>>>       int i;
>>>> +    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
>>>> +    if (!status)
>>>> +        return IRQ_NONE;
>>>> +
>>>>       /*
>>>>        * the stmpe_block_read() call below, imposes to set statmsbreg
>>>>        * with the register located at the lowest address. As STMPE1600
>>>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>>>           }
>>>>       }
>>>> +    kfree(status);
>>>>       return IRQ_HANDLED;
>>>>   }
>>>>
>>>
>>> Doing this in an irq handler seems wrong.
>>> Perhaps better if a buffer is pre-allocated in stmpe_gpio
>>
>> Sure, I can pre-allocate the buffer in the probe.
> 
> Actually I wonder if there would be concurrency problems if we
> tried to pre-allocate a global buffer. But the IRQ handler
> calls stmpe_block_read which takes a mutex to sleep so I think
> doing the (small) kmalloc should be fine and I can change it to
> a GFP_KERNEL.
> 
I'm no expert on this driver, but I wouldn't think there'd be any concurrency
problem if the buffer is only used by the irq handler.
It should never get called concurrently for the same device.

As to the impact, I'll admit I've really got no idea how much potential overhead a
kmalloc has compared to a mutex for the linux kernel.
Just a general rule of thumb, that memory allocations are usually expensive.
-- 
Regards
Phil Reid

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-12 15:00   ` Rasmus Villemoes
  2018-03-12 23:40     ` Laura Abbott
@ 2018-03-17  8:25     ` Lukas Wunner
  2018-03-18 14:23       ` Lukas Wunner
  1 sibling, 1 reply; 26+ messages in thread
From: Lukas Wunner @ 2018-03-17  8:25 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Laura Abbott, Linus Walleij, Kees Cook, linux-gpio, linux-kernel,
	kernel-hardening, Mathias Duckeck, Nandor Han, Semi Malinen,
	Patrice Chotard

On Mon, Mar 12, 2018 at 04:00:36PM +0100, Rasmus Villemoes wrote:
> On 2018-03-10 01:10, Laura Abbott wrote:
> > @@ -2887,14 +2909,30 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
> >  
> >  	while (i < array_size) {
> >  		struct gpio_chip *chip = desc_array[i]->gdev->chip;
> > -		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
> > -		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
> > +		unsigned long *mask;
> > +		unsigned long *bits;
> >  		int count = 0;
> >  
> > +		mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
> > +				sizeof(*mask),
> > +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> > +
> > +		if (!mask)
> > +			return;
> > +
> > +		bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
> > +				sizeof(*bits),
> > +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> > +
> > +		if (!bits) {
> > +			kfree(mask);
> > +			return;
> > +		}
> > +
> >  		if (!can_sleep)
> >  			WARN_ON(chip->can_sleep);
> >  
> > -		memset(mask, 0, sizeof(mask));
> > +		memset(mask, 0, sizeof(*mask));
> 
> Other random thoughts: maybe two allocations for each loop iteration is
> a bit much. Maybe do a first pass over the array and collect the maximal
> chip->ngpio, do the memory allocation and freeing outside the loop (then
> you'd of course need to preserve the memset() with appropriate length
> computed). And maybe even just do one allocation, making bits point at
> the second half.

I think those are great ideas because the function is kind of a hotpath
and usage of VLAs was motivated by the desire to make it fast.

I'd go one step further and store the maximum ngpio of all registered
chips in a global variable (and update it in gpiochip_add_data_with_key()),
then allocate 2 * max_ngpio once before entering the loop (as you've
suggested).  That would avoid the first pass to determine the maximum
chip->ngpio.  In most systems max_ngpio will be < 64, so one or two
unsigned longs depending on the arch's bitness.

FWIW, to achieve a stack overflow the platform or a driver need to specify
a huge number of GPIOs for a chip.  So the exploitability is limited,
but of course it's still better to get rid of the VLAs.

Running v2 of this patch through checkpatch --strict results in a few
"Alignment should match open parenthesis" and one "Please don't use
multiple blank lines" complaint, granted those are nits but it may
be worth fixing them up front lest the usual suspects come along and
submit bikeshedding patches.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-17  8:25     ` Lukas Wunner
@ 2018-03-18 14:23       ` Lukas Wunner
  2018-03-18 20:34         ` Rasmus Villemoes
  2018-03-28  0:37         ` Laura Abbott
  0 siblings, 2 replies; 26+ messages in thread
From: Lukas Wunner @ 2018-03-18 14:23 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Laura Abbott, Linus Walleij, Kees Cook, linux-gpio, linux-kernel,
	kernel-hardening, Mathias Duckeck, Nandor Han, Semi Malinen,
	Patrice Chotard

On Sat, Mar 17, 2018 at 09:25:09AM +0100, Lukas Wunner wrote:
> On Mon, Mar 12, 2018 at 04:00:36PM +0100, Rasmus Villemoes wrote:
> > On 2018-03-10 01:10, Laura Abbott wrote:
> > > @@ -2887,14 +2909,30 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
> > >  
> > >  	while (i < array_size) {
> > >  		struct gpio_chip *chip = desc_array[i]->gdev->chip;
> > > -		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
> > > -		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
> > > +		unsigned long *mask;
> > > +		unsigned long *bits;
> > >  		int count = 0;
> > >  
> > > +		mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
> > > +				sizeof(*mask),
> > > +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> > > +
> > > +		if (!mask)
> > > +			return;
> > > +
> > > +		bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
> > > +				sizeof(*bits),
> > > +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> > > +
> > > +		if (!bits) {
> > > +			kfree(mask);
> > > +			return;
> > > +		}
> > > +
> > >  		if (!can_sleep)
> > >  			WARN_ON(chip->can_sleep);
> > >  
> > > -		memset(mask, 0, sizeof(mask));
> > > +		memset(mask, 0, sizeof(*mask));
> > 
> > Other random thoughts: maybe two allocations for each loop iteration is
> > a bit much. Maybe do a first pass over the array and collect the maximal
> > chip->ngpio, do the memory allocation and freeing outside the loop (then
> > you'd of course need to preserve the memset() with appropriate length
> > computed). And maybe even just do one allocation, making bits point at
> > the second half.
> 
> I think those are great ideas because the function is kind of a hotpath
> and usage of VLAs was motivated by the desire to make it fast.
> 
> I'd go one step further and store the maximum ngpio of all registered
> chips in a global variable (and update it in gpiochip_add_data_with_key()),
> then allocate 2 * max_ngpio once before entering the loop (as you've
> suggested).  That would avoid the first pass to determine the maximum
> chip->ngpio.  In most systems max_ngpio will be < 64, so one or two
> unsigned longs depending on the arch's bitness.

Actually, scratch that.  If ngpio is usually smallish, we can just
allocate reasonably sized space for mask and bits on the stack,
and fall back to the kcalloc slowpath only if chip->ngpio exceeds
that limit.  Basically the below (likewise compile-tested only),
this is on top of Laura's patch, could be squashed together.
Let me know what you think, thanks.

-- >8 --
Subject: [PATCH] gpio: Add fastpath to gpiod_get/set_array_value_complex()

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpio/gpiolib.c | 76 ++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 39 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 429bc251392b..ffc67b0b866c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2432,6 +2432,8 @@ static int gpio_chip_get_multiple(struct gpio_chip *chip,
 	return -EIO;
 }
 
+#define FASTPATH_NGPIO 256
+
 int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				  unsigned int array_size,
 				  struct gpio_desc **desc_array,
@@ -2441,27 +2443,24 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 
 	while (i < array_size) {
 		struct gpio_chip *chip = desc_array[i]->gdev->chip;
-		unsigned long *mask;
-		unsigned long *bits;
+		unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+		unsigned long *slowpath = NULL, *mask, *bits;
 		int first, j, ret;
 
-		mask = kcalloc(BITS_TO_LONGS(chip->ngpio),
-				sizeof(*mask),
-				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
-
-		if (!mask)
-			return -ENOMEM;
-
-		bits = kcalloc(BITS_TO_LONGS(chip->ngpio),
-				sizeof(*bits),
-				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
-
-		if (!bits) {
-			kfree(mask);
-			return -ENOMEM;
+		if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+			memset(fastpath, 0, sizeof(fastpath));
+			mask = fastpath;
+			bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);
+		} else {
+			slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+					   sizeof(*slowpath),
+					   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+			if (!slowpath)
+				return -ENOMEM;
+			mask = slowpath;
+			bits = slowpath + BITS_TO_LONGS(chip->ngpio);
 		}
 
-
 		if (!can_sleep)
 			WARN_ON(chip->can_sleep);
 
@@ -2478,8 +2477,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 
 		ret = gpio_chip_get_multiple(chip, mask, bits);
 		if (ret) {
-			kfree(bits);
-			kfree(mask);
+			if (slowpath)
+				kfree(slowpath);
 			return ret;
 		}
 
@@ -2493,8 +2492,9 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			value_array[j] = value;
 			trace_gpio_value(desc_to_gpio(desc), 1, value);
 		}
-		kfree(bits);
-		kfree(mask);
+
+		if (slowpath)
+			kfree(slowpath);
 	}
 	return 0;
 }
@@ -2699,24 +2699,22 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 
 	while (i < array_size) {
 		struct gpio_chip *chip = desc_array[i]->gdev->chip;
-		unsigned long *mask;
-		unsigned long *bits;
+		unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+		unsigned long *slowpath = NULL, *mask, *bits;
 		int count = 0;
 
-		mask = kcalloc(BITS_TO_LONGS(chip->ngpio),
-				sizeof(*mask),
-				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
-
-		if (!mask)
-			return -ENOMEM;
-
-		bits = kcalloc(BITS_TO_LONGS(chip->ngpio),
-				sizeof(*bits),
-				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
-
-		if (!bits) {
-			kfree(mask);
-			return -ENOMEM;
+		if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+			memset(fastpath, 0, sizeof(fastpath));
+			mask = fastpath;
+			bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);
+		} else {
+			slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+					   sizeof(*slowpath),
+					   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+			if (!slowpath)
+				return -ENOMEM;
+			mask = slowpath;
+			bits = slowpath + BITS_TO_LONGS(chip->ngpio);
 		}
 
 		if (!can_sleep)
@@ -2753,8 +2751,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 		if (count != 0)
 			gpio_chip_set_multiple(chip, mask, bits);
 
-		kfree(mask);
-		kfree(bits);
+		if (slowpath)
+			kfree(slowpath);
 	}
 	return 0;
 }
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-18 14:23       ` Lukas Wunner
@ 2018-03-18 20:34         ` Rasmus Villemoes
  2018-03-19  7:00           ` Lukas Wunner
  2018-03-28  0:37         ` Laura Abbott
  1 sibling, 1 reply; 26+ messages in thread
From: Rasmus Villemoes @ 2018-03-18 20:34 UTC (permalink / raw)
  To: Lukas Wunner, Rasmus Villemoes
  Cc: Laura Abbott, Linus Walleij, Kees Cook, linux-gpio, linux-kernel,
	kernel-hardening, Mathias Duckeck, Nandor Han, Semi Malinen,
	Patrice Chotard

On 2018-03-18 15:23, Lukas Wunner wrote:
>>>
>>> Other random thoughts: maybe two allocations for each loop iteration is
>>> a bit much. Maybe do a first pass over the array and collect the maximal
>>> chip->ngpio, do the memory allocation and freeing outside the loop (then
>>> you'd of course need to preserve the memset() with appropriate length
>>> computed). And maybe even just do one allocation, making bits point at
>>> the second half.
>>
>> I think those are great ideas because the function is kind of a hotpath
>> and usage of VLAs was motivated by the desire to make it fast.
>>
>> I'd go one step further and store the maximum ngpio of all registered
>> chips in a global variable (and update it in gpiochip_add_data_with_key()),
>> then allocate 2 * max_ngpio once before entering the loop (as you've
>> suggested).  That would avoid the first pass to determine the maximum
>> chip->ngpio.  In most systems max_ngpio will be < 64, so one or two
>> unsigned longs depending on the arch's bitness.
> 
> Actually, scratch that.  If ngpio is usually smallish, we can just
> allocate reasonably sized space for mask and bits on the stack,

Yes.

> and fall back to the kcalloc slowpath only if chip->ngpio exceeds
> that limit.

Well, I'd suggest not adding that fallback code now, but simply add a
check in gpiochip_add_data_with_key to ensure ngpio is sane (and refuse
to register the chip otherwise), at least if we know that every
currently supported/known chip is covered by the 256 (?). That keeps the
code simple and fast, and then if somebody has a chip with 40000 gpio
lines, we can add a fallback path. Or we could consider alternative
solutions, to avoid a 10000 byte GFP_ATOMIC allocation (maybe hang a
pre-allocation off the gpio_chip; that's only two more bits per
descriptor, and there's already a whole gpio_desc for each - but not
sure about the locking in that case).

Rasmus

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-18 20:34         ` Rasmus Villemoes
@ 2018-03-19  7:00           ` Lukas Wunner
  2018-03-19 15:09             ` Andy Shevchenko
  0 siblings, 1 reply; 26+ messages in thread
From: Lukas Wunner @ 2018-03-19  7:00 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Laura Abbott, Linus Walleij, Kees Cook, linux-gpio, linux-kernel,
	kernel-hardening, Mathias Duckeck, Nandor Han, Semi Malinen,
	Patrice Chotard

On Sun, Mar 18, 2018 at 09:34:12PM +0100, Rasmus Villemoes wrote:
> On 2018-03-18 15:23, Lukas Wunner wrote:
> >>> Other random thoughts: maybe two allocations for each loop iteration is
> >>> a bit much. Maybe do a first pass over the array and collect the maximal
> >>> chip->ngpio, do the memory allocation and freeing outside the loop (then
> >>> you'd of course need to preserve the memset() with appropriate length
> >>> computed). And maybe even just do one allocation, making bits point at
> >>> the second half.
> >>
> >> I think those are great ideas because the function is kind of a hotpath
> >> and usage of VLAs was motivated by the desire to make it fast.
> >>
> >> I'd go one step further and store the maximum ngpio of all registered
> >> chips in a global variable (and update it in gpiochip_add_data_with_key()),
> >> then allocate 2 * max_ngpio once before entering the loop (as you've
> >> suggested).  That would avoid the first pass to determine the maximum
> >> chip->ngpio.  In most systems max_ngpio will be < 64, so one or two
> >> unsigned longs depending on the arch's bitness.
> > 
> > Actually, scratch that.  If ngpio is usually smallish, we can just
> > allocate reasonably sized space for mask and bits on the stack,
> 
> Yes.
> 
> > and fall back to the kcalloc slowpath only if chip->ngpio exceeds
> > that limit.
> 
> Well, I'd suggest not adding that fallback code now, but simply add a
> check in gpiochip_add_data_with_key to ensure ngpio is sane (and refuse
> to register the chip otherwise), at least if we know that every
> currently supported/known chip is covered by the 256 (?).

The number 256 was arbitrarily chosen.  I really wouldn't be surprised
if gpiochips with more pins exist, but they're probably rare, so using
the slowpath seems fine, but dropping support for them completely would
be a regression.

E.g. many serially attached chips such as MAX3191X are daisy-chainable
and the driver deliberately doesn't impose an upper limit on the number
of chips because the spec doesn't contain one either.  To the OS a
daisy-chain of such chips appears as a single gpiochip with many pins.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-19  7:00           ` Lukas Wunner
@ 2018-03-19 15:09             ` Andy Shevchenko
  0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2018-03-19 15:09 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Rasmus Villemoes, Laura Abbott, Linus Walleij, Kees Cook,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
	kernel-hardening, Mathias Duckeck, Nandor Han, Semi Malinen,
	Patrice Chotard

On Mon, Mar 19, 2018 at 9:00 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Sun, Mar 18, 2018 at 09:34:12PM +0100, Rasmus Villemoes wrote:
>> On 2018-03-18 15:23, Lukas Wunner wrote:

>> > Actually, scratch that.  If ngpio is usually smallish, we can just
>> > allocate reasonably sized space for mask and bits on the stack,
>> > and fall back to the kcalloc slowpath only if chip->ngpio exceeds
>> > that limit.

>> Well, I'd suggest not adding that fallback code now, but simply add a
>> check in gpiochip_add_data_with_key to ensure ngpio is sane (and refuse
>> to register the chip otherwise), at least if we know that every
>> currently supported/known chip is covered by the 256 (?).
>
> The number 256 was arbitrarily chosen.  I really wouldn't be surprised
> if gpiochips with more pins exist, but they're probably rare, so using
> the slowpath seems fine, but dropping support for them completely would
> be a regression.

All modern Intel SoCs have GPIO count in between of ~230-380.
Though, few of them are split to communities by (much) less than 256
pin in each when there is a 1:1 mapping between community and
gpiochip.

OTOH, the function you are fixing is most likely is not used together
with the drivers for x86.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] gpio: Remove VLA from MAX3191X driver
  2018-03-10  0:10 ` [PATCH 2/4] gpio: Remove VLA from MAX3191X driver Laura Abbott
@ 2018-03-26  9:07   ` Linus Walleij
  0 siblings, 0 replies; 26+ messages in thread
From: Linus Walleij @ 2018-03-26  9:07 UTC (permalink / raw)
  To: Laura Abbott
  Cc: Kees Cook, Lukas Wunner, open list:GPIO SUBSYSTEM, linux-kernel,
	kernel-hardening, Mathias Duckeck

On Sat, Mar 10, 2018 at 1:10 AM, Laura Abbott <labbott@redhat.com> wrote:

>
> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
>
> This patch replaces several a VLA with an appropriate call to
> kmalloc_array.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Patch applied.

It's very simple and straight-forward after all.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] gpio: Remove VLA from xra1403 driver
  2018-03-10  0:10 ` [PATCH 3/4] gpio: Remove VLA from xra1403 driver Laura Abbott
  2018-03-12  6:06   ` EXT: " Nandor Han
@ 2018-03-26  9:09   ` Linus Walleij
  2018-03-28  7:27   ` Geert Uytterhoeven
  2 siblings, 0 replies; 26+ messages in thread
From: Linus Walleij @ 2018-03-26  9:09 UTC (permalink / raw)
  To: Laura Abbott
  Cc: Kees Cook, Nandor Han, Semi Malinen, open list:GPIO SUBSYSTEM,
	linux-kernel, kernel-hardening

On Sat, Mar 10, 2018 at 1:10 AM, Laura Abbott <labbott@redhat.com> wrote:

> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
>
> This patch replaces a VLA with an appropriate call to kmalloc_array.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Patch applied with Nandor's review tag.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-18 14:23       ` Lukas Wunner
  2018-03-18 20:34         ` Rasmus Villemoes
@ 2018-03-28  0:37         ` Laura Abbott
  2018-03-28  3:54           ` Lukas Wunner
  1 sibling, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-28  0:37 UTC (permalink / raw)
  To: Lukas Wunner, Rasmus Villemoes
  Cc: Linus Walleij, Kees Cook, linux-gpio, linux-kernel,
	kernel-hardening, Mathias Duckeck, Nandor Han, Semi Malinen,
	Patrice Chotard

On 03/18/2018 07:23 AM, Lukas Wunner wrote:
> On Sat, Mar 17, 2018 at 09:25:09AM +0100, Lukas Wunner wrote:
>> On Mon, Mar 12, 2018 at 04:00:36PM +0100, Rasmus Villemoes wrote:
>>> On 2018-03-10 01:10, Laura Abbott wrote:
>>>> @@ -2887,14 +2909,30 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
>>>>   
>>>>   	while (i < array_size) {
>>>>   		struct gpio_chip *chip = desc_array[i]->gdev->chip;
>>>> -		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
>>>> -		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
>>>> +		unsigned long *mask;
>>>> +		unsigned long *bits;
>>>>   		int count = 0;
>>>>   
>>>> +		mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
>>>> +				sizeof(*mask),
>>>> +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
>>>> +
>>>> +		if (!mask)
>>>> +			return;
>>>> +
>>>> +		bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio),
>>>> +				sizeof(*bits),
>>>> +				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
>>>> +
>>>> +		if (!bits) {
>>>> +			kfree(mask);
>>>> +			return;
>>>> +		}
>>>> +
>>>>   		if (!can_sleep)
>>>>   			WARN_ON(chip->can_sleep);
>>>>   
>>>> -		memset(mask, 0, sizeof(mask));
>>>> +		memset(mask, 0, sizeof(*mask));
>>>
>>> Other random thoughts: maybe two allocations for each loop iteration is
>>> a bit much. Maybe do a first pass over the array and collect the maximal
>>> chip->ngpio, do the memory allocation and freeing outside the loop (then
>>> you'd of course need to preserve the memset() with appropriate length
>>> computed). And maybe even just do one allocation, making bits point at
>>> the second half.
>>
>> I think those are great ideas because the function is kind of a hotpath
>> and usage of VLAs was motivated by the desire to make it fast.
>>
>> I'd go one step further and store the maximum ngpio of all registered
>> chips in a global variable (and update it in gpiochip_add_data_with_key()),
>> then allocate 2 * max_ngpio once before entering the loop (as you've
>> suggested).  That would avoid the first pass to determine the maximum
>> chip->ngpio.  In most systems max_ngpio will be < 64, so one or two
>> unsigned longs depending on the arch's bitness.
> 
> Actually, scratch that.  If ngpio is usually smallish, we can just
> allocate reasonably sized space for mask and bits on the stack,
> and fall back to the kcalloc slowpath only if chip->ngpio exceeds
> that limit.  Basically the below (likewise compile-tested only),
> this is on top of Laura's patch, could be squashed together.
> Let me know what you think, thanks.
> 

It seems like there's general consensus this is okay so I'm going
to fold it into the next version. If not, we can discuss again.

> -- >8 --
> Subject: [PATCH] gpio: Add fastpath to gpiod_get/set_array_value_complex()
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>   drivers/gpio/gpiolib.c | 76 ++++++++++++++++++++++++--------------------------
>   1 file changed, 37 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 429bc251392b..ffc67b0b866c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2432,6 +2432,8 @@ static int gpio_chip_get_multiple(struct gpio_chip *chip,
>   	return -EIO;
>   }
>   
> +#define FASTPATH_NGPIO 256
> +
>   int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   				  unsigned int array_size,
>   				  struct gpio_desc **desc_array,
> @@ -2441,27 +2443,24 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   
>   	while (i < array_size) {
>   		struct gpio_chip *chip = desc_array[i]->gdev->chip;
> -		unsigned long *mask;
> -		unsigned long *bits;
> +		unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
> +		unsigned long *slowpath = NULL, *mask, *bits;
>   		int first, j, ret;
>   
> -		mask = kcalloc(BITS_TO_LONGS(chip->ngpio),
> -				sizeof(*mask),
> -				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> -
> -		if (!mask)
> -			return -ENOMEM;
> -
> -		bits = kcalloc(BITS_TO_LONGS(chip->ngpio),
> -				sizeof(*bits),
> -				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> -
> -		if (!bits) {
> -			kfree(mask);
> -			return -ENOMEM;
> +		if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
> +			memset(fastpath, 0, sizeof(fastpath));
> +			mask = fastpath;
> +			bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);
> +		} else {
> +			slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
> +					   sizeof(*slowpath),
> +					   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> +			if (!slowpath)
> +				return -ENOMEM;
> +			mask = slowpath;
> +			bits = slowpath + BITS_TO_LONGS(chip->ngpio);
>   		}
>   
> -
>   		if (!can_sleep)
>   			WARN_ON(chip->can_sleep);
>   
> @@ -2478,8 +2477,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   
>   		ret = gpio_chip_get_multiple(chip, mask, bits);
>   		if (ret) {
> -			kfree(bits);
> -			kfree(mask);
> +			if (slowpath)
> +				kfree(slowpath);
>   			return ret;
>   		}
>   
> @@ -2493,8 +2492,9 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   			value_array[j] = value;
>   			trace_gpio_value(desc_to_gpio(desc), 1, value);
>   		}
> -		kfree(bits);
> -		kfree(mask);
> +
> +		if (slowpath)
> +			kfree(slowpath);
>   	}
>   	return 0;
>   }
> @@ -2699,24 +2699,22 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>   
>   	while (i < array_size) {
>   		struct gpio_chip *chip = desc_array[i]->gdev->chip;
> -		unsigned long *mask;
> -		unsigned long *bits;
> +		unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
> +		unsigned long *slowpath = NULL, *mask, *bits;
>   		int count = 0;
>   
> -		mask = kcalloc(BITS_TO_LONGS(chip->ngpio),
> -				sizeof(*mask),
> -				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> -
> -		if (!mask)
> -			return -ENOMEM;
> -
> -		bits = kcalloc(BITS_TO_LONGS(chip->ngpio),
> -				sizeof(*bits),
> -				can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> -
> -		if (!bits) {
> -			kfree(mask);
> -			return -ENOMEM;
> +		if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
> +			memset(fastpath, 0, sizeof(fastpath));
> +			mask = fastpath;
> +			bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);
> +		} else {
> +			slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
> +					   sizeof(*slowpath),
> +					   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> +			if (!slowpath)
> +				return -ENOMEM;
> +			mask = slowpath;
> +			bits = slowpath + BITS_TO_LONGS(chip->ngpio);
>   		}
>   
>   		if (!can_sleep)
> @@ -2753,8 +2751,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>   		if (count != 0)
>   			gpio_chip_set_multiple(chip, mask, bits);
>   
> -		kfree(mask);
> -		kfree(bits);
> +		if (slowpath)
> +			kfree(slowpath);
>   	}
>   	return 0;
>   }
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] gpio: Remove VLA from gpiolib
  2018-03-28  0:37         ` Laura Abbott
@ 2018-03-28  3:54           ` Lukas Wunner
  0 siblings, 0 replies; 26+ messages in thread
From: Lukas Wunner @ 2018-03-28  3:54 UTC (permalink / raw)
  To: Laura Abbott
  Cc: Rasmus Villemoes, Linus Walleij, Kees Cook, linux-gpio,
	linux-kernel, kernel-hardening, Mathias Duckeck, Nandor Han,
	Semi Malinen, Patrice Chotard

On Tue, Mar 27, 2018 at 05:37:18PM -0700, Laura Abbott wrote:
> On 03/18/2018 07:23 AM, Lukas Wunner wrote:
> > Actually, scratch that.  If ngpio is usually smallish, we can just
> > allocate reasonably sized space for mask and bits on the stack,
> > and fall back to the kcalloc slowpath only if chip->ngpio exceeds
> > that limit.  Basically the below (likewise compile-tested only),
> > this is on top of Laura's patch, could be squashed together.
> > Let me know what you think, thanks.
> >
> 
> It seems like there's general consensus this is okay so I'm going
> to fold it into the next version. If not, we can discuss again.

Yes, feel free to squash into your original patch with my S-o-b,
keep your authorship.

You may want to raise FASTPATH_NGPIO to something like 384, 448 or 512
to accommodate for the Intel chips Andy mentioned.  It's kind of a
"640k should be enough for everyone" thing but I'd expect the performance
impact of the extra bytes on the stack / memsetting them to zero
to be absolutely negligible.

Thanks!

Lukas

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] gpio: Remove VLA from xra1403 driver
  2018-03-10  0:10 ` [PATCH 3/4] gpio: Remove VLA from xra1403 driver Laura Abbott
  2018-03-12  6:06   ` EXT: " Nandor Han
  2018-03-26  9:09   ` Linus Walleij
@ 2018-03-28  7:27   ` Geert Uytterhoeven
  2018-03-28 17:27     ` Laura Abbott
  2 siblings, 1 reply; 26+ messages in thread
From: Geert Uytterhoeven @ 2018-03-28  7:27 UTC (permalink / raw)
  To: Laura Abbott
  Cc: Linus Walleij, Kees Cook, Nandor Han, Semi Malinen,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
	kernel-hardening

Hi Laura,

On Sat, Mar 10, 2018 at 1:10 AM, Laura Abbott <labbott@redhat.com> wrote:
> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
>
> This patch replaces a VLA with an appropriate call to kmalloc_array.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Thanks for your patch!

> --- a/drivers/gpio/gpio-xra1403.c
> +++ b/drivers/gpio/gpio-xra1403.c
> @@ -126,11 +126,16 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  {
>         int reg;
>         struct xra1403 *xra = gpiochip_get_data(chip);
> -       int value[xra1403_regmap_cfg.max_register];

Apparently xra1403_regmap_cfg.max_register is always 0x15?

What about adding

        #define XRA_LAST 15

at the top, and replacing both "XRA_IFR | 0x01" and
xra1403_regmap_cfg.max_register by XRA_LAST instead?
That would avoid doing yet another memory allocation over and over.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] gpio: Remove VLA from xra1403 driver
  2018-03-28  7:27   ` Geert Uytterhoeven
@ 2018-03-28 17:27     ` Laura Abbott
  2018-04-04 12:53       ` Linus Walleij
  0 siblings, 1 reply; 26+ messages in thread
From: Laura Abbott @ 2018-03-28 17:27 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linus Walleij, Kees Cook, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List, kernel-hardening



On 03/28/2018 12:27 AM, Geert Uytterhoeven wrote:
> Hi Laura,
> 
> On Sat, Mar 10, 2018 at 1:10 AM, Laura Abbott <labbott@redhat.com> wrote:
>> The new challenge is to remove VLAs from the kernel
>> (see https://lkml.org/lkml/2018/3/7/621)
>>
>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/gpio/gpio-xra1403.c
>> +++ b/drivers/gpio/gpio-xra1403.c
>> @@ -126,11 +126,16 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>>   {
>>          int reg;
>>          struct xra1403 *xra = gpiochip_get_data(chip);
>> -       int value[xra1403_regmap_cfg.max_register];
> 
> Apparently xra1403_regmap_cfg.max_register is always 0x15?
> 
> What about adding
> 
>          #define XRA_LAST 15
> 
> at the top, and replacing both "XRA_IFR | 0x01" and
> xra1403_regmap_cfg.max_register by XRA_LAST instead?
> That would avoid doing yet another memory allocation over and over.
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

I'm okay with making the change but I think Linus already picked
up the patch into his gpio trees. Linus, do you want a patch on
top of your -devel branch or should I just send a new patch?

Thanks,
Laura

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] gpio: Remove VLA from xra1403 driver
  2018-03-28 17:27     ` Laura Abbott
@ 2018-04-04 12:53       ` Linus Walleij
  0 siblings, 0 replies; 26+ messages in thread
From: Linus Walleij @ 2018-04-04 12:53 UTC (permalink / raw)
  To: Laura Abbott
  Cc: Geert Uytterhoeven, Kees Cook, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List, kernel-hardening

On Wed, Mar 28, 2018 at 7:27 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 03/28/2018 12:27 AM, Geert Uytterhoeven wrote:
>>
>> Hi Laura,
>>
>> On Sat, Mar 10, 2018 at 1:10 AM, Laura Abbott <labbott@redhat.com> wrote:
>>>
>>> The new challenge is to remove VLAs from the kernel
>>> (see https://lkml.org/lkml/2018/3/7/621)
>>>
>>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>
>>
>> Thanks for your patch!
>>
>>> --- a/drivers/gpio/gpio-xra1403.c
>>> +++ b/drivers/gpio/gpio-xra1403.c
>>> @@ -126,11 +126,16 @@ static void xra1403_dbg_show(struct seq_file *s,
>>> struct gpio_chip *chip)
>>>   {
>>>          int reg;
>>>          struct xra1403 *xra = gpiochip_get_data(chip);
>>> -       int value[xra1403_regmap_cfg.max_register];
>>
>>
>> Apparently xra1403_regmap_cfg.max_register is always 0x15?
>>
>> What about adding
>>
>>          #define XRA_LAST 15
>>
>> at the top, and replacing both "XRA_IFR | 0x01" and
>> xra1403_regmap_cfg.max_register by XRA_LAST instead?
>> That would avoid doing yet another memory allocation over and over.
>>
>> Gr{oetje,eeting}s,
>>
>>                          Geert
>>
>
> I'm okay with making the change but I think Linus already picked
> up the patch into his gpio trees. Linus, do you want a patch on
> top of your -devel branch or should I just send a new patch?

Yeah a patch on top is fine, I sent my pull request to Torvalds
today so we can take this as a fix for the -rc cycle simply.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2018-04-04 12:53 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-10  0:10 [PATCH 0/4] VLA removal from the GPIO subsystem Laura Abbott
2018-03-10  0:10 ` [PATCH 1/4] gpio: Remove VLA from gpiolib Laura Abbott
2018-03-12 15:00   ` Rasmus Villemoes
2018-03-12 23:40     ` Laura Abbott
2018-03-13  7:23       ` Rasmus Villemoes
2018-03-17  8:25     ` Lukas Wunner
2018-03-18 14:23       ` Lukas Wunner
2018-03-18 20:34         ` Rasmus Villemoes
2018-03-19  7:00           ` Lukas Wunner
2018-03-19 15:09             ` Andy Shevchenko
2018-03-28  0:37         ` Laura Abbott
2018-03-28  3:54           ` Lukas Wunner
2018-03-10  0:10 ` [PATCH 2/4] gpio: Remove VLA from MAX3191X driver Laura Abbott
2018-03-26  9:07   ` Linus Walleij
2018-03-10  0:10 ` [PATCH 3/4] gpio: Remove VLA from xra1403 driver Laura Abbott
2018-03-12  6:06   ` EXT: " Nandor Han
2018-03-26  9:09   ` Linus Walleij
2018-03-28  7:27   ` Geert Uytterhoeven
2018-03-28 17:27     ` Laura Abbott
2018-04-04 12:53       ` Linus Walleij
2018-03-10  0:10 ` [PATCH 4/4] gpio: Remove VLA from stmpe driver Laura Abbott
2018-03-13  9:13   ` Phil Reid
2018-03-14  0:18     ` Laura Abbott
2018-03-14  1:16       ` Laura Abbott
2018-03-14  2:55         ` Phil Reid
2018-03-13  9:42 ` [PATCH 0/4] VLA removal from the GPIO subsystem Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).