netpoll_setup questions

netpoll_setup questions

[Jeff Moyer]

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.  However, I'm wondering
why we try to force the interface up in the first place?  Just because we
force it up doesn't mean that it will get an IP address.  And, in the case
where it doesn't, you will get an oops further on when dereferencing the
ifa_list.  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?

Thanks!

Jeff

Re: netpoll_setup questions

[Matt Mackall]

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.

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

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

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

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

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

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

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

> 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?

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

-- 
Mathematics is the supreme nostalgia of our time.

Re: netpoll_setup questions

[Jeff Moyer]

==> Regarding Re: netpoll_setup questions; Matt Mackall <mpm@selenic.com> 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);

Re: netpoll_setup questions

[Matt Mackall]

On Wed, Oct 27, 2004 at 02:58:04PM -0400, Jeff Moyer wrote:
> ==> Regarding Re: netpoll_setup questions; Matt Mackall <mpm@selenic.com> 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?

net/ipv4/ipconfig.c, IIRC.
 
> 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:

Yeah..
 
> 	if (!np->local_ip) {

"but if we don't"

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

"we will check if there's an 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);

"if so, look up the device's IP"

> 		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.

Again, does the oops actually happen? I'd expect I'd have seen it by
now if this were really a problem, but I don't mind adding another check.

> 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);

Looks good to me.

-- 
Mathematics is the supreme nostalgia of our time.

Re: netpoll_setup questions

[Jeff Moyer]

==> Regarding Re: netpoll_setup questions; Matt Mackall <mpm@selenic.com> adds:

mpm> On Wed, Oct 27, 2004 at 02:58:04PM -0400, Jeff Moyer wrote:
>> ==> Regarding Re: netpoll_setup questions; Matt Mackall
>> <mpm@selenic.com> 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?

mpm> net/ipv4/ipconfig.c, IIRC.
 
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>

mpm> Yeah..
 
>> if (!np->local_ip) {

mpm> "but if we don't"

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

mpm> "we will check if there's an 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);

mpm> "if so, look up the device's IP"

>> 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.

mpm> Again, does the oops actually happen? I'd expect I'd have seen it by
mpm> now if this were really a problem, but I don't mind adding another
mpm> check.

I'm sorry, I should have said this before.  Yes, it occurs.  You simply
need to have an interface in the down state, plugged into a network that
will not auto-assign an address.  Then, modprobe netconsole, and don't
specify what the local IP address is.  Believe it or not, I have a bug
filed on this.  ;)

-Jeff

Re: netpoll_setup questions

[Matt Mackall]

On Wed, Oct 27, 2004 at 03:44:33PM -0400, Jeff Moyer wrote:
> mpm> Again, does the oops actually happen? I'd expect I'd have seen it by
> mpm> now if this were really a problem, but I don't mind adding another
> mpm> check.
> 
> I'm sorry, I should have said this before.  Yes, it occurs.  You simply
> need to have an interface in the down state, plugged into a network that
> will not auto-assign an address.  Then, modprobe netconsole, and don't
> specify what the local IP address is.  Believe it or not, I have a bug
> filed on this.  ;)

Ok, fair enough. Resend your patch when you've tested it and I'll
forward it on to Andrew.

-- 
Mathematics is the supreme nostalgia of our time.

Re: netpoll_setup questions

[Jeff Moyer]

==> Regarding Re: netpoll_setup questions; Matt Mackall <mpm@selenic.com> adds:

mpm> On Wed, Oct 27, 2004 at 03:44:33PM -0400, Jeff Moyer wrote: Again,
mpm> does the oops actually happen? I'd expect I'd have seen it by now if
mpm> this were really a problem, but I don't mind adding another check.
>> I'm sorry, I should have said this before.  Yes, it occurs.  You simply
>> need to have an interface in the down state, plugged into a network that
>> will not auto-assign an address.  Then, modprobe netconsole, and don't
>> specify what the local IP address is.  Believe it or not, I have a bug
>> filed on this.  ;)

mpm> Ok, fair enough. Resend your patch when you've tested it and I'll
mpm> forward it on to Andrew.

Patch attached, unchanged.  This did indeed fix the problem.

-Jeff

Signed-off by: Jeff Moyer <jmoyer@redhat.com>

--- 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);