netdev - RE: netdev carrier changes is one even after ethernet link up.

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, 1 Sep 2017 05:49:12 +0000
From:   Bhadram Varka <vbhadram@...dia.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     linux-netdev <netdev@...r.kernel.org>
Subject: RE: netdev carrier changes is one even after ethernet link up.

Thanks for responding. Now responding inline

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@...il.com]
> Sent: Friday, September 01, 2017 5:53 AM
> To: Bhadram Varka <vbhadram@...dia.com>; andrew@...n.ch
> Cc: linux-netdev <netdev@...r.kernel.org>
> Subject: Re: netdev carrier changes is one even after ethernet link up.
> 
> On 08/30/2017 10:53 PM, Bhadram Varka wrote:
> > Hi,
> >
> >
> >
> > I have observed that carrier_changes is one even in case of the
> > ethernet link is up.
> >
> >
> >
> > After investigating the code below is my observation –
> >
> >
> >
> > ethernet_driver_probe()
> >
> > +--->phy_connect()
> >
> > |     +--->phy_attach_direct()
> >
> > |           +---> netif_carrier_off()    : which increments
> > carrier_changes to one.
> >
> > +--->register_netdevice() : will the carrier_changes becomes zero here ?
> >
> > +--->netif_carrier_off(): not increment the carrier_changes since
> > __LINK_STATE_NOCARRIER already set.
> >
> >
> >
> > From ethernet driver open will start the PHY and trigger the
> > phy_state_machine.
> >
> > Phy_state_machine workqueue calling netif_carrier_on() once the link is
> UP.
> >
> > netif_carrier_on() increments the carrier_changes by one.
> 
> If the call trace is correct, then there is at least two problems here:
> 
> - phy_connect() does start the PHY machine which means that as soon as it
> detects a link state of any kind (up or down) it can call
> netif_carrier_off() respectively netif_carrier_on()
> 
> - as soon as you call register_netdevice() notifiers run and other parts of the
> kernel or user-space programs can see an inconsistent link state
> 
> I would suggest doing the following sequence instead:
> 
> netif_carrier_off()
> register_netdevice()
> phy_connect()
> 
> Which should result in a consistent link state and carrier value.
> 
Yes, It will address the issue. 

If we did the phy_conect in ndo_open it will make the carrier changes as two. But if we did in probe function then it's not working.

In ethernet driver probe - (below sequence is not working)
phy_connect()
register_netdevice()
netif_carrier_off()

working sequence:
In probe():
register_netdevice()
ndo_open:
           phy_connect()

After reverting - https://lkml.org/lkml/2016/1/9/173 this works if we do phy_connect in probe as well.

Thanks,
Bhadram.
> >
> >
> >
> > After link is UP if we check the carrier_changes sysfs node - it will
> > be one only.
> >
> >
> >
> > $ cat /sys/class/net/eth0/carrier_changes
> >
> > 1
> >
> >
> >
> > After reverting the change - https://lkml.org/lkml/2016/1/9/173 (net:
> > phy: turn carrier off on phy attach) then I could see the carrier
> > changes incremented to 2 after Link UP.
> >
> > $ cat /sys/class/net/eth0/carrier_changes
> >
> > 2
> >
> >
> >
> > Thanks,
> >
> > Bhadram.
> >
> > ----------------------------------------------------------------------
> > -- This email message is for the sole use of the intended recipient(s)
> > and may contain confidential information.  Any unauthorized review,
> > use, disclosure or distribution is prohibited.  If you are not the
> > intended recipient, please contact the sender by reply email and
> > destroy all copies of the original message.
> > ----------------------------------------------------------------------
> > --
> 
> 
> --
> Florian