[PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Paul Bolle]

From: Paul Bolle <pebolle@tiscali.nl>
   
Commit bada339 (Validate device addr prior to interface-up) caused a regression
in the ISDN network code, see: http://bugzilla.kernel.org/show_bug.cgi?id=9923
The trivial fix is to remove the pointer to eth_validate_addr() in the
net_device struct in isdn_net_init().
    
Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
Please note that I hardly know what the device address validation is good for.
Neither do I know much about the ISDN network code or why the device address
validation fails for ISDN interfaces. This patch however basically reverts 
commit bada339 just for the ISDN network code. I can't think of any side
effects.

http://bugzilla.kernel.org/show_bug.cgi?id=9923 would have been much easier to
track down if eth_validate_addr() would somehow complain aloud if an address 
is invalid. Shouldn't it make at least some noise?

diff --git a/drivers/isdn/i4l/isdn_net.c b/drivers/isdn/i4l/isdn_net.c
index ced83c2..ef1a300 100644
--- a/drivers/isdn/i4l/isdn_net.c
+++ b/drivers/isdn/i4l/isdn_net.c
@@ -2010,6 +2010,7 @@ isdn_net_init(struct net_device *ndev)
 	ndev->flags = IFF_NOARP|IFF_POINTOPOINT;
 	ndev->type = ARPHRD_ETHER;
 	ndev->addr_len = ETH_ALEN;
+	ndev->validate_addr = NULL;
 
 	/* for clients with MPPP maybe higher values better */
 	ndev->tx_queue_len = 30;

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

Paul Bolle wrote:
> From: Paul Bolle <pebolle@tiscali.nl>
>    
> Commit bada339 (Validate device addr prior to interface-up) caused a regression
> in the ISDN network code, see: http://bugzilla.kernel.org/show_bug.cgi?id=9923
> The trivial fix is to remove the pointer to eth_validate_addr() in the
> net_device struct in isdn_net_init().
>     
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> Please note that I hardly know what the device address validation is good for.
> Neither do I know much about the ISDN network code or why the device address
> validation fails for ISDN interfaces. This patch however basically reverts 
> commit bada339 just for the ISDN network code. I can't think of any side
> effects.

The patch looks fine. is_valid_ether_addr() fails because the ISDN
device's address is 00:00:00:00:00:00 at this point.

> http://bugzilla.kernel.org/show_bug.cgi?id=9923 would have been much easier to
> track down if eth_validate_addr() would somehow complain aloud if an address 
> is invalid. Shouldn't it make at least some noise?

I guess it should return -EADDRNOTAVAIL similar to eth_mac_addr()
when validation fails.

Signed-off-by: Patrick McHardy <kaber@trash.net>


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 378 bytes --]

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index a7b4175..a80839b 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -301,7 +301,7 @@ static int eth_change_mtu(struct net_device *dev, int new_mtu)
 static int eth_validate_addr(struct net_device *dev)
 {
 	if (!is_valid_ether_addr(dev->dev_addr))
-		return -EINVAL;
+		return -EADDRNOTAVAIL;
 
 	return 0;
 }

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Patrick McHardy]

Jarek Poplawski wrote:
> Patrick McHardy wrote, On 04/13/2008 08:29 AM:
> ...
>
>   
>> I guess it should return -EADDRNOTAVAIL similar to eth_mac_addr()
>> when validation fails.
>>     
>
> I think an additional printk should really save some time in guessing
> what's wrong and with which address, especially if the same config
> works OK with earlier kernels.
>   

In my opinion a proper error code like EADDRNOTAVAIL is enough, we
have thousands of checks that might make things fail, should we
add a printk to every one of those?

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Jarek Poplawski]

Patrick McHardy wrote, On 04/13/2008 08:29 AM:
...

> I guess it should return -EADDRNOTAVAIL similar to eth_mac_addr()
> when validation fails.

I think an additional printk should really save some time in guessing
what's wrong and with which address, especially if the same config
works OK with earlier kernels.

Regards,
Jarek P.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Patrick McHardy]

Jarek Poplawski wrote:
> On Sun, Apr 13, 2008 at 10:05:08AM +0200, Patrick McHardy wrote:
> ...
>   
>> In my opinion a proper error code like EADDRNOTAVAIL is enough, we
>> have thousands of checks that might make things fail, should we
>> add a printk to every one of those?
>>     
>
> IMHO yes - but only when "the functionality" is changed causing
> regressions like this.

This one is simply a bug, not a functional change.

>  BTW, I've assisted some similar case in
> bugzilla, and I guess users will still ask for an explanation after
> this error code change.
>   

Well, I don't have particulary strong feelings about this, but
I think adding a printk to one out of thousands of possible
errors won't help much.

What would be nice is some extended error reporting for netlink.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Jarek Poplawski]

On Sun, Apr 13, 2008 at 10:05:08AM +0200, Patrick McHardy wrote:
...
> In my opinion a proper error code like EADDRNOTAVAIL is enough, we
> have thousands of checks that might make things fail, should we
> add a printk to every one of those?

IMHO yes - but only when "the functionality" is changed causing
regressions like this. BTW, I've assisted some similar case in
bugzilla, and I guess users will still ask for an explanation after
this error code change.

Jarek P.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Jarek Poplawski]

On Sun, Apr 13, 2008 at 10:26:49AM +0200, Patrick McHardy wrote:
...
> This one is simply a bug, not a functional change.

I agree 100%. But since it worked for some people for long time...

>>  BTW, I've assisted some similar case in
>> bugzilla, and I guess users will still ask for an explanation after
>> this error code change.
>>   
>
> Well, I don't have particulary strong feelings about this, but
> I think adding a printk to one out of thousands of possible
> errors won't help much.

Maybe you are right, we can simply wait. I know Paul, me and a few
more people lost "a bit" of time because of this fix already, and
I hope the next time there will be somebody like you around to give
an advice before git bisect was finished or a few kB of logs are sent
here.

Jarek P.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[David Miller]

From: Paul Bolle <pebolle@tiscali.nl>
Date: Thu, 10 Apr 2008 17:00:17 +0200

> From: Paul Bolle <pebolle@tiscali.nl>
>    
> Commit bada339 (Validate device addr prior to interface-up) caused a regression
> in the ISDN network code, see: http://bugzilla.kernel.org/show_bug.cgi?id=9923
> The trivial fix is to remove the pointer to eth_validate_addr() in the
> net_device struct in isdn_net_init().
>     
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>

Applied, thanks a lot Paul.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sun, 13 Apr 2008 08:29:51 +0200

> I guess it should return -EADDRNOTAVAIL similar to eth_mac_addr()
> when validation fails.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks Patrick.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Patrick McHardy]

Jarek Poplawski wrote:
> On 14-04-2008 07:44, David Miller wrote:
>   
>> From: Paul Bolle <pebolle@tiscali.nl>
>> Date: Thu, 10 Apr 2008 17:00:17 +0200
>>
>>     
>>> From: Paul Bolle <pebolle@tiscali.nl>
>>>    
>>> Commit bada339 (Validate device addr prior to interface-up) caused a regression
>>> in the ISDN network code, see: http://bugzilla.kernel.org/show_bug.cgi?id=9923
>>> The trivial fix is to remove the pointer to eth_validate_addr() in the
>>> net_device struct in isdn_net_init().
>>>     
>>> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
>>>       
>> Applied, thanks a lot Paul.
>>     
>
> Maybe I missed Patrick point, but it seems this patch isn't good:
> it omits address validation hiding possible problems.
The ISDN device's address is computed from the IP addresses
in ->open(), so there is nothing to validate at the time
eth_validate_addr() is called.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Jarek Poplawski]

On 14-04-2008 07:44, David Miller wrote:
> From: Paul Bolle <pebolle@tiscali.nl>
> Date: Thu, 10 Apr 2008 17:00:17 +0200
> 
>> From: Paul Bolle <pebolle@tiscali.nl>
>>    
>> Commit bada339 (Validate device addr prior to interface-up) caused a regression
>> in the ISDN network code, see: http://bugzilla.kernel.org/show_bug.cgi?id=9923
>> The trivial fix is to remove the pointer to eth_validate_addr() in the
>> net_device struct in isdn_net_init().
>>     
>> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> 
> Applied, thanks a lot Paul.

Maybe I missed Patrick point, but it seems this patch isn't good:
it omits address validation hiding possible problems.

Regards,
Jarek P.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[Jarek Poplawski]

On Mon, Apr 14, 2008 at 07:57:31AM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
...
>> Maybe I missed Patrick point, but it seems this patch isn't good:
>> it omits address validation hiding possible problems.
> The ISDN device's address is computed from the IP addresses
> in ->open(), so there is nothing to validate at the time
> eth_validate_addr() is called.

...So I missed the point, sorry!

Thanks for the explanation,
Jarek P.

Re: [PATCH] [NET] [ISDN]: Do not validate ISDN net device address prior to interface-up

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 14 Apr 2008 07:57:31 +0200

> Jarek Poplawski wrote:
> > Maybe I missed Patrick point, but it seems this patch isn't good:
> > it omits address validation hiding possible problems.
> The ISDN device's address is computed from the IP addresses
> in ->open(), so there is nothing to validate at the time
> eth_validate_addr() is called.

Right.