vlan: update vlan carrier state for admin up/down

vlan: update vlan carrier state for admin up/down

[Patrick McHardy]

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



[-- Attachment #2: 01.diff --]
[-- Type: text/x-patch, Size: 2650 bytes --]

commit fcfdf9ecbaf759b658a9722af2862e595f2dec6b
Author: Jay Vosburgh <fubar@us.ibm.com>
Date:   Fri Apr 24 17:22:08 2009 +0200

    vlan: update vlan carrier state for admin up/down
    
    	Currently, the VLAN event handler does not adjust the VLAN
    device's carrier state when the real device or the VLAN device is set
    administratively up or down.
    
    	The following patch adds a transfer of operating state from the
    real device to the VLAN device when the real device is administratively
    set up or down, and sets the carrier state up or down during init, open
    and close of the VLAN device.
    
    	This permits observers above the VLAN device that care about the
    carrier state (bonding's link monitor, for example) to receive updates
    for administrative changes by more closely mimicing the behavior of real
    devices.
    
    Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 2b7390e..d1e1054 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -492,6 +492,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 				continue;
 
 			dev_change_flags(vlandev, flgs & ~IFF_UP);
+			vlan_transfer_operstate(dev, vlandev);
 		}
 		break;
 
@@ -507,6 +508,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 				continue;
 
 			dev_change_flags(vlandev, flgs | IFF_UP);
+			vlan_transfer_operstate(dev, vlandev);
 		}
 		break;
 
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 1b34135..2ce6658 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -462,6 +462,7 @@ static int vlan_dev_open(struct net_device *dev)
 	if (vlan->flags & VLAN_FLAG_GVRP)
 		vlan_gvrp_request_join(dev);
 
+	netif_carrier_on(dev);
 	return 0;
 
 clear_allmulti:
@@ -471,6 +472,7 @@ del_unicast:
 	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
 		dev_unicast_delete(real_dev, dev->dev_addr, ETH_ALEN);
 out:
+	netif_carrier_off(dev);
 	return err;
 }
 
@@ -492,6 +494,7 @@ static int vlan_dev_stop(struct net_device *dev)
 	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
 		dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
 
+	netif_carrier_off(dev);
 	return 0;
 }
 
@@ -612,6 +615,8 @@ static int vlan_dev_init(struct net_device *dev)
 	struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
 	int subclass = 0;
 
+	netif_carrier_off(dev);
+
 	/* IFF_BROADCAST|IFF_MULTICAST; ??? */
 	dev->flags  = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI);
 	dev->iflink = real_dev->ifindex;

Re: vlan: update vlan carrier state for admin up/down

[Ben Greear]

Suppose that someone has eth0, eth0.2 and eth0.5 admin UP.

Then, sets eth0.5 admin DOWN.

Later they set eth0 down and up for some reason.

Would eth0.5 now go admin UP?

If so, I think that is not a good idea and could possibly be considered
a security issue.  I think instead there should be a new flag for VLANs
that is 'preferred-admin-state'.  To be UP, both the underlying device
and the preferred state must be UP.  That should allow bouncing eth0
w/out affecting the eventual admin state of the VLANs on eth0 in
the example above.

For what it's worth, it seems that other virtual devices, such as VIFS
on a wifi radio, might need the same sort of behaviour.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: vlan: update vlan carrier state for admin up/down

[Jay Vosburgh]

Ben Greear <greearb@candelatech.com> wrote:

>Suppose that someone has eth0, eth0.2 and eth0.5 admin UP.
>
>Then, sets eth0.5 admin DOWN.
>
>Later they set eth0 down and up for some reason.
>
>Would eth0.5 now go admin UP?

	Yes, with the patch applied, it does.

>If so, I think that is not a good idea and could possibly be considered
>a security issue.  I think instead there should be a new flag for VLANs
>that is 'preferred-admin-state'.  To be UP, both the underlying device
>and the preferred state must be UP.  That should allow bouncing eth0
>w/out affecting the eventual admin state of the VLANs on eth0 in
>the example above.

	I dunno if it's a security issue, but I'd agree it's wrong.  I
looked, and I suspect that a couple of judiciously placed "if
(dev->flags & IFF_UP)" bits ought to sort things out by simply not
copying the carrier state to the upper level VLAN device if that device
is down.  Unless somebody works this up over the weekend, I'll work that
out on Monday.

	When the vlan device (eth0.5 in the above example) later is set
up, it'll do the right thing for its own carrier state.

>For what it's worth, it seems that other virtual devices, such as VIFS
>on a wifi radio, might need the same sort of behaviour.

	Would they actually need a flag, or could it be worked out just
by checking their IFF_UP-ness?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: vlan: update vlan carrier state for admin up/down

[Ben Greear]

Jay Vosburgh wrote:
>> If so, I think that is not a good idea and could possibly be considered
>> a security issue.  I think instead there should be a new flag for VLANs
>> that is 'preferred-admin-state'.  To be UP, both the underlying device
>> and the preferred state must be UP.  That should allow bouncing eth0
>> w/out affecting the eventual admin state of the VLANs on eth0 in
>> the example above.
>>     
>
> 	I dunno if it's a security issue, but I'd agree it's wrong.  I
> looked, and I suspect that a couple of judiciously placed "if
> (dev->flags & IFF_UP)" bits ought to sort things out by simply not
> copying the carrier state to the upper level VLAN device if that device
> is down.  Unless somebody works this up over the weekend, I'll work that
> out on Monday.
>   
That may be fine.  I suppose you'll also be ignoring admin state changes 
for the
underlying devices in the VLAN code?  Ie, if eth0 goes admin down, you won't
change the eth0.5 vlan to be admin down?


> 	Would they actually need a flag, or could it be worked out just
> by checking their IFF_UP-ness?
>   
If no admin state is propagated at all, I think you can skip any extra 
flags.  But,
if you want to propagate admin state (as the kernel does now with VLANs),
then I don't see how you can get by without another flag.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: vlan: update vlan carrier state for admin up/down

[Jay Vosburgh]

Ben Greear <greearb@candelatech.com> wrote:

>Jay Vosburgh wrote:
>>> If so, I think that is not a good idea and could possibly be considered
>>> a security issue.  I think instead there should be a new flag for VLANs
>>> that is 'preferred-admin-state'.  To be UP, both the underlying device
>>> and the preferred state must be UP.  That should allow bouncing eth0
>>> w/out affecting the eventual admin state of the VLANs on eth0 in
>>> the example above.
>>>     
>>
>> 	I dunno if it's a security issue, but I'd agree it's wrong.  I
>> looked, and I suspect that a couple of judiciously placed "if
>> (dev->flags & IFF_UP)" bits ought to sort things out by simply not
>> copying the carrier state to the upper level VLAN device if that device
>> is down.  Unless somebody works this up over the weekend, I'll work that
>> out on Monday.
>>   
>That may be fine.  I suppose you'll also be ignoring admin state changes
>for the
>underlying devices in the VLAN code?  Ie, if eth0 goes admin down, you won't
>change the eth0.5 vlan to be admin down?

	Ok, I did a bit of testing on this, and I don't believe the
"update vlan carrier state" patch I did changed anything in this regard.

	Without the patch, given a configuration of eth0 with eth0.600
and eth0.700 stacked above, the sequence "ifconfig eth0.700 down;
ifconfig eth0 down; ifconfig eth0 up" results in eth0.700 being up at
the end, and both eth0.600 and eth0.700 being down when eth0 is down (up
and down here referring to IFF_UP, not carrier state).  The VLAN event
handler (vlan_device_event) deliberately sets all VLAN devices to match
the real device when the real device goes up or down.

	I still agree that this is a bit counterintuitive, but the patch
doesn't change the VLAN behavior in this regard.

	With the patch, the same thing happens, but the carrier state
also changes to match (without the patch, the VLAN interfaces remain
carrier up the whole time).

	I'm reluctant to mess with this behavior without knowing why it
works this way; there may be a good reason for it that I'm not aware of.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: vlan: update vlan carrier state for admin up/down

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Mon, 27 Apr 2009 16:55:58 -0700

> I'm reluctant to mess with this behavior without knowing why it
> works this way; there may be a good reason for it that I'm not aware
> of.

To be honest I think it's just that this is one huge dark
corner of behavior for many virtual devices, rather than
any of it being intentional.

Re: vlan: update vlan carrier state for admin up/down

[Ben Greear]

David Miller wrote:
> From: Jay Vosburgh <fubar@us.ibm.com>
> Date: Mon, 27 Apr 2009 16:55:58 -0700
> 
>> I'm reluctant to mess with this behavior without knowing why it
>> works this way; there may be a good reason for it that I'm not aware
>> of.
> 
> To be honest I think it's just that this is one huge dark
> corner of behavior for many virtual devices, rather than
> any of it being intentional.

For what it's worth, I've been using a patch that disables the
behaviour in vlan that brings up/down the interface based on
the underlying device for several years w/out noticeable problems.

However, I may just be getting lucky.

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: vlan: update vlan carrier state for admin up/down

[Patrick McHardy]

David Miller wrote:
> From: Jay Vosburgh <fubar@us.ibm.com>
> Date: Mon, 27 Apr 2009 16:55:58 -0700
> 
>> I'm reluctant to mess with this behavior without knowing why it
>> works this way; there may be a good reason for it that I'm not aware
>> of.
> 
> To be honest I think it's just that this is one huge dark
> corner of behavior for many virtual devices, rather than
> any of it being intentional.

Indeed. In case of VLANs it has always been this way and I'm
reluctant to change the default behaviour since people might
be relying on this to flush their routes or something similar.

For new drivers I'd say they should always use operstate. For
VLAN the safest way would be to add a flag to disable this
behaviour.

Re: vlan: update vlan carrier state for admin up/down

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 24 Apr 2009 17:44:47 +0200

>     vlan: update vlan carrier state for admin up/down
>     
>     	Currently, the VLAN event handler does not adjust the VLAN
>     device's carrier state when the real device or the VLAN device is set
>     administratively up or down.
>     
>     	The following patch adds a transfer of operating state from the
>     real device to the VLAN device when the real device is administratively
>     set up or down, and sets the carrier state up or down during init, open
>     and close of the VLAN device.
>     
>     	This permits observers above the VLAN device that care about the
>     carrier state (bonding's link monitor, for example) to receive updates
>     for administrative changes by more closely mimicing the behavior of real
>     devices.
>     
>     Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

I've applied this to net-2.6, thanks!

I realize there are some more issues to deal with wrt. the
comments Ben made.  And I eagerly await patches for that :)