linux-kernel - Re: [PATCH v2] gpiolib: Avoid side effects in gpio_is

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]

Date:   Sun, 28 May 2023 21:21:32 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Johan Hovold <johan@...nel.org>,
        Bartosz Golaszewski <brgl@...ev.pl>
CC:     Andy Shevchenko <andy.shevchenko@...il.com>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "maz@...nel.org" <maz@...nel.org>,
        "warthog618@...il.com" <warthog618@...il.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] gpiolib: Avoid side effects in gpio_is_visible()


On 27/05/23 01:23, Johan Hovold wrote:
> On Fri, May 26, 2023 at 03:01:01PM +0200, Bartosz Golaszewski wrote:
>> On Fri, May 19, 2023 at 12:09 PM Andy Shevchenko
>> <andy.shevchenko@...il.com> wrote:
>>> On Fri, May 19, 2023 at 8:07 AM Chris Packham
>>> <chris.packham@...iedtelesis.co.nz> wrote:
>>>> On a system with pca9555 GPIOs that have been exported via sysfs the
>>>> following warning could be triggered on kexec().
>>>>
>>>>    WARNING: CPU: 0 PID: 265 at drivers/gpio/gpiolib.c:3411 gpiochip_disable_irq
>>>>    Call trace:
>>>>     gpiochip_disable_irq
>>>>     machine_crash_shutdown
>>>>     __crash_kexec
>>>>     panic
>>>>     sysrq_reset_seq_param_set
>>>>     __handle_sysrq
>>>>     write_sysrq_trigger
>>>>
>>>> The warning is triggered because there is an irq_desc for the GPIO but
>>>> it does not have the FLAG_USED_AS_IRQ set. This is because when the GPIO
>>>> is exported via gpiod_export(), gpio_is_visible() is used to determine
>>>> if the "edge" attribute should be provided but in doing so it ends up
>>>> calling gpiochip_to_irq() which creates the irq_desc.
>>>>
>>>> Remove the call to gpiod_to_irq() from gpio_is_visible(). The actual
>>>> intended creation of the irq_desc comes via edge_store() when requested
>>>> by the user.
>>> To me it still sounds like a hack and the real solution should be done
>>> differently/elsewhere.
>>>
>>> Also I'm worrying that not having this file visible or not may affect
>>> existing user space custom scripts we will never hear about.
>>>
>>> P.S. TBH, I don't care much about sysfs, so if this patch finds its
>>> way upstream, I won't be unhappy.
>>>
>> Same. Which is why - if there'll be no more objections, I will apply it.
> I don't think this should be applied.
>
> It's still not clear from the commit message why gpiochip_disable_irq()
> is called for a line which has not been requested.

The code that does the calling is in machine_kexec_mask_interrupts(). 
The problem is that for some irq_chips irq_mask is set to the disable 
function. The disable call immediately after the mask call does check to 
see if the irq is not already disabled.

>   That seems like what
> should be fixed, not changing some behaviour in the gpio sysfs interface
> which has been there since forever (e.g. do not create the edge
> attributes for gpios that cannot be used as interrupts).

I don't disagree with the sentiment. The problem is there doesn't appear 
to be an API that can tell if a GPIO pin is capable of being an irq 
without actually converting it into one.

> There are other ways that mappings can be created (e.g. a gpio that
> requested as as interrupt and then released) which would trigger the
> same warning it seems.
I've tried a few of those cases and haven't been able to provoke the 
same warning. gpio_sysfs_free_irq() seems to clear whatever flags 
gpiochip_disable_irq() is complaining about.
> Fix the root cause, don't just paper over the symptom.
I think maybe there is a compromise where I do something in 
gpiochip_to_irq() instead of gpio_is_visible(). I'm not entirely sure 
what that something is
>
> Johan