linux-kernel - Re: [PATCH 0/2] arm64 defconfig: Get faddr2line working

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: <CAK8P3a2+0EwSJ=fRBL2JCGypJRL-qE4rEiXYnJbqhZ=weethdQ@mail.gmail.com>
Date:   Mon, 25 Jul 2022 14:51:00 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     John Garry <john.garry@...wei.com>
Cc:     Arnd Bergmann <arnd@...db.de>, Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Olof Johansson <olof@...om.net>, SoC Team <soc@...nel.org>,
        jpoimboe@...nel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/2] arm64 defconfig: Get faddr2line working

On Mon, Jul 25, 2022 at 1:57 PM John Garry <john.garry@...wei.com> wrote:
>
> On 24/07/2022 21:35, Arnd Bergmann wrote:
>
> Here's a brief'ish summary for why it's ok to delete these mentioned
> configs:
>
> > CONFIG_ARCH_BCMBCA=y
>
> On next-20220722, config ARCH_BCMBCA is selected by config ARCH_BCM4908
> from arch/arm64/Kconfig.platforms and the latter is enabled in the
> defconfig, so no need to explicitly enable in the defconfig.
>
> On arm-soc arm/defconfig, config ARCH_BCMBCA does not exist for arm64
> platforms yet, so this is why we see this config deleted for the sync.
>
> > CONFIG_SECCOMP=y
>
> Since commit 282a181b1a0d, config SECCOMP was changed to def_bool y, so
> no need to explicitly enable in the defconfig.

Ok

> > CONFIG_QRTR=m
>
>  From commit fdf4632aa834, we enable config ATH11K_PCI, which selects
> config QRTR, so no need to explicitly enable in the defconfig.

Something is wrong here, as qrtr is used the same way in both ath10k and
ath11k, but only one of them selects the symbol. My guess is that there is
no actual hard requirement to enable it.

> > CONFIG_PINCTRL_MSM=y
>
>  From commit c807a335d3b1, config PINCTRL_SM8450 is enabled, which
> selects PINCTRL_MSM, so no need to explicitly enable in the defconfig.

This looks like a bug, where the driver should have used 'depends on'
rather than 'select'.

> > CONFIG_SND_SOC_TEGRA210_OPE=m
>
> There is no issue on next-20220722.
>
> On arm-soc arm/defconfig, config SND_SOC_TEGRA210_OPE just does not yet
> exist, so that's why it get removed from the sync of the defconfig.

Ok

> > CONFIG_MAILBOX=y
>
> In commit fc739069aa92, config MAILBOX was enabled in the defconfig but
> it was already being enabled from elsewhere. There was definitely no
> sync of the savedefconfig going on there :)

I see it's selected by a couple of drivers using mailboxes, and I
agree we shouldn't
need it here. It might be good to just hide the symbol in this case
and select it
consistently from all drivers using it.

> > CONFIG_QCOM_ICC_BWMON=m
>
> Commit 76f11e77f919 enabled config QCOM_ICC_BWMON, but like config
> ARCH_BCMBCA, that config does not exist on arm-soc arm/defconfig branch
>
> On next-20220722, no sync is required

ok

> > CONFIG_SLIMBUS=m
>
> Config 5bd773242f75 added a kconfig "imply" on config SLIMBUS from
> config SOUNDWIRE_QCOM, and this happens to mean that we don't explicitly
> require config SLIMBUS enabled in the defconfig.

That 'imply' looks like it was added to solve a problem using the old 'imply'
semantics that are not what this keyword does today. I suspect it's still
broken when CONFIG=SOUNDWIRE_QCOM=y and CONFIG_SLIBMUS=m,
and the correct fix is to use a dependency like

      depends on SLIMBUS || !SLIMBUS

so the defconfig symbol should stay.

> > CONFIG_INTERCONNECT=y
>
> Since commit 06f079816d4c, config TEGRA_MC added a kconfig select on
> config INTERCONNECT, so no need to explicitly enable from the kconfig

We have one driver using 'depends on INTERCONNECT' and two drivers
using 'select INTERCONNECT', so at least one of them is wrong.

There are also a couple of drivers that use neither and instead rely on the
IS_ENABLED() check in include/linux/interconnect.h, but this is potentially
wrong in the same way as the SLIMBUS dependency above.

This clearly needs some more discussion. I would suggest removing the
#iff check in the header and forcing all users to use 'depends on', leaving
the defconfig symbol.

> > CONFIG_CONFIGFS_FS=y
>
>  From commit cd8bc7d4eb66, config PCI_ENDPOINT_CONFIGFS is enabled in
> the defconfig, and this selects CONFIG_CONFIGFS_FS, so no need to have
> explicit enabling in the defconfig.

Again, half the users of CONFIGFS_FS use 'depends on', the other half use
'select'. We should be consistent here and always use the same method,
probably 'depends on' if we want to keep it as user-visible.

> We still have issues on next-22072022:
> CONFIG_ARM_CPUIDE
> CONFIG_DEBUG_INFO
> CONFIG_DEBUG_INFO_REDUCED
>
> The latter two are not an issue on the arm-soc arm/defconfig.

Ok.

> So let me know if any comments or how to proceed.
>
> And would each config item deletion merit a separate patch?

You send a combined patch for the obvious ones (secccomp
and mailbox AFAICT) or send them separately. For the other ones I think
we should try fixing the Kconfig files first, otherwise we just end up
putting them back afterwards.

        Arnd