netdev - Re: nt: usb: USB_RTL8153_ECM should not default to y

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: <20210917123713.67e116aa@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 17 Sep 2021 12:37:13 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Maciej Żenczykowski <zenczykowski@...il.com>
Cc:     Geert Uytterhoeven <geert+renesas@...der.be>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux NetDev <netdev@...r.kernel.org>,
        Hayes Wang <hayeswang@...ltek.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: nt: usb: USB_RTL8153_ECM should not default to y

On Fri, 17 Sep 2021 21:05:55 +0200 Maciej Żenczykowski wrote:
> > Yeah.. more context here:
> >
> > https://lore.kernel.org/all/7fd014f2-c9a5-e7ec-f1c6-b3e4bb0f6eb6@samsung.com/
> >
> > default !USB_RTL8152 would be my favorite but that probably doesn't
> > compute in kconfig land. Or perhaps bring back the 'y' but more clearly
> > mark it as a sub-option of CDCETHER? It's hard to blame people for
> > expecting drivers to default to n, we should make it clearer that this
> > is more of a "make driver X support variation Y", 'cause now it sounds
> > like a completely standalone driver from the Kconfig wording. At least
> > to a lay person like myself.  
> 
> I think:
>         depends on USB_NET_CDCETHER && (USB_RTL8152 || USB_RTL8152=n)
>         default y
> accomplished exactly what was wanted.
> 
> USB_NET_CDCETHER is a dependency, hence:
> 
> USB_NET_CDCETHER=n forces it off - as it should - it's an addon to cdcether.
> 
> USB_NET_CDCETHER=m disallows 'y' - module implies addon must be module.
> 
> similarly USB_RTL8152 is a dependency, so it being a module disallows 'y'.
> This is desired, because if CDCETHER is builtin, so this addon could
> be builtin, then RTL8152 would fail to bind it by default.
> ie. CDCETHER=y && RTL8152=m must force RTL8153_ECM != y  (this is the bugfix)
> 
> basically the funky 'USB_RTL8152 || USB_RTL8152=n' --> disallows 'y'
> iff RTL8152=m
> 
> 'default y' enables it by default as 'y' if possible, as 'm' if not,
> and disables it if impossible.
> 
> So I believe this had the exact right default behaviour - and allowed
> all the valid options.

Right, it was _technically_ correct but run afoul of the "drivers
should default to 'n'" policy. If it should not be treated as a driver
but more of a feature of an existing driver which user has already
selected we should refine the name of the option to make that clear.