linux-kernel - Re: [PATCHv4] gpio: Remove VLA from gpiolib

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c618af81-2dbb-1aff-212a-e87e56caa7eb@electromag.com.au>
Date:   Fri, 13 Apr 2018 08:39:06 +0800
From:   Phil Reid <preid@...ctromag.com.au>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Laura Abbott <labbott@...hat.com>
Cc:     Kees Cook <keescook@...omium.org>, Lukas Wunner <lukas@...ner.de>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCHv4] gpio: Remove VLA from gpiolib

On 12/04/2018 16:38, Linus Walleij wrote:
> On Wed, Apr 11, 2018 at 3:03 AM, Laura Abbott <labbott@...hat.com> wrote:
> 
>> The new challenge is to remove VLAs from the kernel
>> (see https://lkml.org/lkml/2018/3/7/621) to eventually
>> turn on -Wvla.
>>
>> Using a kmalloc array is the easy way to fix this but kmalloc is still
>> more expensive than stack allocation. Introduce a fast path with a
>> fixed size stack array to cover most chip with gpios below some fixed
>> amount. The slow path dynamically allocates an array to cover those
>> chips with a large number of gpios.
>>
>> Reviewed-and-tested-by: Lukas Wunner <lukas@...ner.de>
>> Signed-off-by: Lukas Wunner <lukas@...ner.de>
>> Signed-off-by: Laura Abbott <labbott@...hat.com>
>> ---
>> v4: Changed some local variables to avoid coccinelle warnings. Added a
>> warning if the number of GPIOs exceeds the current fast path define.
>>
>> Lukas, I kept your Tested-by because the changes were pretty minimal.
>> Let me know if you want to run the tests again.
> 
> This patch is starting to look really good.
> 
>> +/*
>> + * Number of GPIOs to use for the fast path in set array
>> + */
>> +#define FASTPATH_NGPIO 256
> 
> There is still some comment about this.
> 
> And now that I am also tryint to think I wonder about it, we
> have a global ARCH_NR_GPIOS that is typically 512.
> Some archs set it up.
> 
> This define is something of an abomination, in the ARM
> case it comes from arch/arm/include/asm/gpio.h
> where #define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
> where the latter is a Kconfig option that is mostly 512 for
> most ARM systems.
> 
> Well, ARM looks like this:
> 
> config ARCH_NR_GPIO
>          int
>          default 2048 if ARCH_SOCFPGA
>          default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \
>                  ARCH_ZYNQ
>          default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \
>                  SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210
>          default 416 if ARCH_SUNXI
>          default 392 if ARCH_U8500
>          default 352 if ARCH_VT8500
>          default 288 if ARCH_ROCKCHIP
>          default 264 if MACH_H4700
>          default 0
>          help
>            Maximum number of GPIOs in the system.
> 
>            If unsure, leave the default value.
> 
> So if FASTPATH_NGPIO should be anything else than
> ARCH_NR_GPIO this has to be established somewhere
> as a floor or half or something, but I would just set it as
> the same as ARCH_NR_GPIOS...
> 
> The main reason this define exist is for this function
> from <linux/gpio/consumer.h>:
> 
> /* Convert between the old gpio_ and new gpiod_ interfaces */
> struct gpio_desc *gpio_to_desc(unsigned gpio);
> 
> Nowadays that fact is a bit obscured since the variable is
> only used when assigning the base (in the global GPIO
> number space, which is what we want to get rid of but
> sigh) in gpiochip_find_base() where it attempts to place
> a newly allocated gpiochip in the higher region of this
> numberspace since the embedded SoC GPIO base tends
> to be 0, on old platforms.
> 
> So I don't know about this.
> 
> Can't we just use ARCH_NR_GPIOS?
> 
> Very few systems have more than 512 assigned global
> GPIO numbers and those are FPGA experimental machines.
> 
> In the long run obviously I want to get rid of these defines
> altogether and only allocate GPIO descriptos dynamically
> so as you see I am reluctant to add new numberspace weirdness
> around here.
Isn't that for total GPIO's in the system?
And the arrays just need to cater for max per chip?
 From what I can understand of the code which is admittedly limited.


-- 
Regards
Phil Reid