[i4l] stop address leakage when shutting down an i4l ppp interface

[i4l] stop address leakage when shutting down an i4l ppp interface

[Paul Bolle]

When an i4l ppp interface is shut down (e.g. with /sbin/ifdown ippp0)
__dev_addr-discard() warns us for "address leakage":
    
    __dev_addr_discard: address leakage! da_users=1
    
The easiest way to stop that leakage is to partly change that interface's
net_device at the end of isdn_net_close() so that it will be acceptable to
arp_mc_map().
    
Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
Please note that I hardly understand what multicast lists are, and why
an i4l ppp interface uses those, etc. I just pinpointed the problem with
a few printk()s. This patch tries to fix only this specific problem
while touching as little as possible (i.e. it only covers the "address
leakage" that I was able to trigger).

Anyhow, a critical review of this patch would be appreciated. 

Sent only to netdev@vger.kernel.org (and not also to
isdn4linux@listserv.isdn4linux.de) because an earlier ISDN patch I wrote
only got a response from this list (and this patch is NETWORKING related
anyway).
---
diff --git a/drivers/isdn/i4l/isdn_net.c b/drivers/isdn/i4l/isdn_net.c
index cb8943d..d58e952 100644
--- a/drivers/isdn/i4l/isdn_net.c
+++ b/drivers/isdn/i4l/isdn_net.c
@@ -1336,6 +1336,13 @@ isdn_net_close(struct net_device *dev)
 	}
 	isdn_net_hangup(dev);
 	isdn_unlock_drivers();
+#ifdef CONFIG_ISDN_PPP
+	/* make sure arp_mc_map() handles this device properly */
+	if (dev->type == ARPHRD_PPP) {
+		dev->type = ARPHRD_ETHER;
+		dev->addr_len = ETH_ALEN;
+	}
+#endif
 	return 0;
 }
 

Re: [i4l] stop address leakage when shutting down an i4l ppp interface

[David Miller]

From: Paul Bolle <pebolle@tiscali.nl>
Date: Mon, 12 Jan 2009 13:54:49 +0100

> @@ -1336,6 +1336,13 @@ isdn_net_close(struct net_device *dev)
>  	}
>  	isdn_net_hangup(dev);
>  	isdn_unlock_drivers();
> +#ifdef CONFIG_ISDN_PPP
> +	/* make sure arp_mc_map() handles this device properly */
> +	if (dev->type == ARPHRD_PPP) {
> +		dev->type = ARPHRD_ETHER;
> +		dev->addr_len = ETH_ALEN;
> +	}
> +#endif
>  	return 0;
>  }

Just like the CONFIG_ISDN_PPP case, this code also changes the
p->dev->type and clears p->dev->addr_len for CONFIG_ISDN_X25

So at a minimum you'd need to add a case for that here as well.

I think it's pretty much illegal to change the device type after the
device has been registered.  Probably a lot of surgery is needed
here to correct this.

Althought what might work is to unregister then re-register the
device when the device type needs to be changed.  That should
work as long as it doesn't cause other undesirable side effects.

Re: [i4l] stop address leakage when shutting down an i4l ppp interface

[Paul Bolle]

It took longer than I hoped to respond to your message.

On Wed, 2009-01-14 at 14:50 -0800, David Miller wrote:
> From: Paul Bolle <pebolle@tiscali.nl>
> > @@ -1336,6 +1336,13 @@ isdn_net_close(struct net_device *dev)
> >  	}
> >  	isdn_net_hangup(dev);
> >  	isdn_unlock_drivers();
> > +#ifdef CONFIG_ISDN_PPP
> > +	/* make sure arp_mc_map() handles this device properly */
> > +	if (dev->type == ARPHRD_PPP) {
> > +		dev->type = ARPHRD_ETHER;
> > +		dev->addr_len = ETH_ALEN;
> > +	}
> > +#endif
> >  	return 0;
> >  }
>
> I think it's pretty much illegal to change the device type after the
> device has been registered.

But isn't that what the i4l code already does?

Bringing up an i4l ppp interface will (in Fedora's ifup-ippp script) do
(among a lot of other stuff):
    isdnctrl addif ippp0
and then: 
    isdnctrl encap ippp0 syncppp

On the kernel side this will result (assuming, of course, I correctly
interpreted the relevant code) in these events: 
    isdn_ioctl(IIOCNETAIF)
    isdn_net_new()
    alloc_netdev()
    _isdn_setup()
    ether_setup()
        [...]
	dev->type = ARPHRD_ETHER
	dev->addr_len = ETH_ALEN
        [...]
    register_netdev() 
    [...]

followed by these events:
    isdn_ioctl(IIOCNETSCF)
    isdn_net_setcfg()
        [...]
        case ISDN_NET_ENCAP_SYNCPPP:
	    p->dev->type = ARPHRD_PPP
	    p->dev->addr_len = 0

This looks to me like it's changing the device type after the device has
been registered. Or were you referring to something else?


Paul