netdev - Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

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: <YaR17NOQqvFxXEVs@unreal>
Date:   Mon, 29 Nov 2021 08:40:44 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc:     Johannes Berg <johannes@...solutions.net>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        Intel Corporation <linuxwwan@...el.com>,
        Loic Poulain <loic.poulain@...aro.org>
Subject: Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs
 optional

On Mon, Nov 29, 2021 at 02:45:16AM +0300, Sergey Ryazanov wrote:
> Add Leon to CC to merge both conversations.
> 
> On Sun, Nov 28, 2021 at 8:01 PM Johannes Berg <johannes@...solutions.net> wrote:
> > On Sun, 2021-11-28 at 15:55 +0300, Sergey Ryazanov wrote:
> >>
> >> +config WWAN_DEBUGFS
> >> +     bool "WWAN subsystem common debugfs interface"
> >> +     depends on DEBUG_FS
> >> +     help
> >> +       Enables common debugfs infrastructure for WWAN devices.
> >> +
> >> +       If unsure, say N.
> >>
> >
> > I wonder if that really should even say "If unsure, say N." because
> > really, once you have DEBUG_FS enabled, you can expect things to show up
> > there?
> >
> > And I'd probably even argue that it should be
> >
> >         bool "..." if EXPERT
> >         default y
> >         depends on DEBUG_FS
> >
> > so most people aren't even bothered by the question?
> >
> >
> >>  config WWAN_HWSIM
> >>       tristate "Simulated WWAN device"
> >>       help
> >> @@ -83,6 +91,7 @@ config IOSM
> >>  config IOSM_DEBUGFS
> >>       bool "IOSM Debugfs support"
> >>       depends on IOSM && DEBUG_FS
> >> +     select WWAN_DEBUGFS
> >>
> > I guess it's kind of a philosophical question, but perhaps it would make
> > more sense for that to be "depends on" (and then you can remove &&
> > DEBUG_FS"), since that way it becomes trivial to disable all of WWAN
> > debugfs and not have to worry about individual driver settings?
> >
> >
> > And after that change, I'd probably just make this one "def_bool y"
> > instead of asking the user.
> 
> When I was preparing this series, my primary considered use case was
> embedded firmwares. For example, in OpenWrt, you can not completely
> disable debugfs, as a lot of wireless stuff can only be configured and
> monitored with the debugfs knobs. At the same time, reducing the size
> of a kernel and modules is an essential task in the world of embedded
> software. Disabling the WWAN and IOSM debugfs interfaces allows us to
> save 50K (x86-64 build) of space for module storage. Not much, but
> already considerable when you only have 16MB of storage.
> 
> I personally like Johannes' suggestion to enable these symbols by
> default to avoid bothering PC users with such negligible things for
> them. One thing that makes me doubtful is whether we should hide the
> debugfs disabling option under the EXPERT. Or it would be an EXPERT
> option misuse, since the debugfs knobs existence themself does not
> affect regular WWAN device use.
> 
> Leon, would it be Ok with you to add these options to the kernel
> configuration and enable them by default?

I didn't block your previous proposal either. Just pointed that your
description doesn't correlate with the actual rationale for the patches.

Instead of security claims, just use your OpenWrt case as a base for
the commit message, which is very reasonable and valuable case.

However you should ask yourself if both IOSM_DEBUGFS and WWAN_DEBUGFS
are needed. You wrote that wwan debugfs is empty without ioasm. Isn't
better to allow user to select WWAN_DEBUGFS and change iosm code to
rely on it instead of IOSM_DEBUGFS?

Thanks

> 
> -- 
> Sergey