netdev - Re: [PATCH] Revert "net: linkwatch: add check for netdevice being present to linkwatch_do

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:   Fri, 18 Sep 2020 12:35:02 +0000
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     "davem@...emloft.net" <davem@...emloft.net>,
        "geert@...ux-m68k.org" <geert@...ux-m68k.org>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "bridge@...ts.linux-foundation.org" 
        <bridge@...ts.linux-foundation.org>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "yoshihiro.shimoda.uh@...esas.com" <yoshihiro.shimoda.uh@...esas.com>,
        "gaku.inami.xh@...esas.com" <gaku.inami.xh@...esas.com>,
        Roopa Prabhu <roopa@...dia.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: Re: [PATCH] Revert "net: linkwatch: add check for netdevice being
 present to linkwatch_do_dev"

On Mon, 2020-09-14 at 09:40 +0200, Geert Uytterhoeven wrote:
> Hi David,
> 
> CC bridge
> 
> On Sun, Sep 13, 2020 at 3:34 AM David Miller <davem@...emloft.net> wrote:
> > From: Geert Uytterhoeven <geert@...ux-m68k.org>
> > Date: Sat, 12 Sep 2020 14:33:59 +0200
> > 
> > > "dev" is not the bridge device, but the physical Ethernet interface, which
> > > may already be suspended during s2ram.
> > 
> > Hmmm, ok.
> > 
> > Looking more deeply NETDEV_CHANGE causes br_port_carrier_check() to run which
> > exits early if netif_running() is false, which is going to be true if
> > netif_device_present() is false:
> > 
> >         *notified = false;
> >         if (!netif_running(br->dev))
> >                 return;
> > 
> > The only other work the bridge notifier does is:
> > 
> >         if (event != NETDEV_UNREGISTER)
> >                 br_vlan_port_event(p, event);
> > 
> > and:
> > 
> >         /* Events that may cause spanning tree to refresh */
> >         if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||
> >                           event == NETDEV_CHANGE || event == NETDEV_DOWN))
> >                 br_ifinfo_notify(RTM_NEWLINK, NULL, p);
> > 
> > So some vlan stuff, and emitting a netlink message to any available
> > listeners.
> > 
> > Should we really do all of this for a device which is not even
> > present?
> > 
> > This whole situation seems completely illogical.  The device is
> > useless, it logically has no link or other state that can be managed
> > or used, while it is not present.
> > 
> > So all of these bridge operations should only happen when the device
> > transitions back to present again.
> 
> Thanks for your analysis!
> I'd like to defer this to the bridge people (CC).
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Hi,
Sorry for the delay. Interesting problem. :)
Thanks for the analysis, I don't see any issues with checking if the device
isn't present. It will have to go through some testing, but no obvious
objections/issues. Have you tried if it fixes your case?
I have briefly gone over drivers' use of net_device_detach(), mostly it's used
for suspends, but there are a few cases which use it on IO error or when a
device is actually detaching (VF detach). The vlan port event is for vlan
devices on top of the bridge when BROPT_VLAN_BRIDGE_BINDING is enabled and their
carrier is changed based on vlan participating ports' state.

Thanks,
 Nik