linux-kernel - Re: [PATCH] staging: vchiq: add 'static' to function definition

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, 30 Oct 2022 11:46:04 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Scott J. Crouch" <scottjcrouch@...il.com>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc:     "Florian Fainelli" <f.fainelli@...il.com>,
        "Broadcom internal kernel review list" 
        <bcm-kernel-feedback-list@...adcom.com>,
        kernel-janitors@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: vchiq: add 'static' to function definition

On Sun, Oct 30, 2022, at 03:43, Scott J. Crouch wrote:
> On 28/10/22 01:08, Arnd Bergmann wrote:
>> On Sat, Oct 22, 2022, at 10:07, Greg Kroah-Hartman wrote:
>>> On Sat, Oct 22, 2022 at 03:35:48PM +1100, Scott J. Crouch wrote:
>> 
>>> Nice try, but this breaks the build in a very horrible and strange way
>>> that no one has been able to figure out yet:
>> 
>> I got curious and figured out what happens:
>> 
>> Without CONFIG_OF, of_match_node() always returns NULL, so
>> vchiq_probe() returns -EINVAL unconditionally before calling
>> vchiq_platform_init(). 
>> 
>> If vchiq_platform_init() is marked 'static', gcc's dead code
>> elimination then eliminates it, which in turn means that
>> 'g_fragments_base' is never initialized and gets replaced
>> with a NULL pointer.
>
> Good spotting.  Actually, I was clumsily learning how to run sparse on the
> staging directory and wasn't sure what config to use that didn't involve
> enabling everything manually.  But from what I can tell, it's OF_OVERLAY (or
> something downstream of that) that gets rid of the warning.  BCM2835_VCHIQ
> requires BCM_VIDEOCORE which already depends on OF. 

Ok, I see. My best guess would be OF_DYNAMIC in this case, but I don't
actually see how that changes anything in this file (I only looked
at the sources, did not build)

> I'm also confused why devm_rpi_firmware_get() doesn't have the same problem as
> of_match_node() just above it -- it returns NULL when RASPBERRYPI_FIRMWARE is
> unset, but gcc still builds without the warning.

Hmm, I see an of_node_put() between devm_rpi_firmware_get() and the
error check. With OF_DYNAMIC=y, this is an external function call, so
I guess gcc can no longer assume that drvdata->fw is NULL after that,
so it doesn't make this optimization.

This would mean you only get a warning when both RASPBERRYPI_FIRMWARE
and OF_DYNAMIC are disabled. If you can confirm that, adding a dependency
on RASPBERRYPI_FIRMWARE is still the correct fix.

        Arnd