Re: netpoll_setup questions

[Jeff Moyer]

==> Regarding Re: netpoll_setup questions; Matt Mackall <mpm@xxxxxxxxxxx> adds:

mpm> On Wed, Oct 27, 2004 at 11:50:05AM -0400, Jeff Moyer wrote:
>> Hi, Matt,
>> 
>> The section of code in the body of this if statement:
>> 
>> if (!(ndev->flags & IFF_UP)) {
>> 
>> is a bit broken.  First, upon discussion with jgarzik, it seems we
>> should not check for IFF_UP, but instead do netif_running.

mpm> Well I cribbed it from the nfsroot code, which is perhaps not up on
mpm> fashion.

Hmm, I can't seem to find the relevant code.  Where is it?

>> However, I'm wondering why we try to force the interface up in the first
>> place?

mpm> Uh, so we can send packets before userspace has configured the
mpm> network?

Heh.  right.

>> Just because we force it up doesn't mean that it will get an IP address.

mpm> I don't expect it does. Usually we have our own IP address from the
mpm> command line, but if we don't, we will check if there's an in_dev
mpm> connected to the device and if so, look up the device's IP. This is
mpm> useful for the modular case where the network is up before we start
mpm> and we have a handy default for local IP.

Right, but the case in question is the one where we don't have a local IP
specified:

	if (!np->local_ip) {
		rcu_read_lock();
		in_dev = __in_dev_get(ndev);

		if (!in_dev) {
			rcu_read_unlock();
			printk(KERN_ERR "%s: no IP address for %s, aborting\n",
			       np->name, np->dev_name);
			goto release;
		}

		np->local_ip = ntohl(in_dev->ifa_list->ifa_local);
		rcu_read_unlock();
		printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
		       np->name, HIPQUAD(np->local_ip));
	}


>> And, in the case where it doesn't, you will get an oops further on when
>> dereferencing the ifa_list.

mpm> Does this actually happen? I'm checking in_dev for null already, but
mpm> perhaps I need to check ifa_list as well.

Yes, that should do it.

>> So, why does this section of code exist at all? If it has a good
>> purpose, can we replace it with a call to ndev->open?

mpm> Maybe. We should probably revisit the nfsroot code as well.

Right, just point me at it.

Here is a patch which should work.  I'll test today.

-Jeff

--- linux-2.6.9/net/core/netpoll.c~	2004-10-21 17:49:10.000000000 -0400
+++ linux-2.6.9/net/core/netpoll.c	2004-10-27 14:53:57.492149880 -0400
@@ -571,7 +571,7 @@ int netpoll_setup(struct netpoll *np)
 		goto release;
 	}
 
-	if (!(ndev->flags & IFF_UP)) {
+	if (!netif_running(ndev)) {
 		unsigned short oflags;
 		unsigned long atmost, atleast;
 
@@ -617,7 +617,7 @@ int netpoll_setup(struct netpoll *np)
 		rcu_read_lock();
 		in_dev = __in_dev_get(ndev);
 
-		if (!in_dev) {
+		if (!in_dev || !in_dev->ifa_list) {
 			rcu_read_unlock();
 			printk(KERN_ERR "%s: no IP address for %s, aborting\n",
 			       np->name, np->dev_name);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/