netdev - Re: [PATCH 1/1] Tell linkwatch about new interfaces

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: <alpine.WNT.2.00.0904091128270.6048@jbrandeb-desk1.amr.corp.intel.com>
Date:	Thu, 9 Apr 2009 17:48:04 -0700 (Pacific Daylight Time)
From:	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>
To:	David Miller <davem@...emloft.net>,
	Andrew Lutomirski <amluto@...il.com>
cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] Tell linkwatch about new interfaces

On Sat, 4 Apr 2009, Andrew Lutomirski wrote:
> On Sat, Apr 4, 2009 at 8:05 PM, David Miller <davem@...emloft.net> wrote:
> > From: Andrew Lutomirski <amluto@...il.com>
> > Date: Wed, 1 Apr 2009 11:40:06 -0400
> >
> >> When a network driver registers a new interface, linkwatch will not notice,
> >> and hence not set the rfc2863 operstate, until netif_carrier_on gets called.
> >> If the new interface has no carrier when it is connected, then a status of
> >> "unknown" is reported to userspace, which confuses various tools
> >> (NetworkManager, for example).
> >>
> >> This fires a linkwatch event for all new interfaces, so that operstate
> >> gets set reasonably quickly.
> >>
> >> Signed-off-by: Andrew Lutomirski <amluto@...il.com>
> >
> > The default assumed state for a freshly registered network
> > device is that the link is up.
> >
> > If that disagrees from reality, the driver should make the
> > appropriate netif_carrier_off() call.
> >
> > I'm sure you'll find that the e1000 driver is not doing this
> > and that is what causes the bug you are seeing.
> >

Dave, if we move the netif_carrier_off call to our dev->open like tg3 has, 
do you think this is sufficient?

I note that we were calling netif_tx_stop_all_queues here, but I'm curious 
if davem thinks we need to lock out our tx routine until dev->open 
completes or whether the starting state of the netdev is sufficient.

> Nope.  The end of e1000_probe (in e1000e) is:
> 
>         /* tell the stack to leave us alone until e1000_open() is called */
>         netif_carrier_off(netdev);
>         netif_tx_stop_all_queues(netdev);
> 
>         strcpy(netdev->name, "eth%d");
>         err = register_netdev(netdev);
>         if (err)
>                 goto err_register;
> 
>         e1000_print_device_info(adapter);
> 
> netif_carrier_off does:
> 
> void netif_carrier_off(struct net_device *dev)
> {
>         if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
>                 if (dev->reg_state == NETREG_UNINITIALIZED)
>                         return;
>                 linkwatch_fire_event(dev);
>         }
> }
> 
> So, either it should be illegal to call netif_carrier_off on an
> unregistered net_device (and there should be a WARN() in
> netif_carrier_off), or it should work correctly, and
> register_netdevice should do the right thing when there is no carrier
> (i.e. something like my patch).

does this patch also fix the issue?

===== begin =====

e1000e: indicate link down at load

From: Jesse Brandeburg <jesse.brandeburg@...el.com>

same kind of patch as e1000, let driver explicitly push link state
once link comes up.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
---

 drivers/net/e1000e/netdev.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index fb78278..6a0411e 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -3072,6 +3072,8 @@ static int e1000_open(struct net_device *netdev)
 	if (test_bit(__E1000_TESTING, &adapter->state))
 		return -EBUSY;
 
+	netif_carrier_off(netdev);
+
 	/* allocate transmit descriptors */
 	err = e1000e_setup_tx_resources(adapter);
 	if (err)
@@ -5006,10 +5008,6 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 	if (!(adapter->flags & FLAG_HAS_AMT))
 		e1000_get_hw_control(adapter);
 
-	/* tell the stack to leave us alone until e1000_open() is called */
-	netif_carrier_off(netdev);
-	netif_tx_stop_all_queues(netdev);
-
 	strcpy(netdev->name, "eth%d");
 	err = register_netdev(netdev);
 	if (err)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html