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-next>] [day] [month] [year] [list]

Message-ID: <CANP3RGeaOqxOMwCFKb=3X5EFaXNG+k3N2CfV4YT-8NiY5GW3Tg@mail.gmail.com>
Date:   Fri, 17 Sep 2021 19:59:15 +0200
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     Geert Uytterhoeven <geert+renesas@...der.be>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Linux NetDev <netdev@...r.kernel.org>,
        Hayes Wang <hayeswang@...ltek.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Cc:     Maciej Zenczykowski <maze@...gle.com>
Subject: Re: nt: usb: USB_RTL8153_ECM should not default to y

I've been browsing some usb ethernet dongle related stuff in the
kernel (trying to figure out which options to enable in Android 13
5.~15 kernels), and I've come across the following patch (see topic,
full patch quoted below).

Doesn't it entirely defeat the purpose of the patch it claims to fix
(and the patch that fixed)?
Certainly the reasoning provided (in general device drivers should not
be enabled by default) doesn't jive with me.
The device driver is CDC_ETHER and AFAICT this is just a compatibility
option for it.

Shouldn't it be reverted (ie. the 'default y' line be re-added) ?

AFAICT the logic should be:
  if we have CDC ETHER (aka. ECM), but we don't have R8152 then we
need to have R8153_ECM.

Alternatively, maybe there shouldn't be a config option for this at all?

Instead r8153_ecm should simply be part of cdc_ether.ko iff r8152=n

I'm not knowledgeable enough about Kconfig syntax to know how to
phrase the logic...
Maybe there shouldn't be a Kconfig option at all, and just some Makefile if'ery.

Something like:

obj-$(CONFIG_USB_RTL8152) += r8152.o
obj-$(CONFIG_USB_NET_CDCETHER) += cdc_ether.o obj-
ifndef CONFIG_USB_RTL8152
obj-$(CONFIG_USB_NET_CDCETHER) += r8153_ecm.o
endif

Though it certainly would be nice to use 8153 devices with the
CDCETHER driver even with the r8152 driver enabled...

Cheers,
- Maciej

--- --- ---

commit 7da17624e7948d5d9660b910f8079d26d26ce453
Author: Geert Uytterhoeven <geert+renesas@...der.be>
Date:   Wed Jan 13 15:43:09 2021 +0100

    nt: usb: USB_RTL8153_ECM should not default to y

    In general, device drivers should not be enabled by default.

    Fixes: 657bc1d10bfc23ac ("r8153_ecm: avoid to be prior to r8152 driver")
    Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
    Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
    Link: https://lore.kernel.org/r/20210113144309.1384615-1-geert+renesas@glider.be
    Signed-off-by: Jakub Kicinski <kuba@...nel.org>

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 1e3719028780..fbbe78643631 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -631,7 +631,6 @@ config USB_NET_AQC111
 config USB_RTL8153_ECM
        tristate "RTL8153 ECM support"
        depends on USB_NET_CDCETHER && (USB_RTL8152 || USB_RTL8152=n)
-       default y
        help
          This option supports ECM mode for RTL8153 ethernet adapter, when
          CONFIG_USB_RTL8152 is not set, or the RTL8153 device is not

Maciej Żenczykowski, Kernel Networking Developer @ Google